Enhance address mode recovery

Enhance address mode recovery

This adds some more patterns to address mode recovery to recover
ConstantRelocatables as displacements, and a few more generalizations that
catch indexed addressing.

BUG=
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=aa0b1a176f6c3306be3e95374d72e735bab2bbab

[sehr, 2015-10-27 00:30:02]

Description was changed from

==========
Enhance address mode recovery

This adds some more patterns to address mode recovery to recover
ConstantRelocatables as displacements, and a few more generalizations that
catch indexed addressing.

BUG=
==========

to

==========
Enhance address mode recovery

This adds some more patterns to address mode recovery to recover
ConstantRelocatables as displacements, and a few more generalizations that
catch indexed addressing.

BUG=
==========

[sehr, 2015-10-27 00:30:03]

sehr@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org

[sehr, 2015-10-27 00:33:05]

I found the relocation issue with Jim's gracious assistance.  Unfortunately, the
fix is a little tedious.  Please take a look at my handiwork to Traits
(Jim/Joao).  If the solution there is acceptable, I will add it to the x8664
side also.  And there's one subtle bit-encoding change for modrm/sib stuff in
src/IceAssemblerX86BaseImpl.h (Karl). I would appreciate it if you agree with my
reading of:
http://wiki.osdev.org/X86-64_Instruction_Encoding#ModR.2FM_and_SIB_bytes

Thanks in advance.

[Jim Stichnoth, 2015-10-27 05:52:34]

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:185: Address(GPRRegister base, int32_t disp,
AssemblerFixup *fixup = nullptr) {
Maybe we can make fixup a required parameter, rather than default-value?

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:2854: // Mov Src0HiRM first, because it was
legalized most recently,
Can you reformat this and other comments to 80-cols?

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4101: if (const InstArithmetic *Arith =
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4113: /*
Why is this commented out?

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4148: if (Variable *SrcVar =
llvm::dyn_cast<Variable>(SrcOp)) {
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4156: } else if (auto Const =
llvm::dyn_cast<ConstantInteger32>(SrcOp)) {
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4158: if (Utils::WouldOverflowAdd(Offset,
MoreOffset))
Is overflow an issue for x86-32?  Can it be disallowed only for x86-64?

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4164: } else if (auto AddReloc =
llvm::dyn_cast<ConstantRelocatable>(SrcOp)) {
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4225: if (const InstArithmetic *ArithInst =
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4227: if (Variable *Var =
llvm::dyn_cast<Variable>(ArithInst->getSrc(0))) {
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4228: if (ConstantInteger32 *Const =
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4300: if (const InstArithmetic *ArithInst =
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4308: Variable *Var0 =
llvm::dyn_cast<Variable>(Src0);
auto * for all of these

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4317: if ((Var0 != nullptr) && (Var1 !=
nullptr))
In these cases, I think omitting the "!= nullptr" would be more readable.

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4342: if (Utils::WouldOverflowAdd(NewOffset,
MoreOffset))
Same 32-bit overflow question as above.

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4542: Variable *Base =
llvm::dyn_cast<Variable>(Addr);
auto *

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4751: Variable *Base =
llvm::dyn_cast<Variable>(Addr);
auto *

https://codereview.chromium.org/1428443002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/address-mode-global.ll (right):

https://codereview.chromium.org/1428443002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/address-mode-global.ll:36: 
git doesn't like "trailing whitespace".

[Jim Stichnoth, 2015-10-27 13:57:10]

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:195: // TODO(sehr): determine if there are
8-bit relocations.
Here and in the TODO below, if there is a fixup, I think you need to just use a
32-bit displacement.  For example, here's a mashup of simple_loop.ll and
globalrelocs.ll:

@bytes = internal global [8 x i8] c"abcdefgh"
define internal i32 @simple_loop(i32 %a, i32 %n) {
entry:
  %cmp4 = icmp sgt i32 %n, 0
  br i1 %cmp4, label %for.body, label %for.end
for.body:
  %i.06 = phi i32 [ %inc, %for.body ], [ 0, %entry ]
  %sum.05 = phi i32 [ %add, %for.body ], [ 0, %entry ]
  %gep_array = mul i32 %i.06, 4
  %gep = add i32 %a, %gep_array
  %baddr = ptrtoint [8 x i8] * @bytes to i32
  %gep2 = add i32 %gep, %baddr
  %gep3 = add i32 %gep2, 4
  %__9 = inttoptr i32 %gep3 to i32*
  %v0 = load i32, i32* %__9, align 1
  %add = add i32 %v0, %sum.05
  %inc = add i32 %i.06, 1
  %cmp = icmp slt i32 %inc, %n
  br i1 %cmp, label %for.body, label %for.end
for.end:
  %sum.0.lcssa = phi i32 [ 0, %entry ], [ %add, %for.body ]
  ret i32 %sum.0.lcssa
}

Now, change %gep3 to add 1024 instead of 4, and it works correctly.

More generally, the first two branches of this if/then/else chain should not
fire when fixup!=nullptr, because disp is really not an 8-bit 0.

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:195: // TODO(sehr): determine if there are
8-bit relocations.
One separate note on these fixups.  It seems that when we neglect to invoke
SetFixup on a fixup, it results in the fixup being emitted at offset 0 of the
function.  Could you fix up the fixups so that we get an assert or something
when this happens?

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4236: uint64_t Mult = Const->getValue();
Maybe this ought to be uint32_t given the dyn_cast<ConstantInteger32> above, and
the use of uint32_t in the new code below.

https://codereview.chromium.org/1428443002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4264: case 1:
Might as well as "case 0:" for symmetry with above.

[Karl, 2015-10-27 16:13:40]

kschimpf@google.com changed reviewers:
+ kschimpf@google.com
- kschimpf@chromium.org

[sehr, 2015-10-27 21:48:01]

Delving into unittests turned up some cleanups I thought should be done while I
was there.  PTAL.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
File src/IceTargetLoweringX8632Traits.h (right):

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX8632Traits.h:185: Address(GPRRegister base, int32_t disp,
AssemblerFixup *fixup = nullptr) {
On 2015/10/27 05:52:34, stichnot wrote:
> Maybe we can make fixup a required parameter, rather than default-value?

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX8632Traits.h:195: // TODO(sehr): determine if there are
8-bit relocations.
On 2015/10/27 13:57:10, stichnot wrote:
> Here and in the TODO below, if there is a fixup, I think you need to just use
a
> 32-bit displacement.  For example, here's a mashup of simple_loop.ll and
> globalrelocs.ll:
> 
> @bytes = internal global [8 x i8] c"abcdefgh"
> define internal i32 @simple_loop(i32 %a, i32 %n) {
> entry:
>   %cmp4 = icmp sgt i32 %n, 0
>   br i1 %cmp4, label %for.body, label %for.end
> for.body:
>   %i.06 = phi i32 [ %inc, %for.body ], [ 0, %entry ]
>   %sum.05 = phi i32 [ %add, %for.body ], [ 0, %entry ]
>   %gep_array = mul i32 %i.06, 4
>   %gep = add i32 %a, %gep_array
>   %baddr = ptrtoint [8 x i8] * @bytes to i32
>   %gep2 = add i32 %gep, %baddr
>   %gep3 = add i32 %gep2, 4
>   %__9 = inttoptr i32 %gep3 to i32*
>   %v0 = load i32, i32* %__9, align 1
>   %add = add i32 %v0, %sum.05
>   %inc = add i32 %i.06, 1
>   %cmp = icmp slt i32 %inc, %n
>   br i1 %cmp, label %for.body, label %for.end
> for.end:
>   %sum.0.lcssa = phi i32 [ 0, %entry ], [ %add, %for.body ]
>   ret i32 %sum.0.lcssa
> }
> 
> Now, change %gep3 to add 1024 instead of 4, and it works correctly.
> 
> More generally, the first two branches of this if/then/else chain should not
> fire when fixup!=nullptr, because disp is really not an 8-bit 0.

Agreed, and that fixed my other bug.  Thanks!

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX8632Traits.h:195: // TODO(sehr): determine if there are
8-bit relocations.
On 2015/10/27 13:57:10, stichnot wrote:
> One separate note on these fixups.  It seems that when we neglect to invoke
> SetFixup on a fixup, it results in the fixup being emitted at offset 0 of the
> function.  Could you fix up the fixups so that we get an assert or something
> when this happens?

Added flag/assertions in IceFixups.h

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:2854: // Mov Src0HiRM first, because it was
legalized most recently,
On 2015/10/27 05:52:34, stichnot wrote:
> Can you reformat this and other comments to 80-cols?

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4101: if (const InstArithmetic *Arith =
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4113: /*
On 2015/10/27 05:52:34, stichnot wrote:
> Why is this commented out?

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4148: if (Variable *SrcVar =
llvm::dyn_cast<Variable>(SrcOp)) {
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4156: } else if (auto Const =
llvm::dyn_cast<ConstantInteger32>(SrcOp)) {
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4158: if (Utils::WouldOverflowAdd(Offset,
MoreOffset))
On 2015/10/27 05:52:34, stichnot wrote:
> Is overflow an issue for x86-32?  Can it be disallowed only for x86-64?

X86-64 immediates are also 32 bits.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4164: } else if (auto AddReloc =
llvm::dyn_cast<ConstantRelocatable>(SrcOp)) {
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4225: if (const InstArithmetic *ArithInst =
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4227: if (Variable *Var =
llvm::dyn_cast<Variable>(ArithInst->getSrc(0))) {
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4228: if (ConstantInteger32 *Const =
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4236: uint64_t Mult = Const->getValue();
On 2015/10/27 13:57:10, stichnot wrote:
> Maybe this ought to be uint32_t given the dyn_cast<ConstantInteger32> above,
and
> the use of uint32_t in the new code below.

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4264: case 1:
On 2015/10/27 13:57:10, stichnot wrote:
> Might as well as "case 0:" for symmetry with above.

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4300: if (const InstArithmetic *ArithInst =
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4308: Variable *Var0 =
llvm::dyn_cast<Variable>(Src0);
On 2015/10/27 05:52:34, stichnot wrote:
> auto * for all of these

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4317: if ((Var0 != nullptr) && (Var1 !=
nullptr))
On 2015/10/27 05:52:34, stichnot wrote:
> In these cases, I think omitting the "!= nullptr" would be more readable.

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4342: if (Utils::WouldOverflowAdd(NewOffset,
MoreOffset))
On 2015/10/27 05:52:34, stichnot wrote:
> Same 32-bit overflow question as above.

Same answer, sadly.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4542: Variable *Base =
llvm::dyn_cast<Variable>(Addr);
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4751: Variable *Base =
llvm::dyn_cast<Variable>(Addr);
On 2015/10/27 05:52:34, stichnot wrote:
> auto *

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/20001/tests_lit/llvm2i...
File tests_lit/llvm2ice_tests/address-mode-global.ll (right):

https://chromiumcodereview.appspot.com/1428443002/diff/20001/tests_lit/llvm2i...
tests_lit/llvm2ice_tests/address-mode-global.ll:36: 
On 2015/10/27 05:52:34, stichnot wrote:
> git doesn't like "trailing whitespace".

Removed.

[Jim Stichnoth, 2015-10-27 22:19:52]

otherwise lgtm

https://codereview.chromium.org/1428443002/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1428443002/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4317: else if (!IsAdd && Var1)
Don't use "else" after "if (...) return;".
(here and below)

https://codereview.chromium.org/1428443002/diff/80001/unittest/AssemblerX8632...
File unittest/AssemblerX8632/ControlFlow.cpp (right):

https://codereview.chromium.org/1428443002/diff/80001/unittest/AssemblerX8632...
unittest/AssemblerX8632/ControlFlow.cpp:136: static constexpr AssemblerFixup
*Fixup = nullptr;
Is there some place you could put a single instance of:

  static constexpr AssemblerFixup *NoFixup = nullptr;

and then use NoFixup as the Address() arg?  Cutting down the repetition of the
NoFixup definitions.

I would be OK with a "#define NoFixup nullptr" if there isn't a common
superclass.

[sehr, 2015-10-27 23:24:05]

Thanks.  Running tests now.

https://chromiumcodereview.appspot.com/1428443002/diff/80001/src/IceTargetLow...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://chromiumcodereview.appspot.com/1428443002/diff/80001/src/IceTargetLow...
src/IceTargetLoweringX86BaseImpl.h:4317: else if (!IsAdd && Var1)
On 2015/10/27 22:19:52, stichnot wrote:
> Don't use "else" after "if (...) return;".
> (here and below)

Done.

https://chromiumcodereview.appspot.com/1428443002/diff/80001/unittest/Assembl...
File unittest/AssemblerX8632/ControlFlow.cpp (right):

https://chromiumcodereview.appspot.com/1428443002/diff/80001/unittest/Assembl...
unittest/AssemblerX8632/ControlFlow.cpp:136: static constexpr AssemblerFixup
*Fixup = nullptr;
On 2015/10/27 22:19:52, stichnot wrote:
> Is there some place you could put a single instance of:
> 
>   static constexpr AssemblerFixup *NoFixup = nullptr;
> 
> and then use NoFixup as the Address() arg?  Cutting down the repetition of the
> NoFixup definitions.
> 
> I would be OK with a "#define NoFixup nullptr" if there isn't a common
> superclass.

Added a NoFixup constexpr to the interface of AssemblerFixup.  Great suggestion.

[sehr, 2015-10-27 23:55:45]

Committed patchset #5 (id:80001) manually as
aa0b1a176f6c3306be3e95374d72e735bab2bbab (presubmit successful).