ARM: Add a postRA pass to legalize stack offsets. Greedy approach (reserve IP).

ARM: Add a postRA pass to legalize stack offsets. Greedy approach (reserve IP).

Make a post-register allocation and post-addProlog pass to
go through variables with stack offsets and legalize them
in case the offsets are not encodeable. The naive approach
is to reserve IP, and use IP to movw/movt the offset, then
add/sub the frame/stack pointer to IP and use IP as the new
base instead of the frame/stack pointer. We do some amount
of CSE within a basic block, and share the IP base pointer
when it is (a) within range for later stack references,
and (b) IP hasn't been clobbered (e.g., by a function call).
I chose to do this greedy approach for both Om1 and O2,
since it should just be a linear pass, and it reduces the
amount of variables/instructions created compared to the
super-naive peephole approach (so might be faster?).

Introduce a test-only flag and use that to artificially
bloat the stack frame so that spill offsets are out
of range for ARM. Use that flag for cross tests to
stress this new code a bit more (than would have been
stressed by simply doing a lit test + FileCheck).

Also, the previous version of emitVariable() was using the
Var's type to determine the range (only +/- 255 for i16,
vs +/- 4095 for i32), even though mov's emit() always
uses a full 32-bit "ldr" instead of a 16-bit "ldrh".
Use a common legality check, which uses the stackSlotType
instead of the Var's type. This previously caused the
test_bitmanip to spuriously complain, even though the
offsets for Om1 were "only" in the 300 byte range. With this
fixed, we can then enable the test_bitmanip test too.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
R=stichnot@chromium.org

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

[jvoung (off chromium), 2015-07-23 23:23:30]

jvoung@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@google.com, stichnot@chromium.org

[jvoung (off chromium), 2015-07-23 23:26:32]

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2242: _mov(T, Zero);
misc legalization fix for test_bitmanip, now that it's testable...

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:759: int32_t BaseRegNum =
Var->getBaseRegNum();
Well.. this is an extra virtual call, but I didn't have a good idea for
overriding if it happened to be something like StackVariable (though that's only
applicable for ARM). I didn't want to override "emit()" and "dump()" and do
something like override stackVarToAsmOperand(), since it's *almost* the same.

[Jim Stichnoth, 2015-07-24 22:08:30]

https://codereview.chromium.org/1241763002/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:272: int32_t BaseRegNum;
int32_t BaseRegNum = Variable::NoRegister;

Then you can drop it from the initialization list.

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering....
src/IceTargetLowering.cpp:390: if (TestPadding && UsesFramePointer)
Maybe just remove "TestPadding && " ?

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering....
src/IceTargetLowering.h:185: virtual SizeT getFrameOrStackReg() const = 0;
What do you think about changing this to something like
  SizeT getFrameReg(const Variable *Var = nullptr) const;
such that if you pass a non-null Var, it first checks Var->getBaseRegNum() and
then defaults to esp/ebp/whatever?  I think it might make the uses of
getBaseRegNum() simpler.

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:759: int32_t BaseRegNum =
Var->getBaseRegNum();
On 2015/07/23 23:26:32, jvoung wrote:
> Well.. this is an extra virtual call, but I didn't have a good idea for
> overriding if it happened to be something like StackVariable (though that's
only
> applicable for ARM). I didn't want to override "emit()" and "dump()" and do
> something like override stackVarToAsmOperand(), since it's *almost* the same.

No need to worry about performance in dump() or emit() functions.

BTW, this being an emit() function, it should start with:
  if (!BuildDefs::dump())
    return;

[jvoung (off chromium), 2015-07-27 17:02:58]

https://codereview.chromium.org/1241763002/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:272: int32_t BaseRegNum;
On 2015/07/24 22:08:29, stichnot wrote:
> int32_t BaseRegNum = Variable::NoRegister;
> 
> Then you can drop it from the initialization list.

Done.

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering....
src/IceTargetLowering.cpp:390: if (TestPadding && UsesFramePointer)
On 2015/07/24 22:08:29, stichnot wrote:
> Maybe just remove "TestPadding && " ?

Done.

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering....
src/IceTargetLowering.h:185: virtual SizeT getFrameOrStackReg() const = 0;
On 2015/07/24 22:08:29, stichnot wrote:
> What do you think about changing this to something like
>   SizeT getFrameReg(const Variable *Var = nullptr) const;
> such that if you pass a non-null Var, it first checks Var->getBaseRegNum() and
> then defaults to esp/ebp/whatever?  I think it might make the uses of
> getBaseRegNum() simpler.

I'm not totally sure that would simplify things.


In some cases, we want two pieces of information back: getBaseRegNum() affects
the Offset as well as the register number. E.g., the ARM pass is taking into
account getStackAdjustment() to see if that also overflows the offset, and then
makes that part of new variable's offset, so we don't want to count that stack
adjustment twice.

  int32_t Offset = Var->getStackOffset();
  int32_t BaseRegNum = Var->getBaseRegNum();
  if (Var->getBaseRegNum() == Variable::NoRegister) {
     BaseRegNum = getFrameOrStackReg();
     if (!hasFramePointer())
       Offset += getStackAdjustment();
     }
  }

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:759: int32_t BaseRegNum =
Var->getBaseRegNum();
On 2015/07/24 22:08:29, stichnot wrote:
> On 2015/07/23 23:26:32, jvoung wrote:
> > Well.. this is an extra virtual call, but I didn't have a good idea for
> > overriding if it happened to be something like StackVariable (though that's
> only
> > applicable for ARM). I didn't want to override "emit()" and "dump()" and do
> > something like override stackVarToAsmOperand(), since it's *almost* the
same.
> 
> No need to worry about performance in dump() or emit() functions.
> 

Okay stackVarToAsmOperand also does this though, and it's used by emitIAS.


> BTW, this being an emit() function, it should start with:
>   if (!BuildDefs::dump())
>     return;

Added here, and in arm and mips.

[Jim Stichnoth, 2015-07-30 13:34:52]

LGTM (and sorry for the delay).

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1241763002/diff/120001/src/IceTargetLowering....
src/IceTargetLowering.h:185: virtual SizeT getFrameOrStackReg() const = 0;
On 2015/07/27 17:02:58, jvoung wrote:
> On 2015/07/24 22:08:29, stichnot wrote:
> > What do you think about changing this to something like
> >   SizeT getFrameReg(const Variable *Var = nullptr) const;
> > such that if you pass a non-null Var, it first checks Var->getBaseRegNum()
and
> > then defaults to esp/ebp/whatever?  I think it might make the uses of
> > getBaseRegNum() simpler.
> 
> I'm not totally sure that would simplify things.
> 
> 
> In some cases, we want two pieces of information back: getBaseRegNum() affects
> the Offset as well as the register number. E.g., the ARM pass is taking into
> account getStackAdjustment() to see if that also overflows the offset, and
then
> makes that part of new variable's offset, so we don't want to count that stack
> adjustment twice.
> 
>   int32_t Offset = Var->getStackOffset();
>   int32_t BaseRegNum = Var->getBaseRegNum();
>   if (Var->getBaseRegNum() == Variable::NoRegister) {
>      BaseRegNum = getFrameOrStackReg();
>      if (!hasFramePointer())
>        Offset += getStackAdjustment();
>      }
>   }

I see, OK.

[jvoung (off chromium), 2015-07-31 19:58:50]

Committed patchset #8 (id:140001) manually as
28068adbf34a4602090efddc18b4dd123ffdeb6a (presubmit successful).