[PATCH] D74225: [AIX] Implement formal arguments passed in stack memory

Wed Feb 26 10:34:41 PST 2020

cebowleratibm added a comment.

I think we need some changes to ensure frame objects are created with their proper size and not word size.  i64 is passed as two i32s but we should coalesce these into a single 8-byte object.  Like-wise arguments smaller than word size are generating frame objects that are word size.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7010
+      int CurArgOffset = VA.getLocMemOffset();
+      // Objects in AIX are right justified.
+      if (ArgSize < ObjSize)
----------------
cebowleratibm wrote:
> cebowleratibm wrote:
> > suggest:
> > 
> > // AIX objects are right justified because they are word written to stack memory from their register image.
> > 
> updated suggestion:
> 
> // Objects are right-justified because AIX is big-endian.
> 
> I tend to avoid the use of the term word because in Power ISA it means 4 bytes.  To others it means PtrByteSize.
I don't see the updated comment reflected in the current patch.  I still see:

    // AIX objects are right justified because they are word written to stack
    // memory from their register image.

and I would prefer this to say 

   // Objects are right-justified because AIX is big-endian.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1320
+; 32BIT-DAG:   - { id: 1, type: default, offset: 88, size: 4
+; 32BIT-DAG:   - { id: 0, type: default, offset: 92, size: 4
+
----------------
When i looked at Linux PPC32 I saw that they used 8 byte frame objects for the long long parameters.  I think we should be consistent with the precedent.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1352
+; ASM32PWR4-DAG:   add 3, 3, 4
+; ASM32PWR4-DAG:   lwz [[REG1:[0-9]+]], 60(1)
+; ASM32PWR4-DAG:   add 3, 3, 5
----------------
I got confused by why there's no load at 56 to account for the msw of the 9th parameter (i64) then realized the result is stored in i32, so the optimizer is probably getting clever.  To keep the test straight forward and avoid confusion, I'd suggest accumulating the result in an i64 so that we'll see (and can validate) the load at 56.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1356
+; ASM32PWR4-DAG:   add 3, 3, 7
+; ASM32PWR4-DAG:   lwz [[REG2:[0-9]+]], 64(1)
+; ASM32PWR4-DAG:   add 3, 3, 8
----------------
My preference is to see the:
add 3, 3, 5
add 3, 3, 6
add 3,3, 7
...
as a block and coalesce the stack loads next to where they're used.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1382
+; ASM64PWR4-DAG:   lwz [[REG7:[0-9]+]], 164(1)
+; ASM64PWR4-DAG:   lwa [[REG8:[0-9]+]], 172(1)
+
----------------
The ASM32PWR4-DAG expects the add operations but they're omitted in the ASM64PWR4-DAG expected output.

I don't strongly prefer either though I strongly prefer consistency.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1409
+
+; 32BIT-DAG:   renamable $r[[REGLL1:[0-9]+]] = LWZtoc @ll1, $r2 :: (load 4 from got)
+; 32BIT-DAG:   renamable $r[[REGLL1ADDR1:[0-9]+]] = LWZ 0, renamable $r[[REGLL1]] :: (dereferenceable load 4 from @ll1, align 8)
----------------
I usually name the toc load appending ADDR to indicate the register holds the address of the variable, then name the loads after to indicate they hold the value of the variable.  Seems the naming you have here his backwards.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1438
+; 32BIT-DAG:   ADJCALLSTACKDOWN 96, 0, implicit-def dead $r1, implicit $r1
+; 32BIT-DAG:   $r3 = LI 1
+; 32BIT-DAG:   $r4 = LI 2
----------------
Logically I like to see the parameter initializations in order.  I find it easier to understand in the approach.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1499
+; ASM32PWR4-DAG:    lwz [[REG8:[0-9]+]], LC17(2)
+; ASM32PWR4-DAG:    lha 5, 0([[REG1]])
+; ASM32PWR4-DAG:    lwz 11, 0([[REG7]])
----------------
would be a bit nicer to tidy the order of parameter reg inits.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1632
+; ASM32PWR4:       fadd 0, 1, 2
+; ASM32PWR4-DAG:   lfs  [[REG1:[0-9]+]], 128(1)
+; ASM32PWR4-DAG:   fadd 0, 0, 3
----------------
A little bit nicer if these are moved next to their use in the DAG block.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1693
+; 32BIT-DAG:   STW killed renamable $r[[SCRATCHREG:[0-9]+]], 80, $r1 :: (store 4, align 8)
+; 32BIT-DAG:   STW renamable $r[[SCRATCHREG:[0-9]+]], 84, $r1 :: (store 4)
+; 32BIT-DAG:   STW killed renamable $r[[SCRATCHREG:[0-9]+]], 88, $r1 :: (store 4, align 8)
----------------
why are all the other STW flagged "killed" and this one not?

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1713
+; 32BIT-DAG:   renamable $r[[SCRATCHREG:[0-9]+]] = LWZ 0, killed renamable $r[[REGF2]] :: (load 4 from @f16)
+; 32BIT-DAG:   renamable $r[[SCRATCHREG:[0-9]+]] = LWZtoc %const.0, $r2 :: (load 4 from got)
+; 32BIT-DAG:   renamable $r[[SCRATCHREG:[0-9]+]] = LWZtoc %const.1, $r2 :: (load 4 from got)
----------------
I don't think that it's useful to expect the constant pool loads given that the STW matches on SCRATCHREG.  There's no validation of where each constant is written and we're not intending to test the constant loads anyways.

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1781
+; ASM32PWR4-DAG:   stwu 1, -144(1)
+; ASM32PWR4-DAG:   lwz [[REGD:[0-9]+]], LC18(2)
+; ASM32PWR4-DAG:   lwz [[REGF1:[0-9]+]], LC19(2)
----------------
REGDADDR

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1784
+; ASM32PWR4-DAG:   lwz [[REGF2:[0-9]+]], LC20(2)
+; ASM32PWR4-DAG:   lfd [[REGDNEW:[0-9]+]], 0([[REGD]])
+; ASM32PWR4-DAG:   lwz [[REGF1NEW:[0-9]+]], 0([[REGF1]])
----------------
lfd [[REGD:[0-9]+]], 0([[REGDADDR]])

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1785
+; ASM32PWR4-DAG:   lfd [[REGDNEW:[0-9]+]], 0([[REGD]])
+; ASM32PWR4-DAG:   lwz [[REGF1NEW:[0-9]+]], 0([[REGF1]])
+; ASM32PWR4-DAG:   lwz [[REGF2NEW:[0-9]+]], 0([[REGF2]])
----------------
diddo on REGF1 and REGF2

================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1861
+; 32BIT-LABEL: fixedStack:
+; 32BIT-DAG:   - { id: 6, type: default, offset: 56, size: 4
+; 32BIT-DAG:   - { id: 5, type: default, offset: 60, size: 4
----------------
Should be size 2.  Though the arg passes in 4 bytes, the object is 2 bytes at offset 58.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74225/new/

https://reviews.llvm.org/D74225