[PATCH] D76902: [PowerPC][AIX] ByVal formal argument support: multiple registers

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7072
     if (Flags.isByVal()) {
-      assert(VA.isRegLoc() && "MemLocs should already be handled.");
-
-      const unsigned ByValSize = Flags.getByValSize();
-      if (ByValSize > PtrByteSize)
-        report_fatal_error("Formal arguments greater then register size not "
-                           "implemented yet.");
+      assert(VA.isRegLoc() && "MemLocs already be handled.");
 
----------------
"MemLocs already be handled"? :)


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7088
+
+      auto HandleRegLoc = [&](const CCValAssign &RegAssign, unsigned Offset) {
+        const MCPhysReg PhysReg = RegAssign.getLocReg();
----------------
Value capture on RegClass and LocVT?  Ref capture works too but isn't necessary, though it does keep the captures simple.  I raise this as a question because I'm not sure what the guidance should be for lambda captures.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7110
+      HandleRegLoc(VA, Offset);
+      while (I != End && ArgLocs[I].getValNo() == VA.getValNo()) {
+        if (!ArgLocs[I].isRegLoc())
----------------
I would tend to write this as:


```
unsigned Offset = 0;
HandleRegLoc(VA, Offset);
Offset != PtrByteSize;
for(; Offset != StackSize; Offset +=PtrByteSize) {
  assert(ArgLocs[I].getValNo() == VA.getValNo() ...);
  HandleRegLoc(ArgLocs[I++], Offset);
}
```

This way we're effectively asserting that we initialize the expected number of bytes.  "I != End" should be part of an assertion.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval.ll:756
+; 32BIT-NEXT:   liveins: $r3, $r4, $r5, $r6, $r7, $r8, $r9, $r10
+; 32BIT-DAG:    STW killed renamable $r3,   0, %fixed-stack.0 :: (store 4 into %fixed-stack.0
+; 32BIT-DAG:    STW killed renamable $r4,   4, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 4
----------------
As before, readers may find the dead stores and failure to elide the store/load for the return odd.  I suggest a brief comment as before.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval.ll:893
+; 32BIT-NEXT:   liveins: $r3, $r4, $r5, $r6, $r7, $r8, $r9, $r10
+; 32BIT-DAG:    STW killed renamable $r3,   0, %fixed-stack.0 :: (store 4 into %fixed-stack.0
+; 32BIT-DAG:    STW killed renamable $r4,   4, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 4
----------------
same comment on dead stores / failure to elide store/load.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76902