[ARM] Remove Subtarget from ARMAsmPrinter

[davemgreen]

Remove Subtarget uses from ARMAsmPrinter, making use of TargetMachine where applicable and getting the Subtarget from the MF where not. Some of the if() llvm_unreachable have been replaced by asserts.

[llvmbot]

@llvm/pr-subscribers-backend-arm

Author: David Green (davemgreen)

Changes

Remove Subtarget uses from ARMAsmPrinter, making use of TargetMachine where applicable and getting the Subtarget from the MF where not. Some of the if() llvm_unreachable have been replaced by asserts.

---

Full diff: https://github.com/llvm/llvm-project/pull/168264.diff

5 Files Affected:

• (modified) llvm/lib/Target/ARM/ARMAsmPrinter.cpp (+21-27)
• (modified) llvm/lib/Target/ARM/ARMAsmPrinter.h (+1-5)
• (modified) llvm/lib/Target/ARM/ARMMCInstLower.cpp (+2-2)
• (modified) llvm/lib/Target/ARM/ARMSubtarget.cpp (+2-8)
• (modified) llvm/lib/Target/ARM/ARMTargetMachine.h (+9)

diff --git a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
index 2d2e62c80c702..c545df2a7ade2 100644
--- a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
+++ b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
@@ -51,7 +51,7 @@ using namespace llvm;
 
 ARMAsmPrinter::ARMAsmPrinter(TargetMachine &TM,
                              std::unique_ptr<MCStreamer> Streamer)
-    : AsmPrinter(TM, std::move(Streamer), ID), Subtarget(nullptr), AFI(nullptr),
+    : AsmPrinter(TM, std::move(Streamer), ID), AFI(nullptr),
       MCP(nullptr), InConstantPool(false), OptimizationGoals(-1) {}
 
 const ARMBaseTargetMachine &ARMAsmPrinter::getTM() const {
@@ -116,7 +116,6 @@ void ARMAsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
 bool ARMAsmPrinter::runOnMachineFunction(MachineFunction &MF) {
   AFI = MF.getInfo<ARMFunctionInfo>();
   MCP = MF.getConstantPool();
-  Subtarget = &MF.getSubtarget<ARMSubtarget>();
 
   SetupMachineFunction(MF);
   const Function &F = MF.getFunction();
@@ -154,7 +153,7 @@ bool ARMAsmPrinter::runOnMachineFunction(MachineFunction &MF) {
   else if (OptimizationGoals != (int)OptimizationGoal) // conflicting goals
     OptimizationGoals = 0;
 
-  if (Subtarget->isTargetCOFF()) {
+  if (TM.getTargetTriple().isOSBinFormatCOFF()) {
     bool Local = F.hasLocalLinkage();
     COFF::SymbolStorageClass Scl =
         Local ? COFF::IMAGE_SYM_CLASS_STATIC : COFF::IMAGE_SYM_CLASS_EXTERNAL;
@@ -260,8 +259,8 @@ void ARMAsmPrinter::printOperand(const MachineInstr *MI, int OpNum,
     break;
   }
   case MachineOperand::MO_ConstantPoolIndex:
-    if (Subtarget->genExecuteOnly())
-      llvm_unreachable("execute-only should not generate constant pools");
+    assert(!MF->getSubtarget<ARMSubtarget>().genExecuteOnly() &&
+           "execute-only should not generate constant pools");
     GetCPISymbol(MO.getIndex())->print(O, MAI);
     break;
   }
@@ -680,14 +679,14 @@ void ARMAsmPrinter::emitAttributes() {
   if (isPositionIndependent()) {
     ATS.emitAttribute(ARMBuildAttrs::ABI_PCS_RW_data,
                       ARMBuildAttrs::AddressRWPCRel);
-  } else if (STI.isRWPI()) {
+  } else if (getTM().isRWPI()) {
     // RWPI specific attributes.
     ATS.emitAttribute(ARMBuildAttrs::ABI_PCS_RW_data,
                       ARMBuildAttrs::AddressRWSBRel);
   }
 
   // RO data addressing.
-  if (isPositionIndependent() || STI.isROPI()) {
+  if (isPositionIndependent() || getTM().isROPI()) {
     ATS.emitAttribute(ARMBuildAttrs::ABI_PCS_RO_data,
                       ARMBuildAttrs::AddressROPCRel);
   }
@@ -833,7 +832,7 @@ void ARMAsmPrinter::emitAttributes() {
   }
 
   // We currently do not support using R9 as the TLS pointer.
-  if (STI.isRWPI())
+  if (getTM().isRWPI())
     ATS.emitAttribute(ARMBuildAttrs::ABI_PCS_R9_use,
                       ARMBuildAttrs::R9IsSB);
   else if (STI.isR9Reserved())
@@ -1048,7 +1047,7 @@ void ARMAsmPrinter::emitJumpTableAddrs(const MachineInstr *MI) {
     //    .word (LBB1 - LJTI_0_0)
     const MCExpr *Expr = MCSymbolRefExpr::create(MBB->getSymbol(), OutContext);
 
-    if (isPositionIndependent() || Subtarget->isROPI())
+    if (isPositionIndependent() || getTM().isROPI())
       Expr = MCBinaryExpr::createSub(Expr, MCSymbolRefExpr::create(JTISymbol,
                                                                    OutContext),
                                      OutContext);
@@ -1097,7 +1096,8 @@ void ARMAsmPrinter::emitJumpTableTBInst(const MachineInstr *MI,
   const MachineOperand &MO1 = MI->getOperand(1);
   unsigned JTI = MO1.getIndex();
 
-  if (Subtarget->isThumb1Only())
+  const ARMSubtarget &STI = MF->getSubtarget<ARMSubtarget>();
+  if (STI.isThumb1Only())
     emitAlignment(Align(4));
 
   MCSymbol *JTISymbol = GetARMJTIPICJumpTableLabel(JTI);
@@ -1905,6 +1905,7 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
   ARM_MC::verifyInstructionPredicates(MI->getOpcode(),
                                       getSubtargetInfo().getFeatureBits());
 
+  const ARMSubtarget &STI = MF->getSubtarget<ARMSubtarget>();
   const DataLayout &DL = getDataLayout();
   MCTargetStreamer &TS = *OutStreamer->getTargetStreamer();
   ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
@@ -1916,7 +1917,7 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
   }
 
   // Emit unwinding stuff for frame-related instructions
-  if (Subtarget->isTargetEHABICompatible() &&
+  if (TM.getTargetTriple().isTargetEHABICompatible() &&
        MI->getFlag(MachineInstr::FrameSetup))
     EmitUnwindingInstruction(MI);
 
@@ -1983,14 +1984,13 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
       // Add 's' bit operand (always reg0 for this)
       .addReg(0));
 
-    assert(Subtarget->hasV4TOps());
-    EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::BX)
-      .addReg(MI->getOperand(0).getReg()));
+    assert(STI.hasV4TOps() && "Expected V4TOps for BX call");
+    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+");
+    assert(!STI.hasV5TOps() && "Expected BLX to be selected for v5t+");
 
     // On ARM v4t, when doing a call from thumb mode, we need to ensure
     // that the saved lr has its LSB set correctly (the arch doesn't
@@ -2279,8 +2279,8 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
     return;
   }
   case ARM::CONSTPOOL_ENTRY: {
-    if (Subtarget->genExecuteOnly())
-      llvm_unreachable("execute-only should not generate constant pools");
+    assert(!STI.genExecuteOnly() &&
+           "execute-only should not generate constant pools");
 
     /// CONSTPOOL_ENTRY - This instruction represents a floating constant pool
     /// in the function.  The first operand is the ID# for this instruction, the
@@ -2486,7 +2486,7 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
   case ARM::TRAP: {
     // Non-Darwin binutils don't yet support the "trap" mnemonic.
     // FIXME: Remove this special case when they do.
-    if (!Subtarget->isTargetMachO()) {
+    if (!TM.getTargetTriple().isOSBinFormatMachO()) {
       uint32_t Val = 0xe7ffdefeUL;
       OutStreamer->AddComment("trap");
       ATS.emitInst(Val);
@@ -2497,7 +2497,7 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
   case ARM::tTRAP: {
     // Non-Darwin binutils don't yet support the "trap" mnemonic.
     // FIXME: Remove this special case when they do.
-    if (!Subtarget->isTargetMachO()) {
+    if (!TM.getTargetTriple().isOSBinFormatMachO()) {
       uint16_t Val = 0xdefe;
       OutStreamer->AddComment("trap");
       ATS.emitInst(Val, 'n');
@@ -2657,9 +2657,6 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
       .addImm(ARMCC::AL)
       .addReg(0));
 
-    const MachineFunction &MF = *MI->getParent()->getParent();
-    const ARMSubtarget &STI = MF.getSubtarget<ARMSubtarget>();
-
     if (STI.isTargetDarwin() || STI.isTargetWindows()) {
       // These platforms always use the same frame register
       EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::LDRi12)
@@ -2688,7 +2685,7 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
         .addReg(0));
     }
 
-    assert(Subtarget->hasV4TOps());
+    assert(STI.hasV4TOps());
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::BX)
       .addReg(ScratchReg)
       // Predicate.
@@ -2705,9 +2702,6 @@ void ARMAsmPrinter::emitInstruction(const MachineInstr *MI) {
     Register SrcReg = MI->getOperand(0).getReg();
     Register ScratchReg = MI->getOperand(1).getReg();
 
-    const MachineFunction &MF = *MI->getParent()->getParent();
-    const ARMSubtarget &STI = MF.getSubtarget<ARMSubtarget>();
-
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tLDRi)
       .addReg(ScratchReg)
       .addReg(SrcReg)
diff --git a/llvm/lib/Target/ARM/ARMAsmPrinter.h b/llvm/lib/Target/ARM/ARMAsmPrinter.h
index 9e92b5a36a672..b9cd3c2613bc8 100644
--- a/llvm/lib/Target/ARM/ARMAsmPrinter.h
+++ b/llvm/lib/Target/ARM/ARMAsmPrinter.h
@@ -9,13 +9,13 @@
 #ifndef LLVM_LIB_TARGET_ARM_ARMASMPRINTER_H
 #define LLVM_LIB_TARGET_ARM_ARMASMPRINTER_H
 
-#include "ARMSubtarget.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/Target/TargetMachine.h"
 
 namespace llvm {
 
 class ARMFunctionInfo;
+class ARMBaseTargetMachine;
 class MCOperand;
 class MachineConstantPool;
 class MachineOperand;
@@ -33,10 +33,6 @@ class LLVM_LIBRARY_VISIBILITY ARMAsmPrinter : public AsmPrinter {
   static char ID;
 
 private:
-  /// Subtarget - Keep a pointer to the ARMSubtarget around so that we can
-  /// make the right decision when printing asm code for different targets.
-  const ARMSubtarget *Subtarget;
-
   /// AFI - Keep a pointer to ARMFunctionInfo for the current
   /// MachineFunction.
   ARMFunctionInfo *AFI;
diff --git a/llvm/lib/Target/ARM/ARMMCInstLower.cpp b/llvm/lib/Target/ARM/ARMMCInstLower.cpp
index f5d6597f214dd..c040904a82b71 100644
--- a/llvm/lib/Target/ARM/ARMMCInstLower.cpp
+++ b/llvm/lib/Target/ARM/ARMMCInstLower.cpp
@@ -112,8 +112,8 @@ bool ARMAsmPrinter::lowerOperand(const MachineOperand &MO,
     MCOp = GetSymbolRef(MO, GetJTISymbol(MO.getIndex()));
     break;
   case MachineOperand::MO_ConstantPoolIndex:
-    if (Subtarget->genExecuteOnly())
-      llvm_unreachable("execute-only should not generate constant pools");
+    assert(!MF->getSubtarget<ARMSubtarget>().genExecuteOnly() &&
+           "execute-only should not generate constant pools");
     MCOp = GetSymbolRef(MO, GetCPISymbol(MO.getIndex()));
     break;
   case MachineOperand::MO_BlockAddress:
diff --git a/llvm/lib/Target/ARM/ARMSubtarget.cpp b/llvm/lib/Target/ARM/ARMSubtarget.cpp
index 7ec232ae9bac5..05630e8583589 100644
--- a/llvm/lib/Target/ARM/ARMSubtarget.cpp
+++ b/llvm/lib/Target/ARM/ARMSubtarget.cpp
@@ -308,14 +308,8 @@ void ARMSubtarget::initSubtargetFeatures(StringRef CPU, StringRef FS) {
   }
 }
 
-bool ARMSubtarget::isROPI() const {
-  return TM.getRelocationModel() == Reloc::ROPI ||
-         TM.getRelocationModel() == Reloc::ROPI_RWPI;
-}
-bool ARMSubtarget::isRWPI() const {
-  return TM.getRelocationModel() == Reloc::RWPI ||
-         TM.getRelocationModel() == Reloc::ROPI_RWPI;
-}
+bool ARMSubtarget::isROPI() const { return TM.isROPI(); }
+bool ARMSubtarget::isRWPI() const { return TM.isRWPI(); }
 
 bool ARMSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
   return TM.isGVIndirectSymbol(GV);
diff --git a/llvm/lib/Target/ARM/ARMTargetMachine.h b/llvm/lib/Target/ARM/ARMTargetMachine.h
index 1f74e9fdd1dc9..d7b2e07a1fb35 100644
--- a/llvm/lib/Target/ARM/ARMTargetMachine.h
+++ b/llvm/lib/Target/ARM/ARMTargetMachine.h
@@ -86,6 +86,15 @@ class ARMBaseTargetMachine : public CodeGenTargetMachineImpl {
            TargetTriple.isOSWindows() || TargetABI == ARM::ARM_ABI_AAPCS16;
   }
 
+  bool isROPI() const {
+    return getRelocationModel() == Reloc::ROPI ||
+           getRelocationModel() == Reloc::ROPI_RWPI;
+  }
+  bool isRWPI() const {
+    return getRelocationModel() == Reloc::RWPI ||
+           getRelocationModel() == Reloc::ROPI_RWPI;
+  }
+
   bool targetSchedulesPostRAScheduling() const override { return true; };
 
   MachineFunctionInfo *

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[smithp35]

I've got a couple of questions surrounding ROPI and RWPI if these are used in LTO but the linker driver step doesn't include -fropi and/or -frwpi. Otherwise LGTM.

From a glance at the existing implementation this may never have worked as they use TM anyway. Due to these being niche embedded options it is possible no-one tried.

In general it is safe to mix -fropi and -fno-ropi (combination is -fno-ropi). The Arm Compiler C-libraries were compiled -fropi so they would be universally compatible.

Mixing -frwpi and -fno-rwpi was not something Arm Compiler permitted. It might be possile to make it work, but it was normally a mistake.

This may be something that we can mitigate with documentation. If you are using LTO and ropi/rwpi make sure to put the flags on the linker driver.

[smithp35]

Would this get set if the user had not put command-line flags on the link step of their LTO.

clang -flto -frwpi input.c
clang input.o (other linker driver flags but no c-flags like -frwpi)

Although looking at the existing ARMSubtarget::isROPI() it looks like that may not have worked beforehand.

[smithp35]

Similar for -fwrpi, we may need to get this from the MachineFunction for the LTO case.