[ARM] GlobalISel: Lower more than 4 arguments

[rovka]

Summary

This adds support for lowering more than 4 arguments (although still i32 only).
It uses the handleAssignments / ValueHandler infrastructure extracted from
the AArch64 backend in D27045.

[t.p.northover]

lib/Target/ARM/ARMCallLowering.cpp

51 ↗

(On Diff #79523)

This is silently ignoring the signext/zeroext modifiers that are needed to correctly handle small types on AAPCS. It'd probably be better to fallback or assert if they're not being supported yet.

lib/Target/ARM/ARMInstructionSelector.cpp

84–86 ↗

(On Diff #79523)

Is this selection stuff intentionally in the same patch? Seems like it's an orthogonal issue.

[qcolombet]

lib/Target/ARM/ARMCallLowering.cpp

51 ↗

(On Diff #79523)

I would recommend to change the ValueHandler APIs to return boolean so we can indeed fallback while stuff is not supported.

[rovka]

lib/Target/ARM/ARMCallLowering.cpp

51 ↗

(On Diff #79523)

Right now we're falling back before reaching this point, by returning false from ARMCallLowering::lowerReturn if the type is not strictly 32-bit wide (see line 86). I think assertions would be more appropriate in this case, because it's not too complicated to check all the value types up front before calling this. I'll add some throughout.

lib/Target/ARM/ARMInstructionSelector.cpp

84–86 ↗

(On Diff #79523)

It's intentional, I wanted to have end-to-end support for more than 4 arguments, and that includes selecting loads from the stack.

[rovka]

Split out the part that was NFC with regard to D26677 (using ValueHandler for <= 4 args). This patch is now only concerned with lowering more than 4 arguments (i.e. loading from the stack). Hopefully it's easier to review now.

[rovka]

Rebase for changes in D26677.

[t.p.northover]

Looks reasonable to me, modulo splitting it up slightly (which isn't even essential).

lib/Target/ARM/ARMInstructionSelector.cpp

84–86 ↗

(On Diff #79523)

Yep, but while arg lowering depends on selection, the reverse isn't true. You could use and test these instructions without touching any CallLowering file. I think the changes are completely sound but the commit messages might be more coherent if you did it in two stages ("select XYZ" and "support more args") .

[rovka]

Thanks for the review, I'll commit it in 2 rounds :)

llvm/trunk/lib/Target/ARM/ARMCallLowering.cpp

 //===----------------------------------------------------------------------===//
 
 #include "ARMCallLowering.h"
 
 #include "ARMBaseInstrInfo.h"
 #include "ARMISelLowering.h"
 
 #include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
+#include "llvm/CodeGen/MachineRegisterInfo.h"
 
 using namespace llvm;
 
 #ifndef LLVM_BUILD_GLOBAL_ISEL
 #error "This shouldn't be built without GISel"
 #endif
 
 ARMCallLowering::ARMCallLowering(const ARMTargetLowering &TLI)

 
 namespace {
 struct FormalArgHandler : public CallLowering::ValueHandler {
   FormalArgHandler(MachineIRBuilder &MIRBuilder, MachineRegisterInfo &MRI)
       : ValueHandler(MIRBuilder, MRI) {}
 
   unsigned getStackAddress(uint64_t Size, int64_t Offset,
                            MachinePointerInfo &MPO) override {
-    llvm_unreachable("Don't know how to get a stack address yet");
+    assert(Size == 4 && "Unsupported size");
+
+    auto &MFI = MIRBuilder.getMF().getFrameInfo();
+
+    int FI = MFI.CreateFixedObject(Size, Offset, true);
+    MPO = MachinePointerInfo::getFixedStack(MIRBuilder.getMF(), FI);
+
+    unsigned AddrReg =
+        MRI.createGenericVirtualRegister(LLT::pointer(MPO.getAddrSpace(), 32));
+    MIRBuilder.buildFrameIndex(AddrReg, FI);
+
+    return AddrReg;
+  }
+
+  void assignValueToAddress(unsigned ValVReg, unsigned Addr, uint64_t Size,
+                            MachinePointerInfo &MPO, CCValAssign &VA) override {
+    assert(Size == 4 && "Unsupported size");
+
+    auto MMO = MIRBuilder.getMF().getMachineMemOperand(
+        MPO, MachineMemOperand::MOLoad, Size, /* Alignment */ 0);
+    MIRBuilder.buildLoad(ValVReg, Addr, *MMO);
   }
 
   void assignValueToReg(unsigned ValVReg, unsigned PhysReg,
                         CCValAssign &VA) override {
     assert(VA.isRegLoc() && "Value shouldn't be assigned to reg");
     assert(VA.getLocReg() == PhysReg && "Assigning to the wrong reg?");
 
     assert(VA.getValVT().getSizeInBits() == 32 && "Unsupported value size");
     assert(VA.getLocVT().getSizeInBits() == 32 && "Unsupported location size");
 
     MIRBuilder.getMBB().addLiveIn(PhysReg);
     MIRBuilder.buildCopy(ValVReg, PhysReg);
   }
-
-  void assignValueToAddress(unsigned ValVReg, unsigned Addr, uint64_t Size,
-                            MachinePointerInfo &MPO, CCValAssign &VA) override {
-    llvm_unreachable("Don't know how to assign a value to an address yet");
-  }
 };
 } // End anonymous namespace
 
 bool ARMCallLowering::lowerFormalArguments(MachineIRBuilder &MIRBuilder,
                                            const Function &F,
                                            ArrayRef<unsigned> VRegs) const {
   // Quick exit if there aren't any args
   if (F.arg_empty())
     return true;
 
-  // Stick to only 4 arguments for now
-  if (F.arg_size() > 4)
-    return false;
-
   if (F.isVarArg())
     return false;
 
   auto DL = MIRBuilder.getMF().getDataLayout();
   auto &TLI = *getTLI<ARMTargetLowering>();
 
   auto &Args = F.getArgumentList();
   for (auto &Arg : Args)

llvm/trunk/test/CodeGen/ARM/GlobalISel/arm-irtranslator.ll

 ; CHECK: [[VREGY:%[0-9]+]]{{.*}} = COPY %r1
 ; CHECK: [[SUM:%[0-9]+]]{{.*}} = G_ADD [[VREGX]], [[VREGY]]
 ; CHECK: %r0 = COPY [[SUM]]
 ; CHECK: BX_RET 14, _, implicit %r0
 entry:
   %sum = add i32 %x, %y
   ret i32 %sum
 }
+
+define i32 @test_many_args(i32 %p0, i32 %p1, i32 %p2, i32 %p3, i32 %p4, i32 %p5) {
+; CHECK-LABEL: name: test_many_args
+; CHECK: fixedStack:
+; CHECK-DAG: id: [[P4:[0-9]]]{{.*}}offset: 0{{.*}}size: 4
+; CHECK-DAG: id: [[P5:[0-9]]]{{.*}}offset: 4{{.*}}size: 4
+; CHECK: liveins: %r0, %r1, %r2, %r3
+; CHECK: [[VREGP2:%[0-9]+]]{{.*}} = COPY %r2
+; CHECK: [[FIP5:%[0-9]+]]{{.*}} = G_FRAME_INDEX %fixed-stack.[[P5]]
+; CHECK: [[VREGP5:%[0-9]+]]{{.*}} = G_LOAD [[FIP5]]
+; CHECK: [[SUM:%[0-9]+]]{{.*}} = G_ADD [[VREGP2]], [[VREGP5]]
+; CHECK: %r0 = COPY [[SUM]]
+; CHECK: BX_RET 14, _, implicit %r0
+entry:
+  %sum = add i32 %p2, %p5
+  ret i32 %sum
+}

llvm/trunk/test/CodeGen/ARM/GlobalISel/arm-isel.ll

 define i32 @test_add(i32 %x, i32 %y) {
 ; CHECK-LABEL: test_add:
 ; CHECK: add r0, r0, r1
 ; CHECK: bx lr
 entry:
   %sum = add i32 %x, %y
   ret i32 %sum
 }
+
+define i32 @test_many_args(i32 %p0, i32 %p1, i32 %p2, i32 %p3, i32 %p4, i32 %p5) {
+; CHECK-LABEL: test_many_args:
+; CHECK: add [[P5ADDR:r[0-9]+]], sp, #4
+; CHECK: ldr [[P5:r[0-9]+]], {{.*}}[[P5ADDR]]
+; CHECK: add r0, r2, [[P5]]
+; CHECK: bx lr
+entry:
+  %sum = add i32 %p2, %p5
+  ret i32 %sum
+}