Fix ARMv4 support

[joerg]

Summary

ARMv4 doesn't support the "BX" instruction, which has been introduced with ARMv4t. Adjust the call lowering and tail call implementation accordingly.

Further changes are necessary to ensure that presence of the v4t feature is correctly set. Most importantly, the "generic" CPU for thumb-* triples should include ARMv4t, since thumb mode without thumb support would naturally be pointless.

Add a couple of asserts to ensure thumb instructions are not emitted without CPU support.

[olista01]

lib/Target/ARM/ARMExpandPseudoInsts.cpp

1028 ↗

(On Diff #112238)

Could you add a test covering the ARM::TAILJMPr4 case?

[fhahn]

lib/Target/ARM/ARMAsmPrinter.cpp

1275 ↗

(On Diff #112238)

Maybe this assert would be clearer if the assert contained something like && "BX instructions are only available in ARMv4t and later". Applies to all added asserts in this patch

[fhahn]

I think this change makes sense. Looping in @rengolin as he may have some more insight

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

1912 ↗

(On Diff #112238)

I'm not sure if this assertion is really needed here. If the block does not already contain a BX instruction, we exit early.

[rengolin]

lib/Target/ARM/ARMAsmPrinter.cpp

1275 ↗

(On Diff #112238)

Well, both BX_CALL and BX_RET have Requires<[IsARM, HasV4T]>, which ARMv4 (in ARM.td) doesn't have.

So, either this BX_CALL is being generated manually (and incorrectly), or there's something wrong with the ISel.

I think it's safe to assume that, if a target has BX_CALL, it also has BX_RET, so the assert here should be redundant.

Your princess might be in another castle. :)

PS: It's possible that the introduction of getReturnOpcode() has fixed that already.

[t.p.northover]

I think the issues have been basically resolved here. The assertion can't hurt. LGTM!

[joerg]

lib/Target/ARM/ARMAsmPrinter.cpp

1275 ↗

(On Diff #112238)

Yes, it is pretty obvious from the instructions why the assert exists. The main motivation for the asserts is to ensure that all places where we create them by hand also have the feature around. Given that we missed a couple of them, I prefer to be safe here.

lib/Target/ARM/ARMExpandPseudoInsts.cpp

1028 ↗

(On Diff #112238)

armv4.ll tests the tail call.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

1912 ↗

(On Diff #112238)

I don't think it hurts either, though. It makes it easier to search for potentially missing places.

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

EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::MOVr)
       .addReg(ARM::LR)
       .addReg(ARM::PC)
       // Add predicate operands.
       .addImm(ARMCC::AL)
       .addReg(0)
       // Add 's' bit operand (always reg0 for this)
       .addReg(0));
 
+    assert(Subtarget->hasV4TOps());
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::BX)
       .addReg(MI->getOperand(0).getReg()));
     return;
   }
   case ARM::tBX_CALL: {
     if (Subtarget->hasV5TOps())
       llvm_unreachable("Expected BLX to be selected for v5t+");
 
case ARM::Int_eh_sjlj_longjmp: {
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::LDRi12)
       .addReg(ARM::R7)
       .addReg(SrcReg)
       .addImm(0)
       // Predicate.
       .addImm(ARMCC::AL)
       .addReg(0));
 
+    assert(Subtarget->hasV4TOps());
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::BX)
       .addReg(ScratchReg)
       // Predicate.
       .addImm(ARMCC::AL)
       .addReg(0));
     return;
   }
   case ARM::tInt_eh_sjlj_longjmp: {

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

bool ARMCallLowering::lowerReturnVal(MachineIRBuilder &MIRBuilder,
   OutgoingValueHandler RetHandler(MIRBuilder, MF.getRegInfo(), Ret, AssignFn);
   return handleAssignments(MIRBuilder, SplitVTs, RetHandler);
 }
 
 bool ARMCallLowering::lowerReturn(MachineIRBuilder &MIRBuilder,
                                   const Value *Val, unsigned VReg) const {
   assert(!Val == !VReg && "Return value without a vreg");
 
-  auto Ret = MIRBuilder.buildInstrNoInsert(ARM::BX_RET).add(predOps(ARMCC::AL));
+  auto const &ST = MIRBuilder.getMF().getSubtarget<ARMSubtarget>();
+  unsigned Opcode = ST.getReturnOpcode();
+  auto Ret = MIRBuilder.buildInstrNoInsert(Opcode).add(predOps(ARMCC::AL));
 
   if (!lowerReturnVal(MIRBuilder, Val, VReg, Ret))
     return false;
 
   MIRBuilder.insertInstr(Ret);
   return true;
 }
 

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

case ARM::TCRETURNri: {
           MIB.addExternalSymbol(JumpTarget.getSymbolName(),
                                 JumpTarget.getTargetFlags());
         }
 
         // Add the default predicate in Thumb mode.
         if (STI->isThumb())
           MIB.add(predOps(ARMCC::AL));
       } else if (RetOpcode == ARM::TCRETURNri) {
+        unsigned Opcode =
+          STI->isThumb() ? ARM::tTAILJMPr
+                         : (STI->hasV4TOps() ? ARM::TAILJMPr : ARM::TAILJMPr4);
         BuildMI(MBB, MBBI, dl,
-                TII.get(STI->isThumb() ? ARM::tTAILJMPr : ARM::TAILJMPr))
+                TII.get(Opcode))
             .addReg(JumpTarget.getReg(), RegState::Kill);
       }
 
       auto NewMI = std::prev(MBBI);
       for (unsigned i = 1, e = MBBI->getNumOperands(); i != e; ++i)
         NewMI->addOperand(MBBI->getOperand(i));
 
       // Delete the pseudo instruction TCRETURN.

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

bool ARMFastISel::SelectBranch(const Instruction *I) {
   return true;
 }
 
 bool ARMFastISel::SelectIndirectBr(const Instruction *I) {
   unsigned AddrReg = getRegForValue(I->getOperand(0));
   if (AddrReg == 0) return false;
 
   unsigned Opc = isThumb2 ? ARM::tBRIND : ARM::BX;
+  assert(isThumb2 || Subtarget->hasV4TOps());
+
   AddOptionalDefs(BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
                           TII.get(Opc)).addReg(AddrReg));
 
   const IndirectBrInst *IB = cast<IndirectBrInst>(I);
   for (const BasicBlock *SuccBB : IB->successors())
     FuncInfo.MBB->addSuccessor(FuncInfo.MBBMap[SuccBB]);
 
   return true;
if (!SrcRC->contains(DstReg))
       return false;
     BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
             TII.get(TargetOpcode::COPY), DstReg).addReg(SrcReg);
 
     // Add register to return instruction.
     RetRegs.push_back(VA.getLocReg());
   }
 
-  unsigned RetOpc = isThumb2 ? ARM::tBX_RET : ARM::BX_RET;
   MachineInstrBuilder MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
-                                    TII.get(RetOpc));
+                                    TII.get(Subtarget->getReturnOpcode()));
   AddOptionalDefs(MIB);
   for (unsigned R : RetRegs)
     MIB.addReg(R, RegState::Implicit);
   return true;
 }
 
 unsigned ARMFastISel::ARMSelectCallOp(bool UseReg) {
   if (UseReg)

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

BuildMI(AllocMBB, DL, TII.get(ARM::LDMIA_UPD))
         .addReg(ScratchReg1);
   }
 
   // Update the CFA offset now that we've popped
   CFIIndex = MF.addFrameInst(MCCFIInstruction::createDefCfaOffset(nullptr, 0));
   BuildMI(AllocMBB, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
       .addCFIIndex(CFIIndex);
 
-  // bx lr - Return from this function.
-  Opcode = Thumb ? ARM::tBX_RET : ARM::BX_RET;
-  BuildMI(AllocMBB, DL, TII.get(Opcode)).add(predOps(ARMCC::AL));
+  // Return from this function.
+  BuildMI(AllocMBB, DL, TII.get(ST->getReturnOpcode())).add(predOps(ARMCC::AL));
 
   // Restore SR0 and SR1 in case of __morestack() was not called.
   // pop {SR0, SR1}
   if (Thumb) {
     BuildMI(PostStackMBB, DL, TII.get(ARM::tPOP))
         .add(predOps(ARMCC::AL))
         .addReg(ScratchReg0)
         .addReg(ScratchReg1);

llvm/trunk/lib/Target/ARM/ARMInstrInfo.td

let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1, Uses = [SP] in {
   def TAILJMPd : ARMPseudoExpand<(outs), (ins arm_br_target:$dst),
                                  4, IIC_Br, [],
                                  (Bcc arm_br_target:$dst, (ops 14, zero_reg))>,
                                  Requires<[IsARM]>, Sched<[WriteBr]>;
 
   def TAILJMPr : ARMPseudoExpand<(outs), (ins tcGPR:$dst),
                                  4, IIC_Br, [],
                                  (BX GPR:$dst)>, Sched<[WriteBr]>,
-                                 Requires<[IsARM]>;
+                                 Requires<[IsARM, HasV4T]>;
 }
 
 // Secure Monitor Call is a system instruction.
 def SMC : ABI<0b0001, (outs), (ins imm0_15:$opt), NoItinerary, "smc", "\t$opt",
               []>, Requires<[IsARM, HasTrustZone]> {
   bits<4> opt;
   let Inst{23-4} = 0b01100000000000000111;
   let Inst{3-0} = opt;

 
 // ARMv4 indirect branch using (MOVr PC, dst)
 let isBranch = 1, isTerminator = 1, isBarrier = 1, isIndirectBranch = 1 in
   def MOVPCRX : ARMPseudoExpand<(outs), (ins GPR:$dst),
                     4, IIC_Br, [(brind GPR:$dst)],
                     (MOVr PC, GPR:$dst, (ops 14, zero_reg), zero_reg)>,
                   Requires<[IsARM, NoV4T]>, Sched<[WriteBr]>;
 
+let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1, Uses = [SP] in
+  def TAILJMPr4 : ARMPseudoExpand<(outs), (ins GPR:$dst),
+                    4, IIC_Br, [],
+                    (MOVr PC, GPR:$dst, (ops 14, zero_reg), zero_reg)>,
+                  Requires<[IsARM, NoV4T]>, Sched<[WriteBr]>;
+
 // Large immediate handling.
 
 // 32-bit immediate using two piece mod_imms or movw + movt.
 // This is a single pseudo instruction, the benefit is that it can be remat'd
 // as a single unit instead of having to handle reg inputs.
 // FIXME: Remove this when we can do generalized remat.
 let isReMaterializable = 1, isMoveImm = 1 in
 def MOVi32imm : PseudoInst<(outs GPR:$dst), (ins i32imm:$src), IIC_iMOVix2,

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

bool ARMLoadStoreOpt::CombineMovBx(MachineBasicBlock &MBB) {
 
   MachineBasicBlock::iterator Prev = MBBI;
   --Prev;
   if (Prev->getOpcode() != ARM::tMOVr || !Prev->definesRegister(ARM::LR))
     return false;
 
   for (auto Use : Prev->uses())
     if (Use.isKill()) {
+      assert(STI->hasV4TOps());
       BuildMI(MBB, MBBI, MBBI->getDebugLoc(), TII->get(ARM::tBX))
           .addReg(Use.getReg(), RegState::Kill)
           .add(predOps(ARMCC::AL))
           .copyImplicitOps(*MBBI);
       MBB.erase(MBBI);
       MBB.erase(Prev);
       return true;
     }

llvm/trunk/lib/Target/ARM/ARMSubtarget.h

int getPreISelOperandLatencyAdjustment() const {
     return PreISelOperandLatencyAdjustment;
   }
 
   /// True if the GV will be accessed via an indirect symbol.
   bool isGVIndirectSymbol(const GlobalValue *GV) const;
 
   /// True if fast-isel is used.
   bool useFastISel() const;
+
+  /// Returns the correct return opcode for the current feature set.
+  /// Use BX if available to allow mixing thumb/arm code, but fall back
+  /// to plain mov pc,lr on ARMv4.
+  unsigned getReturnOpcode() const {
+    if (isThumb())
+      return ARM::tBX_RET;
+    if (hasV4TOps())
+      return ARM::BX_RET;
+    return ARM::MOVPCLR;
+  }
 };
 
 } // end namespace llvm
 
 #endif  // LLVM_LIB_TARGET_ARM_ARMSUBTARGET_H

llvm/trunk/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

std::string ARM_MC::ParseARMTriple(const Triple &TT, StringRef CPU) {
   std::string ARMArchFeature;
 
   ARM::ArchKind ArchID = ARM::parseArch(TT.getArchName());
   if (ArchID != ARM::ArchKind::INVALID &&  (CPU.empty() || CPU == "generic"))
     ARMArchFeature = (ARMArchFeature + "+" + ARM::getArchName(ArchID)).str();
 
   if (TT.isThumb()) {
     if (ARMArchFeature.empty())
-      ARMArchFeature = "+thumb-mode";
+      ARMArchFeature = "+thumb-mode,+v4t";
     else
-      ARMArchFeature += ",+thumb-mode";
+      ARMArchFeature += ",+thumb-mode,+v4t";
   }
 
   if (TT.isOSNaCl()) {
     if (ARMArchFeature.empty())
       ARMArchFeature = "+nacl-trap";
     else
       ARMArchFeature += ",+nacl-trap";
   }

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

-; RUN: llc -mtriple arm-unknown -mattr=+vfp2 -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=LITTLE
-; RUN: llc -mtriple armeb-unknown -mattr=+vfp2 -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=BIG
+; RUN: llc -mtriple arm-unknown -mattr=+vfp2,+v4t -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=LITTLE
+; RUN: llc -mtriple armeb-unknown -mattr=+vfp2,+v4t -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - | FileCheck %s -check-prefix=CHECK -check-prefix=BIG
 
 define void @test_void_return() {
 ; CHECK-LABEL: name: test_void_return
 ; CHECK: BX_RET 14, _
 entry:
   ret void
 }
 

llvm/trunk/test/CodeGen/ARM/armv4.ll

 ; RUN: llc < %s -mtriple=armv4t-unknown-eabi | FileCheck %s -check-prefix=THUMB
 ; RUN: llc < %s -mtriple=armv4-unknown-eabi -mcpu=strongarm | FileCheck %s -check-prefix=ARM
 ; RUN: llc < %s -mtriple=armv7-unknown-eabi -mcpu=cortex-a8 | FileCheck %s -check-prefix=THUMB
 ; RUN: llc < %s -mtriple=armv6-unknown-eabi | FileCheck %s -check-prefix=THUMB
 ; RUN: llc < %s -mtriple=armv4-unknown-eabi | FileCheck %s -check-prefix=ARM
 ; RUN: llc < %s -mtriple=armv4t-unknown-eabi | FileCheck %s -check-prefix=THUMB
 
-define i32 @test(i32 %a) nounwind readnone {
+define i32 @test_return(i32 %a) nounwind readnone {
 entry:
+; ARM-LABEL: test_return
 ; ARM: mov pc
+; THUMB-LABEL: test_return
 ; THUMB: bx
   ret i32 %a
 }
+
+@helper = global i32 ()* null, align 4
+
+define i32 @test_indirect() #0 {
+entry:
+; ARM-LABEL: test_indirect
+; ARM: mov pc
+; THUMB-LABEL: test_indirect
+; THUMB: bx
+  %0 = load i32 ()*, i32 ()** @helper, align 4
+  %call = tail call i32 %0()
+  ret i32 %call
+}

llvm/trunk/test/CodeGen/ARM/debug-segmented-stacks.ll

-; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -verify-machineinstrs -filetype=asm | FileCheck %s -check-prefix=ARM-linux
-; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -filetype=obj
+; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -mattr=+v4t -verify-machineinstrs -filetype=asm | FileCheck %s -check-prefix=ARM-linux
+; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -mattr=+v4t -filetype=obj
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!9, !10}
 !llvm.ident = !{!11}
 
 define void @test_basic() #0 !dbg !4 {
         %mem = alloca i32, i32 10
         call void @dummy_use (i32* %mem, i32 10)

llvm/trunk/test/CodeGen/ARM/segmented-stacks-dynamic.ll

-; RUN: llc < %s -mtriple=arm-linux-androideabi -verify-machineinstrs | FileCheck %s -check-prefix=ARM-android
-; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -verify-machineinstrs | FileCheck %s -check-prefix=ARM-linux
-; RUN: llc < %s -mtriple=arm-linux-androideabi -filetype=obj
-; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -filetype=obj
+; RUN: llc < %s -mtriple=arm-linux-androideabi -mattr=+v4t -verify-machineinstrs | FileCheck %s -check-prefix=ARM-android
+; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -mattr=+v4t -verify-machineinstrs | FileCheck %s -check-prefix=ARM-linux
+; RUN: llc < %s -mtriple=arm-linux-androideabi -mattr=+v4t -filetype=obj
+; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -mattr=+v4t -filetype=obj
 
 ; Just to prevent the alloca from being optimized away
 declare void @dummy_use(i32*, i32)
 
 define i32 @test_basic(i32 %l) #0 {
         %mem = alloca i32, i32 %l
         call void @dummy_use (i32* %mem, i32 %l)
         %terminate = icmp eq i32 %l, 0

llvm/trunk/test/CodeGen/ARM/segmented-stacks.ll

-; RUN: llc < %s -mtriple=arm-linux-androideabi -verify-machineinstrs | FileCheck %s -check-prefix=ARM-android
-; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -verify-machineinstrs | FileCheck %s -check-prefix=ARM-linux
+; RUN: llc < %s -mtriple=arm-linux-androideabi -mattr=+v4t -verify-machineinstrs | FileCheck %s -check-prefix=ARM-android
+; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -mattr=+v4t  -verify-machineinstrs | FileCheck %s -check-prefix=ARM-linux
 
 ; We used to crash with filetype=obj
 ; RUN: llc < %s -mtriple=arm-linux-androideabi -filetype=obj
 ; RUN: llc < %s -mtriple=arm-linux-unknown-gnueabi -filetype=obj
 
 
 ; Just to prevent the alloca from being optimized away
 declare void @dummy_use(i32*, i32)