Fix frame pointer loads/stores in the ARM integrated assembler.

Fix frame pointer loads/stores in the ARM integrated assembler.

This CL passes in context from the ARM target lowering, so that
it can figure out what register (SP or FP) and offset to use
when loading/storing variables.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
R=stichnot@chromium.org

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

[Karl, 2015-11-10 22:09:13]

Description was changed from

==========
Fix frame pointer loads/stores in the ARM integrated assembler.

This CL passes in context from the ARM target lowering, so that
it can figure out what register (SP or FP) and offset to use
when loading/storing variables.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

to

==========
Fix frame pointer loads/stores in the ARM integrated assembler.

This CL passes in context from the ARM target lowering, so that
it can figure out what register (SP or FP) and offset to use
when loading/storing variables.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

[Karl, 2015-11-10 22:09:14]

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

[Jim Stichnoth, 2015-11-11 20:42:09]

https://codereview.chromium.org/1414043015/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1414043015/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:237: const AssemblerARM32::TargetInfo &Target) {
Can you rename the "Target" parameter?  When I see that, I expect it to be a
TargetLowering pointer.  Maybe TI?

https://codereview.chromium.org/1414043015/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:248: if (Target.HasFramePointer)
I thought that we need to adjust the offset only if the target does NOT have a
frame pointer.  Can you take another look at this?

https://codereview.chromium.org/1414043015/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/store-sf.ll (right):

https://codereview.chromium.org/1414043015/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/store-sf.ll:1: ; Sample program that generates "str
reg, [fp, #CCCC]", to show that we
This test case seems vastly over-complicated.  How about this?

define internal void @test_vla_non_entry_block(i32 %n) {
entry:
  br label %next
next:
; Put the variable-length alloca in a non-entry block, to reduce the chance of
; the optimizer putting it before the regular frame creation.
  %v = alloca i8, i32 %n, align 4
  ret void
}

[Karl, 2015-11-11 22:42:57]

https://codereview.chromium.org/1414043015/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1414043015/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:237: const AssemblerARM32::TargetInfo &Target) {
On 2015/11/11 20:42:09, stichnot wrote:
> Can you rename the "Target" parameter?  When I see that, I expect it to be a
> TargetLowering pointer.  Maybe TI?

Used TInfo.

https://codereview.chromium.org/1414043015/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:248: if (Target.HasFramePointer)
On 2015/11/11 20:42:09, stichnot wrote:
> I thought that we need to adjust the offset only if the target does NOT have a
> frame pointer.  Can you take another look at this?

I was wrong. "(!" looked like "(" when I looked at it. Fixing.

https://codereview.chromium.org/1414043015/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/store-sf.ll (right):

https://codereview.chromium.org/1414043015/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/store-sf.ll:1: ; Sample program that generates "str
reg, [fp, #CCCC]", to show that we
On 2015/11/11 20:42:09, stichnot wrote:
> This test case seems vastly over-complicated.  How about this?
> 
> define internal void @test_vla_non_entry_block(i32 %n) {
> entry:
>   br label %next
> next:
> ; Put the variable-length alloca in a non-entry block, to reduce the chance of
> ; the optimizer putting it before the regular frame creation.
>   %v = alloca i8, i32 %n, align 4
>   ret void
> }

Done.

[Jim Stichnoth, 2015-11-11 23:12:27]

lgtm

https://codereview.chromium.org/1414043015/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/store-sf.ll (right):

https://codereview.chromium.org/1414043015/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/store-sf.ll:1: ; Sample program that generates "str
reg, [fp, #CCCC]", to show that we
On 2015/11/11 22:42:57, Karl wrote:
> On 2015/11/11 20:42:09, stichnot wrote:
> > This test case seems vastly over-complicated.  How about this?
> > 
> > define internal void @test_vla_non_entry_block(i32 %n) {
> > entry:
> >   br label %next
> > next:
> > ; Put the variable-length alloca in a non-entry block, to reduce the chance
of
> > ; the optimizer putting it before the regular frame creation.
> >   %v = alloca i8, i32 %n, align 4
> >   ret void
> > }
> 
> Done.

Could you add that comment to the function?  Specifically this:

; Put the variable-length alloca in a non-entry block, to reduce the chance of
; the optimizer putting it before the regular frame creation.

This may help future observers understand the choices made here.

[Karl, 2015-11-11 23:37:54]

Committed patchset #4 (id:60001) manually as
2c68d90fea484ed089ac167ed29c60742c8a6c62 (presubmit successful).

[Karl, 2015-11-11 23:38:38]

https://codereview.chromium.org/1414043015/diff/60001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/store-sf.ll (right):

https://codereview.chromium.org/1414043015/diff/60001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/store-sf.ll:69: ; IASM-NEXT: 	.byte 0xea
Added following comment.