Merge fixed alloca stack adjustments into the prolog

Merge fixed alloca stack adjustments into the prolog

Also removes reliance on lowerAlloca entirely for the fixed allocations.

BUG=
R=jpp@chromium.org, stichnot@chromium.org

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

[sehr, 2015-11-12 22:08:39]

Description was changed from

==========
Merge fixed alloca stack adjustments into the prolog

Also removes reliance on lowerAlloca entirely for the fixed allocations.

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

to

==========
Merge fixed alloca stack adjustments into the prolog

Also removes reliance on lowerAlloca entirely for the fixed allocations.

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

[sehr, 2015-11-12 22:08:40]

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

[sehr, 2015-11-12 22:09:00]

Fixed allocas are now a part of the prolog.  PTAL.

[sehr, 2015-11-12 23:30:59]

REVIEWERS: Hold off on the review for now.  In my fine tradition of strategic
overreach, I've decided to try to fuse call argument allocation into this also.

[sehr, 2015-11-13 06:01:35]

On 2015/11/12 23:30:59, sehr (please use this account) wrote:
> REVIEWERS: Hold off on the review for now.  In my fine tradition of strategic
> overreach, I've decided to try to fuse call argument allocation into this
also.

OK, I've decided the extension was too much for one CL.  PLEASE CONTINUE
REVIEWING THIS CL.

[sehr, 2015-11-16 06:18:04]

On 2015/11/13 06:01:35, sehr (please use this account) wrote:
> On 2015/11/12 23:30:59, sehr (please use this account) wrote:
> > REVIEWERS: Hold off on the review for now.  In my fine tradition of
strategic
> > overreach, I've decided to try to fuse call argument allocation into this
> also.
> 
> OK, I've decided the extension was too much for one CL.  PLEASE CONTINUE
> REVIEWING THIS CL.

(FOR MONDAY MORNING EARLY RISERS, NOT WEEKEND: PING!)

[John, 2015-11-16 14:00:02]

I'll let the official LGTM to Jim.

LGTM

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp#newcode624
src/IceCfg.cpp:624: AllocaBaseVariableType BasePointerType =
const AllocaBaseVariableType

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:103: const Ice::TargetLowering *Target) {
const Ice::TargetX8632 *? I would hope llvm would devirtualize the call to
getStackReg and getFrameReg if you passed a concrete, final class.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:366: // Rematerializable uses of esp/ebp do not count
as uses for spilling.
I would rather say

... uses of stack/frame pointer do not ...

for this is an abstract, base class, and esp/ebp are target specific.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:91: return UsesFramePointer ? getFrameReg() :
getStackReg();
nice. :)

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:523:
Ctx->getConstantInt32(-FixedAllocaAlignBytes));
~FixedAllocaAlignBytes?

[Jim Stichnoth, 2015-11-16 14:47:58]

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp#newcode502
src/IceCfg.cpp:502: for (size_t i = 0; i < Allocas.size(); ++i) {
For better or worse, most of the Subzero code that iterates directly over vector
indices uses SizeT.

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp#newcode514
src/IceCfg.cpp:514: InstAlloca::create(this, AllocaSize, CombinedAlignment,
BaseVariable);
Wow, I just realized that InstAlloca is unique among Inst subclasses, in that
its Dest argument is at the end of the ctor and create() arg lists, instead of
in the first position after the Cfg* arg.  Do you think it's reasonable to fix
that in this CL?

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:106: if (static_cast<SizeT>(Var->getRegNum()) ==
Target->getStackReg()) {
Pull "static_cast<SizeT>(Var->getRegNum())" out of the if/then/else construct
for clarity?

SizeT RegNum = static_cast<SizeT>(Var->getRegNum());
if (RegNum == Target->getStackReg()) {

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:112: llvm_unreachable("Unexpected rematerializable register
type");
llvm::report_fatal_error

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:116: }  // end namespace
end of anonymous namespace

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:121: Ice::TargetLowering *Target = Func->getTarget();
Retain the const?

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:366: // Rematerializable uses of esp/ebp do not count
as uses for spilling.
I don't understand what this change does.

The purpose of the Var->hasReg() check is to record the complete set of physical
registers used in the function, so that the prolog/epilog generation code can
filter them against the set of callee-save registers to get the set of registers
to save/restore.  It doesn't seem like this conditional should have any effect
on the outcome.  (But I haven't tried it, so I could be wrong...)

Also, it's better to say something like "stack/frame pointer" instead of
"esp/ebp" in this target-neutral file.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:367: if( !Var->isRematerializable())
Please run clang-format...

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:509: // Combine fixed allocations into
SpillAreaSizeBytes if we are emitting
reflow to 80-cols

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:528: // not combined with prolog.
maybe it would be better something like, "Account for known-frame-offset alloca
instructions that were not already combined into the prolog."

Also, reflow to 80-cols

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:110: /// Returns the (negative) offset from ebp
where the fixed Allocas start.
Should probably start getting used to calling it ebp/rbp (even if the code still
refers to possibly the wrong register).

[sehr, 2015-11-16 18:42:25]

Thanks for the reviews.  PTAL.

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp#newcode502
src/IceCfg.cpp:502: for (size_t i = 0; i < Allocas.size(); ++i) {
On 2015/11/16 14:47:57, stichnot wrote:
> For better or worse, most of the Subzero code that iterates directly over
vector
> indices uses SizeT.

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp#newcode514
src/IceCfg.cpp:514: InstAlloca::create(this, AllocaSize, CombinedAlignment,
BaseVariable);
On 2015/11/16 14:47:57, stichnot wrote:
> Wow, I just realized that InstAlloca is unique among Inst subclasses, in that
> its Dest argument is at the end of the ctor and create() arg lists, instead of
> in the first position after the Cfg* arg.  Do you think it's reasonable to fix
> that in this CL?

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceCfg.cpp#newcode624
src/IceCfg.cpp:624: AllocaBaseVariableType BasePointerType =
On 2015/11/16 14:00:02, John wrote:
> const AllocaBaseVariableType

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:103: const Ice::TargetLowering *Target) {
On 2015/11/16 14:00:02, John wrote:
> const Ice::TargetX8632 *? I would hope llvm would devirtualize the call to
> getStackReg and getFrameReg if you passed a concrete, final class.

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:106: if (static_cast<SizeT>(Var->getRegNum()) ==
Target->getStackReg()) {
On 2015/11/16 14:47:57, stichnot wrote:
> Pull "static_cast<SizeT>(Var->getRegNum())" out of the if/then/else construct
> for clarity?
> 
> SizeT RegNum = static_cast<SizeT>(Var->getRegNum());
> if (RegNum == Target->getStackReg()) {

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:112: llvm_unreachable("Unexpected rematerializable register
type");
On 2015/11/16 14:47:57, stichnot wrote:
> llvm::report_fatal_error

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:116: }  // end namespace
On 2015/11/16 14:47:57, stichnot wrote:
> end of anonymous namespace

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:121: Ice::TargetLowering *Target = Func->getTarget();
On 2015/11/16 14:47:57, stichnot wrote:
> Retain the const?

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:366: // Rematerializable uses of esp/ebp do not count
as uses for spilling.
On 2015/11/16 14:00:02, John wrote:
> I would rather say
> 
> ... uses of stack/frame pointer do not ...
> 
> for this is an abstract, base class, and esp/ebp are target specific.

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:366: // Rematerializable uses of esp/ebp do not count
as uses for spilling.
On 2015/11/16 14:47:57, stichnot wrote:
> I don't understand what this change does.
> 
> The purpose of the Var->hasReg() check is to record the complete set of
physical
> registers used in the function, so that the prolog/epilog generation code can
> filter them against the set of callee-save registers to get the set of
registers
> to save/restore.  It doesn't seem like this conditional should have any effect
> on the outcome.  (But I haven't tried it, so I could be wrong...)
> 
> Also, it's better to say something like "stack/frame pointer" instead of
> "esp/ebp" in this target-neutral file.

Well, it doesn't work to remove it, as I discovered.  The issue is as follows. 
This code keeps track of references to variables before (I think) register
allocating anything.  It's looking for physical register references to ebp and
noting that those would cause ebp to need to be spilled and restored.  With
rematerialization I create uses of ebp that are to the binding established
within the call frame, not outside it, so I don't want the spilled version. 
Make sense?

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:367: if( !Var->isRematerializable())
On 2015/11/16 14:47:57, stichnot wrote:
> Please run clang-format...

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:91: return UsesFramePointer ? getFrameReg() :
getStackReg();
On 2015/11/16 14:00:02, John wrote:
> nice. :)

I thought so -- one place for constants is a good idea.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:509: // Combine fixed allocations into
SpillAreaSizeBytes if we are emitting
On 2015/11/16 14:47:57, stichnot wrote:
> reflow to 80-cols

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:523:
Ctx->getConstantInt32(-FixedAllocaAlignBytes));
On 2015/11/16 14:00:02, John wrote:
> ~FixedAllocaAlignBytes?

FixedAllocaAlignBytes is a power of 2, so what I want is ~(FixedAllocaAlignBytes
- 1), which if I remember my two's complement arithmetic is the same as
-FixedAllocaAlignBytes.  Am I wrong?

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:528: // not combined with prolog.
On 2015/11/16 14:47:57, stichnot wrote:
> maybe it would be better something like, "Account for known-frame-offset
alloca
> instructions that were not already combined into the prolog."
> 
> Also, reflow to 80-cols

Done.

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:110: /// Returns the (negative) offset from ebp
where the fixed Allocas start.
On 2015/11/16 14:47:58, stichnot wrote:
> Should probably start getting used to calling it ebp/rbp (even if the code
still
> refers to possibly the wrong register).

Done.

[Jim Stichnoth, 2015-11-16 22:47:18]

lgtm

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:366: // Rematerializable uses of esp/ebp do not count
as uses for spilling.
On 2015/11/16 18:42:24, sehr (please use this account) wrote:
> On 2015/11/16 14:47:57, stichnot wrote:
> > I don't understand what this change does.
> > 
> > The purpose of the Var->hasReg() check is to record the complete set of
> physical
> > registers used in the function, so that the prolog/epilog generation code
can
> > filter them against the set of callee-save registers to get the set of
> registers
> > to save/restore.  It doesn't seem like this conditional should have any
effect
> > on the outcome.  (But I haven't tried it, so I could be wrong...)
> > 
> > Also, it's better to say something like "stack/frame pointer" instead of
> > "esp/ebp" in this target-neutral file.
> 
> Well, it doesn't work to remove it, as I discovered.  The issue is as follows.

> This code keeps track of references to variables before (I think) register
> allocating anything.  It's looking for physical register references to ebp and
> noting that those would cause ebp to need to be spilled and restored.  With
> rematerialization I create uses of ebp that are to the binding established
> within the call frame, not outside it, so I don't want the spilled version. 
> Make sense?

I spent some quality time with the code to understand what breaks when the
condition is removed.  In the end I agree that this is the right thing to do,
but I would change the comment to something like:

Don't consider a rematerializable variable to be an actual register use
(specifically of the frame pointer).  Otherwise, the prolog may decide to save
the frame pointer twice - once because of the explicit need for a frame pointer,
and once because of an active use of a callee-save register.

https://codereview.chromium.org/1435363002/diff/100001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1435363002/diff/100001/src/IceTargetLowering....
src/IceTargetLowering.cpp:366: // Rematerializable uses of stack/frame do not
count as uses for spilling.
stack/frame pointer

[sehr, 2015-11-17 00:51:45]

Committed patchset #7 (id:120001) manually as
2f3b8ec812321b1549bc1714a239b9dd0c95ebd7 (tree was closed).

[sehr, 2015-11-17 01:00:00]

On 2015/11/16 22:47:18, stichnot wrote:
> lgtm
> 
>
https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.cpp
> File src/IceTargetLowering.cpp (right):
> 
>
https://codereview.chromium.org/1435363002/diff/60001/src/IceTargetLowering.c...
> src/IceTargetLowering.cpp:366: // Rematerializable uses of esp/ebp do not
count
> as uses for spilling.
> On 2015/11/16 18:42:24, sehr (please use this account) wrote:
> > On 2015/11/16 14:47:57, stichnot wrote:
> > > I don't understand what this change does.
> > > 
> > > The purpose of the Var->hasReg() check is to record the complete set of
> > physical
> > > registers used in the function, so that the prolog/epilog generation code
> can
> > > filter them against the set of callee-save registers to get the set of
> > registers
> > > to save/restore.  It doesn't seem like this conditional should have any
> effect
> > > on the outcome.  (But I haven't tried it, so I could be wrong...)
> > > 
> > > Also, it's better to say something like "stack/frame pointer" instead of
> > > "esp/ebp" in this target-neutral file.
> > 
> > Well, it doesn't work to remove it, as I discovered.  The issue is as
follows.
> 
> > This code keeps track of references to variables before (I think) register
> > allocating anything.  It's looking for physical register references to ebp
and
> > noting that those would cause ebp to need to be spilled and restored.  With
> > rematerialization I create uses of ebp that are to the binding established
> > within the call frame, not outside it, so I don't want the spilled version. 
> > Make sense?
> 
> I spent some quality time with the code to understand what breaks when the
> condition is removed.  In the end I agree that this is the right thing to do,
> but I would change the comment to something like:
> 
> Don't consider a rematerializable variable to be an actual register use
> (specifically of the frame pointer).  Otherwise, the prolog may decide to save
> the frame pointer twice - once because of the explicit need for a frame
pointer,
> and once because of an active use of a callee-save register.
> 
>
https://codereview.chromium.org/1435363002/diff/100001/src/IceTargetLowering.cpp
> File src/IceTargetLowering.cpp (right):
> 
>
https://codereview.chromium.org/1435363002/diff/100001/src/IceTargetLowering....
> src/IceTargetLowering.cpp:366: // Rematerializable uses of stack/frame do not
> count as uses for spilling.
> stack/frame pointer

Done.  Thanks.