This is an archive of the discontinued LLVM Phabricator instance.

[NFC] [PPC] [AIX] Test improvements for byval arguments that fit in a single register
ClosedPublic

Authored by cebowleratibm on Mar 26 2020, 11:52 AM.

Details

Summary

Some test improvements were requested in https://reviews.llvm.org/D76380 (Implement by-val caller arguments in multiple registers), which are not directly related to that work.

I'd like the test updates reviewed independently of that work.

Diff Detail

Event Timeline

cebowleratibm created this revision.Mar 26 2020, 11:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 11:52 AM
cebowleratibm retitled this revision from [PPC][AIX] Test improvements for byval arguments that fit in a single register to [NFC] [PPC] [AIX] Test improvements for byval arguments that fit in a single register.Mar 26 2020, 12:11 PM
ZarkoCA added inline comments.Mar 27 2020, 6:00 AM
llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
118

I think making one of the floats a double would be more useful to show how the 32BIT AIX ABI works.

181

Same as above with respect to using a double instead of float, but I think you only need do it one time.

cebowleratibm marked 2 inline comments as done.Mar 27 2020, 7:50 AM
cebowleratibm added inline comments.
llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
118

We have other tests that mix floats and doubles. The point here was to validate that the byval arg shuffles the arguments around it properly. I prefer to leave this as-is.

181

I think this is minimal value add.

sfertile accepted this revision.Mar 27 2020, 10:04 AM

One comment, but otherwise LGTM.

llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
125

The AdjustCallStackDown nodes should be a 'CHECK' as opposed to a 'CHECK-DAG'.

This revision is now accepted and ready to land.Mar 27 2020, 10:04 AM
cebowleratibm marked an inline comment as done.Mar 29 2020, 5:17 PM
cebowleratibm added inline comments.
llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
125

The actual output is:

renamable $r3 = LWZtoc @f, $r2 :: (load 4 from got)
renamable $f1 = LFS 0, killed renamable $r3 :: (dereferenceable load 4 from @f)
ADJCALLSTACKDOWN 56, 0, implicit-def dead $r1, implicit $r1
renamable $r3 = LWZtoc @gS2, $r2 :: (load 4 from got)
renamable $r3 = LHZ 0, killed renamable $r3 :: (load 2)
renamable $r5 = RLWINM killed renamable $r3, 16, 0, 15
$r3 = LI 42
$f2 = COPY renamable $f1
$r7 = LI 43
BL_NOP <mcsymbol .test_byval_2Byte>, csr_aix32, implicit-def dead $lr, implicit $rm, implicit $r3, implicit $f1, implicit $r5, implicit killed $f2, implicit killed $r7, implicit $r2, implicit-def $r1, implicit-def dead $r3
ADJCALLSTACKUP 56, 0, implicit-def dead $r1, implicit $r1

I used the CHECK-DAG to keep it in the logical order, however, now I'm thinking we're missing some dependency information in the DAG during the call lowering.

sfertile added inline comments.Mar 30 2020, 7:39 AM
llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
125

Sorry Chris, I didn't consider that we were loading from a global. Because we are loading the float from a global we will have a DAG where the load is before the ADJUSTCALLSTACK nodes, with a CopyToReg from the virtual reg to f1/f2 inside the ADJUSTCALLSTACK nodes. We are testing too late in the pipeline to check for CopyToREG nodes though.

If we had a local with a constant:

%call = call zeroext i8 @test_byval_2Byte(i32 signext 42, float 0x400921FA00000000, %struct.S2* byval(%struct.S2) align 1 @gS2, float 0x400921FA00000000, i32 signext 43)

then we should see the load of the float from the constant pool *inside* the ADJUSTCALLSTACK pair like I was thinking.

I suggest adjusting the tests: either to move the load of the global outside the call sequence or use a constant for the float arguments. But either way don't use CHECK-DAG on the ADJUSTCALLSTACKDOWN node,

Discussion ensued post-approval regarding use of CHECK-DAG on the ADJSTACKDOWN lines so I thought I'd post the final version before committing.

LGTM. Thanks for the updates Chris.

This revision was automatically updated to reflect the committed changes.