[ARM] GlobalISel: Support i8/i16 ABI extensions

[rovka]

Summary

At the moment, this means supporting the signext/zeroext attribute on the return
type of the function. For function arguments, signext/zeroext should be handled
by the caller, so there's nothing for us to do until we start lowering calls.

Note that this does not include support for other extensions (i8 to i16), those will
be added later.

[t.p.northover]

lib/Target/ARM/ARMInstructionSelector.cpp

78

If we don't already, it would be a very good idea to make sure we bail completely in Thumb mode. Incidentally, is there any particular reason you decided to go for ARM first? I thought most things were compiled in Thumb these days.

[rovka]

Bail out if we're in Thumb mode (I'll commit that part separately, I'm just adding it here to make sure it looks ok).

[rovka]

lib/Target/ARM/ARMInstructionSelector.cpp

78

I'm mostly going with ARM first because it seems simpler than Thumb.
At the moment I'm trying to focus on the call lowering + what it needs to get some simple examples through the entire pipeline. I don't want to beef up the instruction selector too much, because that will go away anyway when we move to TableGen. So for instance we can select the extensions that we need for the ABI zeroext/signext, but I'm not really in any rush right now to add other extensions (e.g. from i8 to i16 or whatever).
I'm open to suggestions if this sounds like a bad plan :)

[rovka]

Ping.

[rovka]

Ping

[rovka]

Ping 3.

[rengolin]

Hi Diana,

There's nothing really complicated in this patch, and it looks obvious to me. LGTM. Thanks!

lib/Target/ARM/ARMCallLowering.cpp

struct FuncReturnHandler : public CallLowering::ValueHandler {
   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");
 
-    assert(VA.getLocInfo() != CCValAssign::SExt &&
-           VA.getLocInfo() != CCValAssign::ZExt &&
-           "ABI extensions not supported yet");
-
-    MIRBuilder.buildCopy(PhysReg, ValVReg);
+    unsigned ExtReg = extendRegister(ValVReg, VA);
+    MIRBuilder.buildCopy(PhysReg, ExtReg);
     MIB.addUse(PhysReg, RegState::Implicit);
   }
 
   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");
   }
 
struct FormalArgHandler : public CallLowering::ValueHandler {
   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");
 
+    // The caller should handle all necesary extensions.
     MIRBuilder.getMBB().addLiveIn(PhysReg);
     MIRBuilder.buildCopy(ValVReg, PhysReg);
   }
 };
 } // End anonymous namespace
 
 bool ARMCallLowering::lowerFormalArguments(MachineIRBuilder &MIRBuilder,
                                            const Function &F,

lib/Target/ARM/ARMInstructionSelector.cpp

static bool selectCopy(MachineInstr &I, const TargetInstrInfo &TII,
   if (!RBI.constrainGenericRegister(DstReg, *RC, MRI)) {
     DEBUG(dbgs() << "Failed to constrain " << TII.getName(I.getOpcode())
                  << " operand\n");
     return false;
   }
   return true;
 }
 
+/// Select the opcode for simple extensions (that translate to a single SXT/UXT
+/// instruction). Extension operations more complicated than that should not
+/// invoke this.
+static unsigned selectSimpleExtOpc(unsigned Opc, unsigned Size) {
+  using namespace TargetOpcode;
+
+  assert((Size == 8 || Size == 16) && "Unsupported size");
+
+  if (Opc == G_SEXT)
+    return Size == 8 ? ARM::SXTB : ARM::SXTH;
+
+  if (Opc == G_ZEXT)
+    return Size == 8 ? ARM::UXTB : ARM::UXTH;
+
+  llvm_unreachable("Unsupported opcode");
+}
+
 bool ARMInstructionSelector::select(MachineInstr &I) const {
   assert(I.getParent() && "Instruction should be in a basic block!");
   assert(I.getParent()->getParent() && "Instruction should be in a function!");
 
   auto &MBB = *I.getParent();
   auto &MF = *MBB.getParent();
   auto &MRI = MF.getRegInfo();
 
   if (!isPreISelGenericOpcode(I.getOpcode())) {
     if (I.isCopy())
       return selectCopy(I, TII, MRI, TRI, RBI);
 
     return true;
   }
 
   MachineInstrBuilder MIB{MF, I};
 
   using namespace TargetOpcode;
   switch (I.getOpcode()) {
+  case G_SEXT:
+  case G_ZEXT: {
+    LLT DstTy = MRI.getType(I.getOperand(0).getReg());
+    // FIXME: Smaller destination sizes coming soon!
+    if (DstTy.getSizeInBits() != 32) {
+      DEBUG(dbgs() << "Unsupported destination size for extension");
+      return false;
+    }
+
+    LLT SrcTy = MRI.getType(I.getOperand(1).getReg());
+    unsigned SrcSize = SrcTy.getSizeInBits();
+    switch (SrcSize) {
+    case 8:
+    case 16: {
+      unsigned NewOpc = selectSimpleExtOpc(I.getOpcode(), SrcSize);
+      I.setDesc(TII.get(NewOpc));
+      AddDefaultPred(MIB.addImm(0));
+      break;
+    }
+    default:
+      DEBUG(dbgs() << "Unsupported source size for extension");
+      return false;
+    }
+    break;
+  }
   case G_ADD:
     I.setDesc(TII.get(ARM::ADDrr));
     AddDefaultCC(AddDefaultPred(MIB));
     break;
   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));

lib/Target/ARM/ARMLegalizerInfo.cpp

ARMLegalizerInfo::ARMLegalizerInfo() {
 
   // TODO: smaller types
   setAction({G_LOAD, s32}, Legal);
   setAction({G_LOAD, 1, p0}, Legal);
 
   for (auto Ty : {s8, s16, s32})
     setAction({G_ADD, Ty}, Legal);
 
+  for (auto Op : {G_SEXT, G_ZEXT}) {
+    setAction({Op, s32}, Legal);
+    for (auto Ty : {s8, s16})
+      setAction({Op, 1, Ty}, Legal);
+  }
+
   computeTables();
 }

lib/Target/ARM/ARMRegisterBankInfo.cpp

 }
 
 const RegisterBank &ARMRegisterBankInfo::getRegBankFromRegClass(
     const TargetRegisterClass &RC) const {
   using namespace ARM;
 
   switch (RC.getID()) {
   case GPRRegClassID:
+  case GPRnopcRegClassID:
   case tGPR_and_tcGPRRegClassID:
     return getRegBank(ARM::GPRRegBankID);
   default:
     llvm_unreachable("Unsupported register kind");
   }
 
   llvm_unreachable("Switch should handle all register classes");
 }
ARMRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
   using namespace TargetOpcode;
 
   unsigned NumOperands = MI.getNumOperands();
   const ValueMapping *OperandsMapping = &ARM::ValueMappings[0];
 
   switch (Opc) {
   case G_ADD:
   case G_LOAD:
+  case G_SEXT:
+  case G_ZEXT:
     // 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=*/1, 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_sext_s8() { ret void }
+  define void @test_zext_s16() { ret void }
+
   define void @test_add_s8() { ret void }
   define void @test_add_s16() { ret void }
   define void @test_add_s32() { ret void }
 
   define void @test_load_from_stack() { ret void }
 ...
 ---
+---
+name:            test_sext_s8
+# CHECK-LABEL: name: test_sext_s8
+legalized:       true
+regBankSelected: true
+selected:        false
+# CHECK: selected: true
+registers:
+  - { id: 0, class: gprb }
+  - { id: 1, class: gprb }
+body:             |
+  bb.0:
+    liveins: %r0
+
+    %0(s8) = COPY %r0
+    ; CHECK: [[VREGX:%[0-9]+]] = COPY %r0
+
+    %1(s32) = G_SEXT %0(s8)
+    ; CHECK: [[VREGEXT:%[0-9]+]] = SXTB [[VREGX]], 0, 14, _
+
+    %r0 = COPY %1(s32)
+    ; CHECK: %r0 = COPY [[VREGEXT]]
+
+    BX_RET 14, _, implicit %r0
+    ; CHECK: BX_RET 14, _, implicit %r0
+...
+---
+name:            test_zext_s16
+# CHECK-LABEL: name: test_zext_s16
+legalized:       true
+regBankSelected: true
+selected:        false
+# CHECK: selected: true
+registers:
+  - { id: 0, class: gprb }
+  - { id: 1, class: gprb }
+body:             |
+  bb.0:
+    liveins: %r0
+
+    %0(s16) = COPY %r0
+    ; CHECK: [[VREGX:%[0-9]+]] = COPY %r0
+
+    %1(s32) = G_ZEXT %0(s16)
+    ; CHECK: [[VREGEXT:%[0-9]+]] = UXTH [[VREGX]], 0, 14, _
+
+    %r0 = COPY %1(s32)
+    ; CHECK: %r0 = COPY [[VREGEXT]]
+
+    BX_RET 14, _, implicit %r0
+    ; CHECK: BX_RET 14, _, implicit %r0
+...
+---
 name:            test_add_s8
 # CHECK-LABEL: name: test_add_s8
 legalized:       true
 regBankSelected: true
 selected:        false
 # CHECK: selected: true
 registers:
   - { id: 0, class: gprb }

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

 ; CHECK: [[SUM:%[0-9]+]](s8) = G_ADD [[VREGX]], [[VREGY]]
 ; CHECK: %r0 = COPY [[SUM]](s8)
 ; CHECK: BX_RET 14, _, implicit %r0
 entry:
   %sum = add i8 %x, %y
   ret i8 %sum
 }
 
+define signext i8 @test_return_sext_i8(i8 %x) {
+; CHECK-LABEL: name: test_return_sext_i8
+; CHECK: liveins: %r0
+; CHECK: [[VREG:%[0-9]+]](s8) = COPY %r0
+; CHECK: [[VREGEXT:%[0-9]+]](s32) = G_SEXT [[VREG]]
+; CHECK: %r0 = COPY [[VREGEXT]](s32)
+; CHECK: BX_RET 14, _, implicit %r0
+entry:
+  ret i8 %x
+}
+
 define i16 @test_add_i16(i16 %x, i16 %y) {
 ; CHECK-LABEL: name: test_add_i16
 ; CHECK: liveins: %r0, %r1
 ; CHECK-DAG: [[VREGX:%[0-9]+]](s16) = COPY %r0
 ; CHECK-DAG: [[VREGY:%[0-9]+]](s16) = COPY %r1
 ; CHECK: [[SUM:%[0-9]+]](s16) = G_ADD [[VREGX]], [[VREGY]]
 ; CHECK: %r0 = COPY [[SUM]](s16)
 ; CHECK: BX_RET 14, _, implicit %r0
 entry:
   %sum = add i16 %x, %y
   ret i16 %sum
 }
 
+define zeroext i16 @test_return_zext_i16(i16 %x) {
+; CHECK-LABEL: name: test_return_zext_i16
+; CHECK: liveins: %r0
+; CHECK: [[VREG:%[0-9]+]](s16) = COPY %r0
+; CHECK: [[VREGEXT:%[0-9]+]](s32) = G_ZEXT [[VREG]]
+; CHECK: %r0 = COPY [[VREGEXT]](s32)
+; CHECK: BX_RET 14, _, implicit %r0
+entry:
+  ret i16 %x
+}
+
 define i32 @test_add_i32(i32 %x, i32 %y) {
 ; CHECK-LABEL: name: test_add_i32
 ; CHECK: liveins: %r0, %r1
 ; CHECK-DAG: [[VREGX:%[0-9]+]](s32) = COPY %r0
 ; CHECK-DAG: [[VREGY:%[0-9]+]](s32) = COPY %r1
 ; CHECK: [[SUM:%[0-9]+]](s32) = G_ADD [[VREGX]], [[VREGY]]
 ; CHECK: %r0 = COPY [[SUM]](s32)
 ; CHECK: BX_RET 14, _, implicit %r0

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

 ; RUN: llc -mtriple arm-unknown -global-isel %s -o - | FileCheck %s
 
 define void @test_void_return() {
 ; CHECK-LABEL: test_void_return:
 ; CHECK: bx lr
 entry:
   ret void
 }
 
+define zeroext i8 @test_ext_i8(i8 %x) {
+; CHECK-LABEL: test_ext_i8:
+; CHECK: uxtb r0, r0
+; CHECK: bx lr
+entry:
+  ret i8 %x
+}
+
+define signext i16 @test_ext_i16(i16 %x) {
+; CHECK-LABEL: test_ext_i16:
+; CHECK: sxth r0, r0
+; CHECK: bx lr
+entry:
+  ret i16 %x
+}
+
 define i8 @test_add_i8(i8 %x, i8 %y) {
 ; CHECK-LABEL: test_add_i8:
 ; CHECK: add r0, r0, r1
 ; CHECK: bx lr
 entry:
   %sum = add i8 %x, %y
   ret i8 %sum
 }

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

 # RUN: llc -mtriple arm-- -global-isel -run-pass=legalizer %s -o - | FileCheck %s
 --- |
+  define void @test_sext_s8() { ret void }
+  define void @test_zext_s16() { ret void }
+
   define void @test_add_s8() { ret void }
   define void @test_add_s16() { ret void }
   define void @test_add_s32() { ret void }
 
   define void @test_load_from_stack() { ret void }
 ...
 ---
+name:            test_sext_s8
+# CHECK-LABEL: name: test_sext_s8
+legalized:       false
+# CHECK: legalized: true
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: _ }
+  - { id: 1, class: _ }
+body:             |
+  bb.0:
+    liveins: %r0
+
+    %0(s8) = COPY %r0
+    %1(s32) = G_SEXT %0
+    ; G_SEXT with s8 is legal, so we should find it unchanged in the output
+    ; CHECK: {{%[0-9]+}}(s32) = G_SEXT {{%[0-9]+}}
+    %r0 = COPY %1(s32)
+    BX_RET 14, _, implicit %r0
+...
+---
+name:            test_zext_s16
+# CHECK-LABEL: name: test_zext_s16
+legalized:       false
+# CHECK: legalized: true
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: _ }
+  - { id: 1, class: _ }
+body:             |
+  bb.0:
+    liveins: %r0
+
+    %0(s16) = COPY %r0
+    %1(s32) = G_ZEXT %0
+    ; G_ZEXT with s16 is legal, so we should find it unchanged in the output
+    ; CHECK: {{%[0-9]+}}(s32) = G_ZEXT {{%[0-9]+}}
+    %r0 = COPY %1(s32)
+    BX_RET 14, _, implicit %r0
+...
+---
 name:            test_add_s8
 # CHECK-LABEL: name: test_add_s8
 legalized:       false
 # CHECK: legalized: true
 regBankSelected: false
 selected:        false
 tracksRegLiveness: true
 registers: