[PATCH] D75863: [AIX] Implement by-val caller arguments in a single register

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7156
+    const MVT ValVT = VA.getValVT();
+    assert((VA.isRegLoc() || VA.isMemLoc()) &&
+           "Unexpected location for function call argument.");
----------------
I don't think we need an assertion like this:  CCValAssign is either a RegLoc or a MemLoc. by the class construction. Its not possible to create an object with both of these false. If the class design ever changes such that there are other options then we can add an assertion then.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7167
+      // ByValSize. For example: a 7 byte by-val arg requires 4, 2 and 1 byte
+      // loads.
+      SDValue RegVal;
----------------
Could we do a larger load if the alignment is greater then the size? For example if we have a size of 7 and a type alignment of 1, but the argument is an auto local and the compiler increase the alignment to 8. I don't suggest making any such changes to this patch if it allowed, but something to consider once we have everything working.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7177
+        if (Bytes != 0) {
+          // Move the load over by the number of bytes read so far.
+          SDNodeFlags Flags;
----------------
suggestion:
`Move the load over ` --> `Adjust the load offset`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7189
+        assert(LocVT.getSizeInBits() >= (Bytes * 8));
+        if (unsigned PaddingInBits = LocVT.getSizeInBits() - (Bytes * 8)) {
+          // By-val arguments are passed left-justfied in register.
----------------
Why the name `PaddingInBits`? Isn't this is the shift size  to justify the bytes just loaded, where does padding come in?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval.ll:38
+; ASM32PWR4:       stwu 1, -64(1)
+; ASM32PWR4-NEXT:  lwz [[REG:[0-9]+]], LC[[IDX:[0-9]+]](2)
+; ASM32PWR4-NEXT:  lbz 3, 0([[REG]])
----------------
IDX isn't used again. You can use just the regex to match the label without having to store it in a file check variable. Ditto on the other filecheck variables that aren't used after capture.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval.ll:125
+; 32BIT-NEXT:  renamable $r[[REGADDR:[0-9]+]] = LWZtoc @gS3, $r2 :: (load 4 from got)
+; 32BIT-DAG:   renamable $r[[REG2BYTE:[0-9]+]] = LHZ 0, killed renamable $r[[REGADDR]] :: (load 2)
+; 32BIT-DAG:   renamable $r[[REG1BYTE:[0-9]+]] = LBZ 2, renamable $r[[REGADDR]] :: (load 1)
----------------
Can we drop the 'BYTE' part of REG2BYTE and REG1BYTE? 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval.ll:136
+; ASM32PWR4:       stwu 1, -64(1)
+; ASM32PWR4-NEXT:  lwz [[REGADDR:[0-9]+]], LC2(2)
+; ASM32PWR4-DAG:   lhz [[REG2BYTE:[0-9]+]], 0([[REGADDR]])
----------------
`LC2(2)` --> `LC{{[0-9]+}}(2)`


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval.ll:156
+; ASM64PWR4:       stdu 1, -112(1)
+; ASM64PWR4-NEXT:  ld [[REGADDR:[0-9]+]], LC2(2)
+; ASM64PWR4-DAG:   lhz [[REG2BYTE:[0-9]+]], 0([[REGADDR]])
----------------
Same fix mentioned above with 'LC2(2)'.


================
Comment at: llvm/test/CodeGen/PowerPC/aix64-cc-byval.ll:27
+; 64BIT-NEXT:  renamable $x[[REGADDR:[0-9]+]] = LDtoc @gS5, $x2 :: (load 8 from got)
+; 64BIT-DAG:   renamable $x[[REG4BYTE:[0-9]+]] = LWZ8 0, killed renamable $x[[REGADDR]] :: (load 4)
+; 64BIT-DAG:   renamable $x[[REG1BYTE:[0-9]+]] = LBZ8 4, renamable $x[[REGADDR]] :: (load 1)
----------------
Same suggestion about dropping 'BYTE' from the variables name.


================
Comment at: llvm/test/CodeGen/PowerPC/aix64-cc-byval.ll:36
+; ASM64PWR4:       stdu 1, -112(1)
+; ASM64PWR4-NEXT:  ld [[REGADDR:[0-9]+]], LC0(2)
+; ASM64PWR4-DAG:   lwz [[REG4BYTE:[0-9]+]], 0([[REGADDR]])
----------------
Same fix on the label here and elsewhere in the test : `LC{{[0-9]+}}` 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75863