Fix cases where fast instruction selection generated both base and index registers

Fix cases where fast instruction selection generated both
base and index registers.

We fall back to normal instruction selection in that case.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2544

[Karl, 2012-07-24 20:44:33]

Please review. Thanks.

[Derek Schuff, 2012-07-24 21:07:47]

otherwise LGTM

don't forget before you commit to make sure your local hg is up to date (hg pull
and hg update) so we don't get multiple heads. Also don't forget to add a Review
URL: marker to the hg commit message.

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
File llvm/lib/Target/X86/X86NaClRewritePass.cpp (right):

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
llvm/lib/Target/X86/X86NaClRewritePass.cpp:539: // @LOCALMOD-START
no need for LOCALMODs here, this entire file is a local mod

[jvoung - send to chromium..., 2012-07-24 21:08:35]

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86FastISe...
File llvm/lib/Target/X86/X86FastISel.cpp (right):

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86FastISe...
llvm/lib/Target/X86/X86FastISel.cpp:585: if ((AM.Base.Reg == 0) && (AM.IndexReg
== 0)) {
You can avoid changing the original behavior for non-NaCl targets, by checking
if we are generating code for NaCl first.  E.g.,:


"if (Subtarget->isTargetNaCl()) {
  ... localmod ...
} else {
  ... original code ...
}"

Then it also makes more obvious what the difference is.

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
File llvm/lib/Target/X86/X86NaClRewritePass.cpp (right):

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
llvm/lib/Target/X86/X86NaClRewritePass.cpp:553: }
} else {
  // Original behavior the zero-based sandbox?
} 

Or is that impossible (covered by the "if (!BaseReg.getReg())" ) ?

If it's impossible, can you put an llvm_unreachable("some error message"); ?

[robertm, 2012-07-24 21:20:17]

+1 for using if guard statements.


On 2012/07/24 21:08:35, jvoung wrote:
>
http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86FastISe...
> File llvm/lib/Target/X86/X86FastISel.cpp (right):
> 
>
http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86FastISe...
> llvm/lib/Target/X86/X86FastISel.cpp:585: if ((AM.Base.Reg == 0) &&
(AM.IndexReg
> == 0)) {
> You can avoid changing the original behavior for non-NaCl targets, by checking
> if we are generating code for NaCl first.  E.g.,:
> 
> 
> "if (Subtarget->isTargetNaCl()) {
>   ... localmod ...
> } else {
>   ... original code ...
> }"
> 
> Then it also makes more obvious what the difference is.
> 
>
http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
> File llvm/lib/Target/X86/X86NaClRewritePass.cpp (right):
> 
>
http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
> llvm/lib/Target/X86/X86NaClRewritePass.cpp:553: }
> } else {
>   // Original behavior the zero-based sandbox?
> } 
> 
> Or is that impossible (covered by the "if (!BaseReg.getReg())" ) ?
> 
> If it's impossible, can you put an llvm_unreachable("some error message"); ?

[Karl, 2012-07-24 22:05:30]

Please take another look. Thanks.

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86FastISe...
File llvm/lib/Target/X86/X86FastISel.cpp (right):

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86FastISe...
llvm/lib/Target/X86/X86FastISel.cpp:585: if ((AM.Base.Reg == 0) && (AM.IndexReg
== 0)) {
On 2012/07/24 21:08:35, jvoung wrote:
> You can avoid changing the original behavior for non-NaCl targets, by checking
> if we are generating code for NaCl first.  E.g.,:
> 
> 
> "if (Subtarget->isTargetNaCl()) {
>   ... localmod ...
> } else {
>   ... original code ...
> }"
> 
> Then it also makes more obvious what the difference is.

Done.

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
File llvm/lib/Target/X86/X86NaClRewritePass.cpp (right):

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
llvm/lib/Target/X86/X86NaClRewritePass.cpp:539: // @LOCALMOD-START
On 2012/07/24 21:07:47, dschuff wrote:
> no need for LOCALMODs here, this entire file is a local mod

Done.

http://codereview.chromium.org/10808110/diff/1/llvm/lib/Target/X86/X86NaClRew...
llvm/lib/Target/X86/X86NaClRewritePass.cpp:553: }
On 2012/07/24 21:08:35, jvoung wrote:
> } else {
>   // Original behavior the zero-based sandbox?
> } 
> 
> Or is that impossible (covered by the "if (!BaseReg.getReg())" ) ?
> 
> If it's impossible, can you put an llvm_unreachable("some error message"); ?

Done.

[jvoung - send to chromium..., 2012-07-24 22:19:21]

lgtm