Subzero. ARM32. Removing memory legalization warts.

Subzero. ARM32. Removes memory legalization warts.

This CL removes two warts from the ARM32 backend:
1) during argument lowering, if a stack parameter is assigned a
register, the backend creates a new Variable that references the stack
location with the incoming argument, and _mov() it to the parameter.

2) During stack slot legalization, all _mov(Mem(), Reg) are converted to
stores; and all _mov(Reg, Mem()) are converted to loads.

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

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

[John, 2015-11-18 17:49:36]

Description was changed from

==========
Subzero. ARM32. Removing memory legalization warts.

This CL removes two warts from the ARM32 backend:
1) during argument lowering, if a stack parameter is assigned a
register, the backend creates a new Variable that references the stack
location with the incoming argument, and _mov() it to the parameter.

2) During stack slot legalization, all _mov(Mem(), Reg) are converted to
stores; and all _mov(Reg, Mem()) are converted to loads.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Removed memory legalization warts.

This CL removes two warts from the ARM32 backend:
1) during argument lowering, if a stack parameter is assigned a
register, the backend creates a new Variable that references the stack
location with the incoming argument, and _mov() it to the parameter.

2) During stack slot legalization, all _mov(Mem(), Reg) are converted to
stores; and all _mov(Reg, Mem()) are converted to loads.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

[John, 2015-11-18 17:49:36]

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

[John, 2015-11-18 17:49:53]

Description was changed from

==========
Subzero. ARM32. Removed memory legalization warts.

This CL removes two warts from the ARM32 backend:
1) during argument lowering, if a stack parameter is assigned a
register, the backend creates a new Variable that references the stack
location with the incoming argument, and _mov() it to the parameter.

2) During stack slot legalization, all _mov(Mem(), Reg) are converted to
stores; and all _mov(Reg, Mem()) are converted to loads.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Removes memory legalization warts.

This CL removes two warts from the ARM32 backend:
1) during argument lowering, if a stack parameter is assigned a
register, the backend creates a new Variable that references the stack
location with the incoming argument, and _mov() it to the parameter.

2) During stack slot legalization, all _mov(Mem(), Reg) are converted to
stores; and all _mov(Reg, Mem()) are converted to loads.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

[Karl, 2015-11-18 18:40:42]

Otherwise, LGTM.

https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:478: /*
Remove comment?

https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1029: auto *SrcR = llvm::cast<Variable>(Src);
const auto?

https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:4644: assert(false);
Why have code below this? Doesn't assert always fail (except for release
compile, which assumes this won't happen)?

[John, 2015-11-19 13:26:11]

Patchset #4 (id:60001) has been deleted

[John, 2015-11-19 13:41:22]

https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:478: /*
On 2015/11/18 18:40:42, Karl wrote:
> Remove comment?

Oops, done.

https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1029: auto *SrcR = llvm::cast<Variable>(Src);
On 2015/11/18 18:40:42, Karl wrote:
> const auto?

done.

https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:4644: assert(false);
On 2015/11/18 18:40:42, Karl wrote:
> Why have code below this? Doesn't assert always fail (except for release
> compile, which assumes this won't happen)?

It used to be the case that TargetARM32::legalizeMov() would take care of these.
This assert was just a "heads up" to me, during development.

These were introduced by TargetARM32::finishArgumentLowering(). I hae changed
the implementation so that these are now emitted as _mov(Var, Mem()), which is
later converted to _ldr(Var, Mem()) during the mov legalization.

[John, 2015-11-19 13:43:03]

Committed patchset #4 (id:80001) manually as
3f6b47d53174f4f44d8b7c32d806fc5b5288a218 (presubmit successful).

[Jim Stichnoth, 2015-11-22 03:40:47]

necro lgtm

https://codereview.chromium.org/1457683004/diff/80001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1457683004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1024: auto *const SrcR =
llvm::cast<Variable>(Src);
"auto *const" seems odd -- did you mean "const auto *"?

https://codereview.chromium.org/1457683004/diff/80001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/branch-mult-fwd.ll (right):

https://codereview.chromium.org/1457683004/diff/80001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/branch-mult-fwd.ll:27: ; ASM-NEXT:     # [sp, #8] =
def.pseudo
This makes me wonder - should we really be emitting such pseudo-instructions in
a comment?

[John, 2015-11-23 18:52:56]

Done in another cl.

https://codereview.chromium.org/1457683004/diff/80001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1457683004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1024: auto *const SrcR =
llvm::cast<Variable>(Src);
On 2015/11/22 03:40:47, stichnot wrote:
> "auto *const" seems odd -- did you mean "const auto *"?

No, I meant auto *const. Removing as git grep 'auto *const' reveals no other
usages.

https://codereview.chromium.org/1457683004/diff/80001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/branch-mult-fwd.ll (right):

https://codereview.chromium.org/1457683004/diff/80001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/branch-mult-fwd.ll:27: ; ASM-NEXT:     # [sp, #8] =
def.pseudo
On 2015/11/22 03:40:47, stichnot wrote:
> This makes me wonder - should we really be emitting such pseudo-instructions
in
> a comment?

Acknowledged.