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

[sfertile]

Summary

Extends the ByVal formal argument handling for arguments that consume several registers.

[ZarkoCA]

A small suggestion would be to add a test to further show that the GPRs are "burned" properly with with floats in the byval handling. Eg. using a struct like `%struct.S31 = type <{ float, i32, i64, double, i32, double }>' and seeing the error that structs split regs and the stack are not handled.

However, it seems like that will be the next patch that will be done. So it may be useful to save that test for it instead of having it here only to show the error.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

7113

"Passing structs split between registers..."?

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

800

This is a really good struct for a test.

[cebowleratibm]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

7072

"MemLocs already be handled"? :)

7088

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.

7110

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.

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

756

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.

893

same comment on dead stores / failure to elide store/load.

[cebowleratibm]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

7110

Offset != PtrByteSize; => Offset += PtrByteSize;

[sfertile]

• Fixed wording on a fatal error.
• Changed the args and captures on the lambda.
• Restructured the loop as suggested.
• Added comment about the dead-stores in the lit test.

[sfertile]

In D76902#1945938, @ZarkoCA wrote:

A small suggestion would be to add a test to further show that the GPRs are "burned" properly with with floats in the byval handling. Eg. using a struct like `%struct.S31 = type <{ float, i32, i64, double, i32, double }>' and seeing the error that structs split regs and the stack are not handled.

This is a really good suggestion. Going to add this next.

[sfertile]

In D76902#1950131, @sfertile wrote:

In D76902#1945938, @ZarkoCA wrote:

A small suggestion would be to add a test to further show that the GPRs are "burned" properly with with floats in the byval handling. Eg. using a struct like `%struct.S31 = type <{ float, i32, i64, double, i32, double }>' and seeing the error that structs split regs and the stack are not handled.

This is a really good suggestion. Going to add this next.

I forgot that Chris already did this in D76875: see the @test_byval_2Byte.

[sfertile]

Add a test that checks for a fatal error when a ByVal argument is split across registers and stack.

[sfertile]

Rebased

[cebowleratibm]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

7113

The first ArgLoc could be a mem loc, in which case the error is a bit odd. I think it's sufficient to use "Passing ByVals on the stack is not yet implemented." Change or leave it at your discretion.

7117

I think this assertion is redundant to the assertion that will occur on ArgLocs[I] anyways.

[sfertile]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

7113

If you mean the first ArgLoc for the ByVal argument is a MemLoc then that is handled elsewhere, hence why this diagnostic is specific to splitting between registers and stack.

7117

This should have come before any of the reads of ArgLocs[I], I'll move it.

[sfertile]

• rebase to pick up landed dependencies.
• Moved assertion that loop index was still valid to top of loop.
• added a test that exercise fatal error for args split between registers and stack.

[cebowleratibm]

I had intended to move the 5-8 byte byval callee cases out of aix64-cc-byval.ll and put them here. I don't want aix64-cc-byval.ll to exist when we're done. If the 5-8 byte tests aren't useful for the callee I'd rather delete them. They were of interest to the caller handling to make sure that we implement the left justification correctly.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

7077

We currently error on byval alignment > PtrByteSize. I'd be inclined to assert it here as well to make it really hard to emit invalid code.

(Noting that we need to decide how to handle 8 byte aligned structs in 32-bit mode)

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

781

I'm ok with the test as written, though I would have been inclined to write the test like this:

; ASM32-DAG:   stw 3, 24(1)
; ASM32-DAG:   stw 4, 28(1)
; ASM32-DAG:   stw 5, 32(1)
; ASM32-DAG:   stw 6, 36(1)
; ASM32-DAG:   stw 7, 40(1)
; ASM32-DAG:   stw 8, 44(1)
; ASM32-DAG:   stw 9, 48(1)
; ASM32-DAG:   stw 10, 52(1)
; ASM32-DAG:   lbz 3, 45(1)
; ASM32-NEXT:  blr

Understanding that this permits the lbz to move above the stw 3. It's easier to read, accepts more valid outputs at the expense of accepting some invalid outputs. It's been my preferred approach to these tests so far.

[cebowleratibm]

LGTM.

My comments are only suggestions/discussion and can be addressed as you see fit on the commit.

[sfertile]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

7077

Good point. I was expecting CC_AIX to handle it by either burning registers, or adjusting the stack offset to a properly aligned address and so it should be a nop in the formal argument handling. I added a fatal error so that I can remove it and add lit testing after we remove the limitation in CC_AIX.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

SDValue PPCTargetLowering::LowerFormalArguments_AIX(
   const EVT PtrVT = getPointerTy(MF.getDataLayout());
   // Reserve space for the linkage area on the stack.
   const unsigned LinkageSize = Subtarget.getFrameLowering()->getLinkageSize();
   CCInfo.AllocateStack(LinkageSize, PtrByteSize);
   CCInfo.AnalyzeFormalArguments(Ins, CC_AIX);
 
   SmallVector<SDValue, 8> MemOps;
 
-  for (CCValAssign &VA : ArgLocs) {
-    EVT ValVT = VA.getValVT();
+  for (size_t I = 0, End = ArgLocs.size(); I != End; /* No increment here */) {
+    CCValAssign &VA = ArgLocs[I++];
     MVT LocVT = VA.getLocVT();
     ISD::ArgFlagsTy Flags = Ins[VA.getValNo()].Flags;
-    assert((VA.isRegLoc() || VA.isMemLoc()) &&
-           "Unexpected location for function call argument.");
 
     // For compatibility with the AIX XL compiler, the float args in the
     // parameter save area are initialized even if the argument is available
     // in register.  The caller is required to initialize both the register
     // and memory, however, the callee can choose to expect it in either.
     // The memloc is dismissed here because the argument is retrieved from
     // the register.
     if (VA.isMemLoc() && VA.needsCustom())
if (Flags.isByVal() && VA.isMemLoc()) {
           /* IsAliased */ true);
       SDValue FIN = DAG.getFrameIndex(FI, PtrVT);
       InVals.push_back(FIN);
 
       continue;
     }
 
     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.");
 
       const MCPhysReg ArgReg = VA.getLocReg();
       const PPCFrameLowering *FL = Subtarget.getFrameLowering();
-      const unsigned Offset = mapArgRegToOffsetAIX(ArgReg, FL);
 
-      const unsigned StackSize = alignTo(ByValSize, PtrByteSize);
+      const unsigned StackSize = alignTo(Flags.getByValSize(), PtrByteSize);
       const int FI = MF.getFrameInfo().CreateFixedObject(
-          StackSize, Offset, /* IsImmutable */ false, /* IsAliased */ true);
+          StackSize, mapArgRegToOffsetAIX(ArgReg, FL), /* IsImmutable */ false,
+          /* IsAliased */ true);
       SDValue FIN = DAG.getFrameIndex(FI, PtrVT);
-
       InVals.push_back(FIN);
 
-      const unsigned VReg = MF.addLiveIn(ArgReg, IsPPC64 ? &PPC::G8RCRegClass
-                                                         : &PPC::GPRCRegClass);
-
+      // Add live ins for all the RegLocs for the same ByVal.
+      const TargetRegisterClass *RegClass =
+          IsPPC64 ? &PPC::G8RCRegClass : &PPC::GPRCRegClass;
+
+      auto HandleRegLoc = [&](const CCValAssign &RegAssign, unsigned Offset) {
+        const MCPhysReg PhysReg = RegAssign.getLocReg();
+        const unsigned VReg = MF.addLiveIn(PhysReg, RegClass);
         // Since the callers side has left justified the aggregate in the
         // register, we can simply store the entire register into the stack
         // slot.
+        SDValue CopyFrom = DAG.getCopyFromReg(Chain, dl, VReg, LocVT);
         // The store to the fixedstack object is needed becuase accessing a
         // field of the ByVal will use a gep and load. Ideally we will optimize
         // to extracting the value from the register directly, and elide the
         // stores when the arguments address is not taken, but that will need to
         // be future work.
-      SDValue CopyFrom = DAG.getCopyFromReg(Chain, dl, VReg, LocVT);
         SDValue Store =
-          DAG.getStore(CopyFrom.getValue(1), dl, CopyFrom, FIN,
-                       MachinePointerInfo::getFixedStack(MF, FI, 0));
+            DAG.getStore(CopyFrom.getValue(1), dl, CopyFrom,
+                         DAG.getObjectPtrOffset(dl, FIN, Offset),
+                         MachinePointerInfo::getFixedStack(MF, FI, Offset));
 
         MemOps.push_back(Store);
+      };
+
+      unsigned Offset = 0;
+      HandleRegLoc(VA, Offset);
+      while (I != End && ArgLocs[I].getValNo() == VA.getValNo()) {
+        if (!ArgLocs[I].isRegLoc())
+          report_fatal_error(
+              "Passing split between registers and stack not yet implemented.");
+        Offset += PtrByteSize;
+        HandleRegLoc(ArgLocs[I], Offset);
+        ++I;
+      }
       continue;
     }
 
+    EVT ValVT = VA.getValVT();
     if (VA.isRegLoc()) {
       MVT::SimpleValueType SVT = ValVT.getSimpleVT().SimpleTy;
       unsigned VReg =
           MF.addLiveIn(VA.getLocReg(), getRegClassForSVT(SVT, IsPPC64));
       SDValue ArgValue = DAG.getCopyFromReg(Chain, dl, VReg, LocVT);
       if (ValVT.isScalarInteger() &&
           (ValVT.getSizeInBits() < LocVT.getSizeInBits())) {
         ArgValue =

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

 @gS32 = external global %struct.S32, align 1
 
 define void @call_test_byval_32Byte() {
 entry:
   %call = call zeroext i8 @test_byval_32Byte(%struct.S32* byval(%struct.S32) align 1 @gS32)
   ret void
 }
 
-declare zeroext i8 @test_byval_32Byte(%struct.S32* byval(%struct.S32) align 1 %s)
-
 ; CHECK-LABEL: name: call_test_byval_32Byte{{.*}}
 
 ; The DAG block permits some invalid inputs for the benefit of allowing more valid orderings.
 ; 32BIT:       ADJCALLSTACKDOWN 56, 0, implicit-def dead $r1, implicit $r1
 ; 32BIT-NEXT:  renamable $r[[REGADDR:[0-9]+]] = LWZtoc @gS32, $r2 :: (load 4 from got)
 ; 32BIT-DAG:   renamable $r3 = LWZ 0, killed renamable $r[[REGADDR]] :: (load 4)
 ; 32BIT-DAG:   renamable $r4 = LWZ 4, renamable $r[[REGADDR]] :: (load 4)
 ; 32BIT-DAG:   renamable $r5 = LWZ 8, renamable $r[[REGADDR]] :: (load 4)

 ; ASM64-NEXT:  ld [[REGADDR:[0-9]+]], LC{{[0-9]+}}(2)
 ; ASM64-DAG:   ld 3, 0([[REGADDR]])
 ; ASM64-DAG:   ld 4, 8([[REGADDR]])
 ; ASM64-DAG:   ld 5, 16([[REGADDR]])
 ; ASM64-DAG:   ld 6, 24([[REGADDR]])
 ; ASM64-NEXT:  bl .test_byval_32Byte
 ; ASM64-NEXT:  nop
 
+define zeroext i8 @test_byval_32Byte(%struct.S32* byval(%struct.S32) align 1 %s) {
+entry:
+  %arrayidx = getelementptr inbounds %struct.S32, %struct.S32* %s, i32 0, i32 0, i32 21
+  %0 = load i8, i8* %arrayidx, align 1
+  ret i8 %0
+}
+
+; CHECK-LABEL: name:            test_byval_32Byte
+
+; 32BIT:      fixedStack:
+; 32BIT-NEXT:   - { id: 0, type: default, offset: 24, size: 32, alignment: 8, stack-id: default,
+; 32BIT-NEXT:       isImmutable: false, isAliased: true, callee-saved-register: '', callee-saved-restored: true,
+
+; 32BIT:      bb.0.entry:
+; 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
+; 32BIT-DAG:    STW killed renamable $r5,   8, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 8
+; 32BIT-DAG:    STW killed renamable $r6,  12, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 12
+; 32BIT-DAG:    STW killed renamable $r7,  16, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 16
+; 32BIT-DAG:    STW killed renamable $r8,  20, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 20
+; 32BIT-DAG:    STW killed renamable $r9,  24, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 24
+; 32BIT-DAG:    STW killed renamable $r10, 28, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 28
+; 32BIT:        renamable $r3 = LBZ 21, %fixed-stack.0 :: (dereferenceable load 1
+; 32BIT:        BLR
+
+; 64BIT:      fixedStack:
+; 64BIT-NEXT:   - { id: 0, type: default, offset: 48, size: 32, alignment: 16, stack-id: default,
+; 64BIT-NEXT:       isImmutable: false, isAliased: true, callee-saved-register: '', callee-saved-restored: true,
+
+; 64BIT:      bb.0.entry:
+; 64BIT-NEXT:   liveins: $x3, $x4, $x5, $x6
+; 64BIT-DAG:    STD killed renamable $x3, 0, %fixed-stack.0 :: (store 8 into %fixed-stack.0
+; 64BIT-DAG:    STD killed renamable $x4, 8, %fixed-stack.0 :: (store 8 into %fixed-stack.0 + 8
+; 64BIT-DAG:    STD killed renamable $x5, 16, %fixed-stack.0 :: (store 8 into %fixed-stack.0 + 16
+; 64BIT-DAG:    STD killed renamable $x6, 24, %fixed-stack.0 :: (store 8 into %fixed-stack.0 + 24
+; 64BIT-NEXT:   renamable $x3 = LBZ8 21, %fixed-stack.0 :: (dereferenceable load 1
+; 64BIT-NEXT:   BLR8
+
+; ASM-LABEL: .test_byval_32Byte:
+
+; ASM32:       stw 8, 44(1)
+; ASM32:       stw 3, 24(1)
+; ASM32-DAG:   lbz 3, 45(1)
+; ASM32-DAG:   stw 4, 28(1)
+; ASM32-DAG:   stw 5, 32(1)
+; ASM32-DAG:   stw 6, 36(1)
+; ASM32-DAG:   stw 7, 40(1)
+; ASM32-DAG:   stw 9, 48(1)
+; ASM32-DAG:   stw 10, 52(1)
+; ASM32-NEXT:  blr
+
+; ASM64:       std 5, 64(1)
+; ASM64:       std 3, 48(1)
+; ASM64-DAG:   lbz 3, 69(1)
+; ASM64-DAG:   std 4, 56(1)
+; ASM64-DAG:   std 6, 72(1)
+; ASM64-NEXT:  blr
 
-%struct.S31 = type { [31 x i8] }
+%struct.S31 = type <{ float, i32, i64, double, i32, i16, i8 }>
 
 @gS31 = external global %struct.S31, align 1
 
 define void @call_test_byval_31Byte() {
 entry:
-  %call = call zeroext i8 @test_byval_31Byte(%struct.S31* byval(%struct.S31) align 1 @gS31)
+  %call = call double @test_byval_31Byte(%struct.S31* byval(%struct.S31) align 1 @gS31)
   ret void
 }
 
-declare zeroext i8 @test_byval_31Byte(%struct.S31* byval(%struct.S31) align 1)
 
 ; CHECK-LABEL: name: call_test_byval_31Byte{{.*}}
 
 ; The DAG block permits some invalid inputs for the benefit of allowing more valid orderings.
 ; 32BIT:       ADJCALLSTACKDOWN 56, 0, implicit-def dead $r1, implicit $r1
 ; 32BIT-NEXT:  renamable $r[[REGADDR:[0-9]+]] = LWZtoc @gS31, $r2 :: (load 4 from got)
 ; 32BIT-DAG:   renamable $r3 = LWZ 0, killed renamable $r[[REGADDR]] :: (load 4)
 ; 32BIT-DAG:   renamable $r4 = LWZ 4, renamable $r[[REGADDR]] :: (load 4)

 ; ASM64-DAG:   lwz [[REG1:[0-9]+]], 24([[REGADDR]])
 ; ASM64-DAG:   lhz [[REG2:[0-9]+]], 28([[REGADDR]])
 ; ASM64-DAG:   lbz [[REG3:[0-9]+]], 30([[REGADDR]])
 ; ASM64-DAG:   rlwinm 6, [[REG3]], 8, 16, 23
 ; ASM64-DAG:   rlwimi 6, [[REG2]], 16, 0, 15
 ; ASM64-DAG:   rldimi 6, [[REG1]], 32, 0
 ; ASM64-NEXT:  bl .test_byval_31Byte
 ; ASM64-NEXT:  nop
+
+define double @test_byval_31Byte(%struct.S31* byval(%struct.S31) align 1 %s) {
+entry:
+  %gep = getelementptr inbounds %struct.S31, %struct.S31* %s, i32 0, i32 3
+  %load = load double, double* %gep, align 1
+  ret double %load
+}
+
+; CHECK-LABEL: name:            test_byval_31Byte
+
+; 32BIT:      fixedStack:
+; 32BIT-NEXT:   - { id: 0, type: default, offset: 24, size: 32, alignment: 8, stack-id: default,
+; 32BIT-NEXT:       isImmutable: false, isAliased: true, callee-saved-register: '', callee-saved-restored: true,
+
+; 32BIT:      bb.0.entry:
+; 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
+; 32BIT-DAG:    STW killed renamable $r5,   8, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 8
+; 32BIT-DAG:    STW killed renamable $r6,  12, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 12
+; 32BIT-DAG:    STW killed renamable $r7,  16, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 16
+; 32BIT-DAG:    STW killed renamable $r8,  20, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 20
+; 32BIT-DAG:    STW killed renamable $r9,  24, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 24
+; 32BIT-DAG:    STW killed renamable $r10, 28, %fixed-stack.0 :: (store 4 into %fixed-stack.0 + 28
+; 32BIT-NEXT:   renamable $f1 = LFD 16, %fixed-stack.0 :: (dereferenceable load 8
+; 32BIT-NEXT:   BLR
+
+; 64BIT:      fixedStack:
+; 64BIT-NEXT:   - { id: 0, type: default, offset: 48, size: 32, alignment: 16, stack-id: default,
+; 64BIT-NEXT:       isImmutable: false, isAliased: true, callee-saved-register: '', callee-saved-restored: true,
+
+; 64BIT:      bb.0.entry:
+; 64BIT-NEXT:   liveins: $x3, $x4, $x5, $x6
+; 64BIT-DAG:    STD killed renamable $x3,  0, %fixed-stack.0 :: (store 8 into %fixed-stack.0
+; 64BIT-DAG:    STD killed renamable $x4,  8, %fixed-stack.0 :: (store 8 into %fixed-stack.0 + 8
+; 64BIT-DAG:    STD killed renamable $x5, 16, %fixed-stack.0 :: (store 8 into %fixed-stack.0 + 16
+; 64BIT-DAG:    STD killed renamable $x6, 24, %fixed-stack.0 :: (store 8 into %fixed-stack.0 + 24
+; 64BIT-NEXT:   renamable $f1 = LFD 16, %fixed-stack.0 :: (dereferenceable load 8
+; 64BIT-NEXT:   BLR8
+
+; ASM32-LABEL: .test_byval_31Byte:
+
+; ASM32-DAG:      stw 8, 44(1)
+; ASM32:          stw 7, 40(1)
+; ASM32-DAG:      lfd 1, 40(1)
+; ASM32-DAG:      stw 3, 24(1)
+; ASM32-DAG:      stw 4, 28(1)
+; ASM32-DAG:      stw 5, 32(1)
+; ASM32-DAG:      stw 6, 36(1)
+; ASM32-DAG:      stw 9, 48(1)
+; ASM32-DAG:      stw 10, 52(1)
+; ASM32-NEXT:     blr
+
+; ASM64:          std 5, 64(1)
+; ASM64:          lfd 1, 64(1)
+; ASM64-DAG:      std 3, 48(1)
+; ASM64-DAG:      std 4, 56(1)
+; ASM64-DAG:      std 6, 72(1)
+; ASM64-NEXT:     blr