[SubZero] Legalize load, store for MIPS post lower

[SubZero] Legalize load, store for MIPS post lower

This patch legalizes load, store instructions post lowering.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-10-12 12:47:18]

Description was changed from

==========
[SubZero] Legalize load, store for MIPS post lower

This patch legalizes load, store instructions post lowering.
==========

to

==========
[SubZero] Legalize load, store for MIPS post lower

This patch legalizes load, store instructions post lowering.
==========

[jaydeep.patil, 2016-10-12 12:47:18]

jaydeep.patil@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
makdstefan@gmail.com, sagar.thakur@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-10-12 13:52:52]

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1534: uint32_t UpperBits = (Offset >> 16) &
0xFFFF;
const uint32_t for these?

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1745:
TargetMIPS32::PostLoweringLegalizer::legalizeMemOperand(OperandMIPS32Mem *Mem) {
It would be nice (though I won't insist) if this CL included some lit tests,
e.g. a new mips_legalization.ll file, such that every test in that file would
fail against the current master branch.

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1750: ConstantInteger32 *Ci32 =
llvm::cast<ConstantInteger32>(Mem->getOffset());
auto *Ci32

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1782: if (auto *MovInstr =
llvm::dyn_cast<InstMIPS32Mov>(CurInstr)) {
This loop body has become kind of hard to read.  I think it can be improved:

1. Use "continue" more aggressively, i.e.
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-sim...

2. Factor out the llvm::cast<> operations.

Here is a suggestion, though I can't know whether lit tests are exercising all
of it:

while (!Context.atEnd()) {
  PostIncrLoweringContext PostIncrement(Context);
  Inst *CurInstr = iteratorToInst(Context.getCur());
  const SizeT NumSrcs = CurInstr->getSrcSize();
  Operand *Src0 = NumSrcs < 1 ? nullptr : CurInstr->getSrc(0);
  Operand *Src1 = NumSrcs < 2 ? nullptr : CurInstr->getSrc(1);
  auto *Src0V = llvm::dyn_cast_or_null<Variable>(Src0);
  auto *Src0M = llvm::dyn_cast_or_null<OperandMIPS32Mem>(Src0);
  auto *Src1M = llvm::dyn_cast_or_null<OperandMIPS32Mem>(Src1);
  Variable *Dst = CurInstr->getDest();
  if (auto *MovInstr = llvm::dyn_cast<InstMIPS32Mov>(CurInstr)) {
    Legalizer.legalizeMov(MovInstr);
    continue;
  }
  if (llvm::isa<InstMIPS32Sw>(CurInstr)) {
    if (auto *LegalMem = Legalizer.legalizeMemOperand(Src1M)) {
      _sw(Src0V, LegalMem);
      CurInstr->setDeleted();
    }
    continue;
  }
  if (llvm::isa<InstMIPS32Swc1>(CurInstr)) {
    if (auto *LegalMem = Legalizer.legalizeMemOperand(Src1M)) {
      _swc1(Src0V, LegalMem);
      CurInstr->setDeleted();
    }
    continue;
  }
  if (llvm::isa<InstMIPS32Sdc1>(CurInstr)) {
    if (auto *LegalMem = Legalizer.legalizeMemOperand(Src1M)) {
      _sdc1(Src0V, LegalMem);
      CurInstr->setDeleted();
    }
    continue;
  }
  if (llvm::isa<InstMIPS32Lw>(CurInstr)) {
    if (auto *LegalMem = Legalizer.legalizeMemOperand(Src0M)) {
      _lw(Dst, LegalMem);
      CurInstr->setDeleted();
    }
    continue;
  }
  if (llvm::isa<InstMIPS32Lwc1>(CurInstr)) {
    if (auto *LegalMem = Legalizer.legalizeMemOperand(Src0M)) {
      _lwc1(Dst, LegalMem);
      CurInstr->setDeleted();
    }
    continue;
  }
  if (llvm::isa<InstMIPS32Ldc1>(CurInstr)) {
    if (auto *LegalMem = Legalizer.legalizeMemOperand(Src0M)) {
      _ldc1(Dst, LegalMem);
      CurInstr->setDeleted();
    }
    continue;
  }
}

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:730: /// Legalizes Mem. if Mem.Base is a
reamaterializable variable,
rematerializable

[jaydeep.patil, 2016-10-13 04:36:35]

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1534: uint32_t UpperBits = (Offset >> 16) &
0xFFFF;
On 2016/10/12 13:52:52, Jim Stichnoth wrote:
> const uint32_t for these?

Done.

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1745:
TargetMIPS32::PostLoweringLegalizer::legalizeMemOperand(OperandMIPS32Mem *Mem) {
On 2016/10/12 13:52:52, Jim Stichnoth wrote:
> It would be nice (though I won't insist) if this CL included some lit tests,
> e.g. a new mips_legalization.ll file, such that every test in that file would
> fail against the current master branch.

Done.

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1750: ConstantInteger32 *Ci32 =
llvm::cast<ConstantInteger32>(Mem->getOffset());
On 2016/10/12 13:52:52, Jim Stichnoth wrote:
> auto *Ci32

Done.

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1782: if (auto *MovInstr =
llvm::dyn_cast<InstMIPS32Mov>(CurInstr)) {
On 2016/10/12 13:52:52, Jim Stichnoth wrote:
> This loop body has become kind of hard to read.  I think it can be improved:
> 
> 1. Use "continue" more aggressively, i.e.
>
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-sim...
> 
> 2. Factor out the llvm::cast<> operations.
> 
> Here is a suggestion, though I can't know whether lit tests are exercising all
> of it:
> 
> while (!Context.atEnd()) {
>   PostIncrLoweringContext PostIncrement(Context);
>   Inst *CurInstr = iteratorToInst(Context.getCur());
>   const SizeT NumSrcs = CurInstr->getSrcSize();
>   Operand *Src0 = NumSrcs < 1 ? nullptr : CurInstr->getSrc(0);
>   Operand *Src1 = NumSrcs < 2 ? nullptr : CurInstr->getSrc(1);
>   auto *Src0V = llvm::dyn_cast_or_null<Variable>(Src0);
>   auto *Src0M = llvm::dyn_cast_or_null<OperandMIPS32Mem>(Src0);
>   auto *Src1M = llvm::dyn_cast_or_null<OperandMIPS32Mem>(Src1);
>   Variable *Dst = CurInstr->getDest();
>   if (auto *MovInstr = llvm::dyn_cast<InstMIPS32Mov>(CurInstr)) {
>     Legalizer.legalizeMov(MovInstr);
>     continue;
>   }
>   if (llvm::isa<InstMIPS32Sw>(CurInstr)) {
>     if (auto *LegalMem = Legalizer.legalizeMemOperand(Src1M)) {
>       _sw(Src0V, LegalMem);
>       CurInstr->setDeleted();
>     }
>     continue;
>   }
>   if (llvm::isa<InstMIPS32Swc1>(CurInstr)) {
>     if (auto *LegalMem = Legalizer.legalizeMemOperand(Src1M)) {
>       _swc1(Src0V, LegalMem);
>       CurInstr->setDeleted();
>     }
>     continue;
>   }
>   if (llvm::isa<InstMIPS32Sdc1>(CurInstr)) {
>     if (auto *LegalMem = Legalizer.legalizeMemOperand(Src1M)) {
>       _sdc1(Src0V, LegalMem);
>       CurInstr->setDeleted();
>     }
>     continue;
>   }
>   if (llvm::isa<InstMIPS32Lw>(CurInstr)) {
>     if (auto *LegalMem = Legalizer.legalizeMemOperand(Src0M)) {
>       _lw(Dst, LegalMem);
>       CurInstr->setDeleted();
>     }
>     continue;
>   }
>   if (llvm::isa<InstMIPS32Lwc1>(CurInstr)) {
>     if (auto *LegalMem = Legalizer.legalizeMemOperand(Src0M)) {
>       _lwc1(Dst, LegalMem);
>       CurInstr->setDeleted();
>     }
>     continue;
>   }
>   if (llvm::isa<InstMIPS32Ldc1>(CurInstr)) {
>     if (auto *LegalMem = Legalizer.legalizeMemOperand(Src0M)) {
>       _ldc1(Dst, LegalMem);
>       CurInstr->setDeleted();
>     }
>     continue;
>   }
> }

Done.

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/2411193003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:730: /// Legalizes Mem. if Mem.Base is a
reamaterializable variable,
On 2016/10/12 13:52:52, Jim Stichnoth wrote:
> rematerializable

Done.

[Jim Stichnoth, 2016-10-13 17:15:30]

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/mips-legalization.ll (right):

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/mips-legalization.ll:3: ; RUN: %if --need=target_MIPS32
--need=allow_dump \
Better to remove both instances of --need=allow_dump and add a line near the
top:

; REQUIRES: allow_dump

The --need=allow_dump is if you have a mix of filetype=asm and filetype=obj
tests, e.g. for different targets, but that should never be the case in this
test file.

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/mips-legalization.ll:5: ; RUN:   --target mips32 -i %s
--args -O2 --skip-unimplemented \
If possible, remove --skip-unimplemented and -allow-externally-defined-symbol.

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/mips-legalization.ll:39: %add1 = add nsw i32 %0, %1
"nsw" is not part of the PNaCl bitcode ABI, so that kind of stands out here. 
(The %p2i script probably strips it, and/or the Subzero bitcode parser ignores
it, but still.)

There is an undocumented pydir/build-pnacl-ir.py script that may be helpful
here, if you're starting from a .c file.

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/mips-legalization.ll:53: ; MIPS32: addiu sp,sp,-48
I would be a bit cautious about assuming specific stack frame sizes or stack
offsets.  It can cause extra hardship if someone modifies/improves stack layout,
register allocation, alloca address rematerialization, etc.

But, it might be less of a pain to fix that when it happens, compared to making
the test fully general today, so it's your call...

[jaydeep.patil, 2016-10-14 04:40:43]

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/mips-legalization.ll (right):

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/mips-legalization.ll:3: ; RUN: %if --need=target_MIPS32
--need=allow_dump \
On 2016/10/13 17:15:29, Jim Stichnoth wrote:
> Better to remove both instances of --need=allow_dump and add a line near the
> top:
> 
> ; REQUIRES: allow_dump
> 
> The --need=allow_dump is if you have a mix of filetype=asm and filetype=obj
> tests, e.g. for different targets, but that should never be the case in this
> test file.

Done.

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/mips-legalization.ll:5: ; RUN:   --target mips32 -i %s
--args -O2 --skip-unimplemented \
On 2016/10/13 17:15:30, Jim Stichnoth wrote:
> If possible, remove --skip-unimplemented and -allow-externally-defined-symbol.

Done.
We are also planning to remove --skip-unimplemented from all lit-tests for MIPS.

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/mips-legalization.ll:39: %add1 = add nsw i32 %0, %1
On 2016/10/13 17:15:29, Jim Stichnoth wrote:
> "nsw" is not part of the PNaCl bitcode ABI, so that kind of stands out here. 
> (The %p2i script probably strips it, and/or the Subzero bitcode parser ignores
> it, but still.)
> 
> There is an undocumented pydir/build-pnacl-ir.py script that may be helpful
> here, if you're starting from a .c file.

Done.
Used pydir/build-pnacl-ir.py script to generate the .ll file.

https://codereview.chromium.org/2411193003/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/mips-legalization.ll:53: ; MIPS32: addiu sp,sp,-48
On 2016/10/13 17:15:29, Jim Stichnoth wrote:
> I would be a bit cautious about assuming specific stack frame sizes or stack
> offsets.  It can cause extra hardship if someone modifies/improves stack
layout,
> register allocation, alloca address rematerialization, etc.
> 
> But, it might be less of a pain to fix that when it happens, compared to
making
> the test fully general today, so it's your call...

Acknowledged.
The pre-colored registers (a0-a3 and v0) are for arguments and return value.
Change in register allocation would not change these registers for MIPS32.
Temporaries are checked using macros. However, we need to get back to this case
whenever we change/optimize the stack layout.

[Jim Stichnoth, 2016-10-16 14:12:01]

lgtm

[Jim Stichnoth, 2016-10-16 14:13:41]

Description was changed from

==========
[SubZero] Legalize load, store for MIPS post lower

This patch legalizes load, store instructions post lowering.
==========

to

==========
[SubZero] Legalize load, store for MIPS post lower

This patch legalizes load, store instructions post lowering.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-10-16 14:13:43]

Committed patchset #3 (id:40001) manually as
ec92917338c13f4791c441deb4e447fd9ef60228 (presubmit successful).