[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

52

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

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

[qcolombet]

lib/Target/ARM/ARMCallLowering.cpp

52

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

52

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

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

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 :)

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)
FuncReturnHandler(MachineIRBuilder &MIRBuilder, MachineRegisterInfo &MRI,
       : ValueHandler(MIRBuilder, MRI), MIB(MIB) {}
 
   unsigned getStackAddress(uint64_t Size, int64_t Offset,
                            MachinePointerInfo &MPO) override {
     llvm_unreachable("Don't know how to get a stack address yet");
   }
 
   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.buildCopy(PhysReg, ValVReg);
     MIB.addUse(PhysReg, RegState::Implicit);

 
 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 == 32 && "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 == 32 && "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)

lib/Target/ARM/ARMInstructionSelector.cpp

bool ARMInstructionSelector::select(MachineInstr &I) const {
   if (!isPreISelGenericOpcode(I.getOpcode())) {
     if (I.isCopy()) {
       return selectCopy(I, TII, MRI, TRI, RBI);
     }
 
     return true;
   }
 
-  if (I.getOpcode() == TargetOpcode::G_ADD) {
+  MachineInstrBuilder MIB{MF, I};
+
+  using namespace TargetOpcode;
+  switch (I.getOpcode()) {
+  case G_ADD:
     I.setDesc(TII.get(ARM::ADDrr));
-    AddDefaultCC(AddDefaultPred(MachineInstrBuilder(MF, I)));
+    AddDefaultCC(AddDefaultPred(MIB));
+    return constrainSelectedInstRegOperands(I, TII, TRI, RBI);
+  case G_FRAME_INDEX:
+    // Add 0 to the given frame index and hope it will eventually be folded into
+    // the user(s).
+    I.setDesc(TII.get(ARM::ADDri));
+    AddDefaultCC(AddDefaultPred(MIB.addImm(0)));
+    return constrainSelectedInstRegOperands(I, TII, TRI, RBI);
+  case G_LOAD:
+    I.setDesc(TII.get(ARM::LDRi12));
+    AddDefaultPred(MIB.addImm(0));
     return constrainSelectedInstRegOperands(I, TII, TRI, RBI);
   }
 
   return false;
 }

lib/Target/ARM/ARMLegalizerInfo.cpp

 using namespace llvm;
 
 #ifndef LLVM_BUILD_GLOBAL_ISEL
 #error "You shouldn't build this"
 #endif
 
 ARMLegalizerInfo::ARMLegalizerInfo() {
   using namespace TargetOpcode;
+  const LLT p0 = LLT::pointer(0, 32);
   const LLT s32 = LLT::scalar(32);
 
+  setAction({G_FRAME_INDEX, p0}, Legal);
+
+  setAction({G_LOAD, s32}, Legal);
+  setAction({G_LOAD, 1, p0}, Legal);
+
   setAction({G_ADD, s32}, Legal);
 
   computeTables();
 }

lib/Target/ARM/ARMRegisterBankInfo.cpp

ARMRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
   // Try the default logic for non-generic instructions that are either copies
   // or already have some operands assigned to banks.
   if (!isPreISelGenericOpcode(Opc)) {
     InstructionMapping Mapping = getInstrMappingImpl(MI);
     if (Mapping.isValid())
       return Mapping;
   }
 
-  if (Opc == TargetOpcode::G_ADD) {
-    unsigned Cost = 1;
-    unsigned NumOperands = MI.getNumOperands();
-    ValueMapping *OperandsMapping = &ARM::ValueMappings[0];
-    return InstructionMapping{DefaultMappingID, Cost, OperandsMapping,
-                              NumOperands};
-  }
+  using namespace TargetOpcode;
+
+  const ValueMapping *OperandsMapping = &ARM::ValueMappings[0];
 
+  switch (Opc) {
+  case G_ADD:
+  case G_LOAD:
+    // FIXME: We're abusing the fact that everything lives in a GPR for now; in
+    // the real world we would use different mappings.
+    OperandsMapping = &ARM::ValueMappings[0];
+    break;
+  case G_FRAME_INDEX:
+    OperandsMapping = getOperandsMapping({&ARM::ValueMappings[0], nullptr});
+    break;
+  default:
     return InstructionMapping{};
   }
+
+  return InstructionMapping{DefaultMappingID, Cost, OperandsMapping,
+                            NumOperands};
+}

test/CodeGen/ARM/GlobalISel/arm-instruction-select.mir

 # RUN: llc -O0 -mtriple arm-- -global-isel -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck %s
 --- |
   define void @test_adds32() { ret void }
+  define void @test_load_from_stack() { ret void }
 ...
 ---
 name:            test_adds32
 # CHECK-LABEL: name: test_adds32
 legalized:       true
 regBankSelected: true
 selected:        false
 # CHECK: selected: true
bb.0:
     ; CHECK: [[VREGSUM:%[0-9]+]] = ADDrr [[VREGX]], [[VREGY]], 14, _, _
 
     %r0 = COPY %2(s32)
     ; CHECK: %r0 = COPY [[VREGSUM]]
 
     BX_RET 14, _, implicit %r0
     ; CHECK: BX_RET 14, _, implicit %r0
 ...
+---
+name:            test_load_from_stack
+# CHECK-LABEL: name: test_load_from_stack
+legalized:       true
+regBankSelected: true
+selected:        false
+# CHECK: selected: true
+registers:
+  - { id: 0, class: gprrb }
+  - { id: 1, class: gprrb }
+  - { id: 2, class: gprrb }
+  - { id: 3, class: gprrb }
+fixedStack:
+  - { id: 0, offset: 0, size: 4, alignment: 4, isImmutable: true, isAliased: false }
+  - { id: 1, offset: 4, size: 4, alignment: 4, isImmutable: true, isAliased: false }
+  - { id: 2, offset: 8, size: 4, alignment: 4, isImmutable: true, isAliased: false }
+# CHECK: id: [[FRAME_INDEX:[0-9]+]], offset: 8
+body:             |
+  bb.0:
+    liveins: %r0, %r1, %r2, %r3
+
+    %0(p0) = G_FRAME_INDEX %fixed-stack.2
+    ; CHECK: [[FIVREG:%[0-9]+]] = ADDri %fixed-stack.[[FRAME_INDEX]], 0, 14, _, _
+
+    %1(s32) = G_LOAD %0(p0)
+    ; CHECK: {{%[0-9]+}} = LDRi12 [[FIVREG]], 0, 14, _
+
+    BX_RET 14, _
+    ; CHECK: BX_RET 14, _
+...

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
+}

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
+}

test/CodeGen/ARM/GlobalISel/arm-legalizer.mir

 # RUN: llc -mtriple arm-- -global-isel -run-pass=legalizer %s -o - | FileCheck %s
 --- |
   define void @test_adds32() { ret void }
+  define void @test_load_from_stack() { ret void }
 ...
 ---
 name:            test_adds32
 # CHECK-LABEL: name: test_adds32
 legalized:       false
 # CHECK: legalized: true
 regBankSelected: false
 selected:        false
bb.0:
     %1(s32) = COPY %r1
     %2(s32) = G_ADD %0, %1
     ; G_ADD with s32 is legal, so we should find it unchanged in the output
     ; CHECK: {{%[0-9]+}}(s32) = G_ADD {{%[0-9]+, %[0-9]+}}
     %r0 = COPY %2(s32)
     BX_RET 14, _, implicit %r0
 
 ...
+---
+name:            test_load_from_stack
+# CHECK-LABEL: name: test_load_from_stack
+legalized:       false
+# CHECK: legalized: true
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: _ }
+  - { id: 1, class: _ }
+  - { id: 2, class: _ }
+  - { id: 3, class: _ }
+fixedStack:
+  - { id: 0, offset: 0, size: 4, alignment: 4, isImmutable: true, isAliased: false }
+  - { id: 1, offset: 4, size: 4, alignment: 4, isImmutable: true, isAliased: false }
+  - { id: 2, offset: 8, size: 4, alignment: 4, isImmutable: true, isAliased: false }
+  # CHECK: id: [[FRAME_INDEX:[0-9]+]], offset: 8
+body:             |
+  bb.0:
+    liveins: %r0, %r1, %r2, %r3
+
+    ; This is legal, so we should find it unchanged in the output
+    ; CHECK: [[FIVREG:%[0-9]+]](p0) = G_FRAME_INDEX %fixed-stack.[[FRAME_INDEX]]
+    ; CHECK: {{%[0-9]+}}(s32) = G_LOAD [[FIVREG]](p0)
+    %0(p0) = G_FRAME_INDEX %fixed-stack.2
+    %1(s32) = G_LOAD %0(p0)
+    BX_RET 14, _
+
+...