Skip to content

[ARM] Add reference to ARMAsmParser in ARMOperand #86110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

AlfieRichardsArm
Copy link
Contributor

This commit adds a pointer to ARMAsmParser in ARMOperand. The reason to do this is there where multiple situations where sensible design choices were avoided because ARMOperand couldn't access information stored in ARMAsmParser. For instance, sometimes when calling addXXOperand on an ARMOperand information may be needed from ARMAsmParser regarding feature support and register classes.

There are two such cleanups in this patch.

This will also hopefully allow for future work cleaning up ARMAsmParser.

This also fixes #84577 permanently and addresses #83436 (comment)

@llvmbot
Copy link
Member

llvmbot commented Mar 21, 2024

@llvm/pr-subscribers-backend-arm

Author: Alfie Richards (AlfieRichardsArm)

Changes

This commit adds a pointer to ARMAsmParser in ARMOperand. The reason to do this is there where multiple situations where sensible design choices were avoided because ARMOperand couldn't access information stored in ARMAsmParser. For instance, sometimes when calling addXXOperand on an ARMOperand information may be needed from ARMAsmParser regarding feature support and register classes.

There are two such cleanups in this patch.

This will also hopefully allow for future work cleaning up ARMAsmParser.

This also fixes #84577 permanently and addresses #83436 (comment)


Patch is 55.76 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/86110.diff

1 Files Affected:

  • (modified) llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp (+258-236)
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index 9cfdb15a0f43d3..1b068e7ef9bba3 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -72,15 +72,6 @@
 
 using namespace llvm;
 
-namespace llvm {
-struct ARMInstrTable {
-  MCInstrDesc Insts[4445];
-  MCOperandInfo OperandInfo[3026];
-  MCPhysReg ImplicitOps[130];
-};
-extern const ARMInstrTable ARMDescs;
-} // end namespace llvm
-
 namespace {
 class ARMOperand;
 
@@ -360,11 +351,6 @@ class ARMAsmParser : public MCTargetAsmParser {
     ITState.CurPosition = ~0U;
   }
 
-  // Return the low-subreg of a given Q register.
-  unsigned getDRegFromQReg(unsigned QReg) const {
-    return MRI->getSubReg(QReg, ARM::dsub_0);
-  }
-
   // Get the condition code corresponding to the current IT block slot.
   ARMCC::CondCodes currentITCond() {
     unsigned MaskBit = extractITMaskBit(ITState.Mask, ITState.CurPosition);
@@ -586,9 +572,6 @@ class ARMAsmParser : public MCTargetAsmParser {
   bool hasV8_1MMainline() const {
     return getSTI().hasFeature(ARM::HasV8_1MMainlineOps);
   }
-  bool hasMVE() const {
-    return getSTI().hasFeature(ARM::HasMVEIntegerOps);
-  }
   bool hasMVEFloat() const {
     return getSTI().hasFeature(ARM::HasMVEFloatOps);
   }
@@ -768,6 +751,19 @@ class ARMAsmParser : public MCTargetAsmParser {
   void doBeforeLabelEmit(MCSymbol *Symbol, SMLoc IDLoc) override;
 
   void onLabelParsed(MCSymbol *Symbol) override;
+
+  const MCInstrDesc &getInstrDesc(unsigned int Opcode) const {
+    return MII.get(Opcode);
+  }
+
+  bool hasMVE() const { return getSTI().hasFeature(ARM::HasMVEIntegerOps); }
+
+  // Return the low-subreg of a given Q register.
+  unsigned getDRegFromQReg(unsigned QReg) const {
+    return MRI->getSubReg(QReg, ARM::dsub_0);
+  }
+
+  const MCRegisterInfo *getMRI() const { return MRI; }
 };
 
 /// ARMOperand - Instances of this class represent a parsed ARM machine
@@ -814,6 +810,8 @@ class ARMOperand : public MCParsedAsmOperand {
   SMLoc StartLoc, EndLoc, AlignmentLoc;
   SmallVector<unsigned, 8> Registers;
 
+  ARMAsmParser *Parser;
+
   struct CCOp {
     ARMCC::CondCodes Val;
   };
@@ -964,7 +962,7 @@ class ARMOperand : public MCParsedAsmOperand {
   };
 
 public:
-  ARMOperand(KindTy K) : Kind(K) {}
+  ARMOperand(KindTy K, ARMAsmParser &Parser) : Kind(K), Parser(&Parser) {}
 
   /// getStartLoc - Get the location of the first token of this operand.
   SMLoc getStartLoc() const override { return StartLoc; }
@@ -2043,6 +2041,11 @@ class ARMOperand : public MCParsedAsmOperand {
   bool isProcIFlags() const { return Kind == k_ProcIFlags; }
 
   // NEON operands.
+  bool isAnyVectorList() const {
+    return Kind == k_VectorList || Kind == k_VectorListAllLanes ||
+           Kind == k_VectorListIndexed;
+  }
+
   bool isVectorList() const { return Kind == k_VectorList; }
 
   bool isSingleSpacedVectorList() const {
@@ -2054,6 +2057,9 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   bool isVecListOneD() const {
+    // We convert a single D reg to a list containing a D reg
+    if (isDReg() && !Parser->hasMVE())
+      return true;
     if (!isSingleSpacedVectorList()) return false;
     return VectorList.Count == 1;
   }
@@ -2065,6 +2071,10 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   bool isVecListDPair() const {
+    // We convert a single Q reg to a list with the two corresponding D
+    // registers
+    if (isQReg() && !Parser->hasMVE())
+      return true;
     if (!isSingleSpacedVectorList()) return false;
     return (ARMMCRegisterClasses[ARM::DPairRegClassID]
               .contains(VectorList.RegNum));
@@ -2542,8 +2552,7 @@ class ARMOperand : public MCParsedAsmOperand {
       RegNum = 0;
     } else {
       unsigned NextOpIndex = Inst.getNumOperands();
-      const MCInstrDesc &MCID =
-          ARMDescs.Insts[ARM::INSTRUCTION_LIST_END - 1 - Inst.getOpcode()];
+      auto &MCID = Parser->getInstrDesc(Inst.getOpcode());
       int TiedOp = MCID.getOperandConstraint(NextOpIndex, MCOI::TIED_TO);
       assert(TiedOp >= 0 &&
              "Inactive register in vpred_r is not tied to an output!");
@@ -3378,7 +3387,21 @@ class ARMOperand : public MCParsedAsmOperand {
 
   void addVecListOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
-    Inst.addOperand(MCOperand::createReg(VectorList.RegNum));
+
+    if (isAnyVectorList())
+      Inst.addOperand(MCOperand::createReg(VectorList.RegNum));
+    else if (isDReg() && !Parser->hasMVE()) {
+      Inst.addOperand(MCOperand::createReg(Reg.RegNum));
+    } else if (isQReg() && !Parser->hasMVE()) {
+      auto DPair = Parser->getDRegFromQReg(Reg.RegNum);
+      DPair = Parser->getMRI()->getMatchingSuperReg(
+          DPair, ARM::dsub_0, &ARMMCRegisterClasses[ARM::DPairRegClassID]);
+      Inst.addOperand(MCOperand::createReg(DPair));
+    } else {
+      LLVM_DEBUG(dbgs() << "TYPE: " << Kind << "\n");
+      llvm_unreachable(
+          "attempted to add a vector list register with wrong type!");
+    }
   }
 
   void addMVEVecListOperands(MCInst &Inst, unsigned N) const {
@@ -3607,67 +3630,72 @@ class ARMOperand : public MCParsedAsmOperand {
 
   void print(raw_ostream &OS) const override;
 
-  static std::unique_ptr<ARMOperand> CreateITMask(unsigned Mask, SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_ITCondMask);
+  static std::unique_ptr<ARMOperand> CreateITMask(unsigned Mask, SMLoc S,
+                                                  ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_ITCondMask, Parser);
     Op->ITMask.Mask = Mask;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateCondCode(ARMCC::CondCodes CC,
-                                                    SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_CondCode);
+  static std::unique_ptr<ARMOperand>
+  CreateCondCode(ARMCC::CondCodes CC, SMLoc S, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_CondCode, Parser);
     Op->CC.Val = CC;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateVPTPred(ARMVCC::VPTCodes CC,
-                                                   SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_VPTPred);
+  static std::unique_ptr<ARMOperand> CreateVPTPred(ARMVCC::VPTCodes CC, SMLoc S,
+                                                   ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_VPTPred, Parser);
     Op->VCC.Val = CC;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateCoprocNum(unsigned CopVal, SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_CoprocNum);
+  static std::unique_ptr<ARMOperand> CreateCoprocNum(unsigned CopVal, SMLoc S,
+                                                     ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_CoprocNum, Parser);
     Op->Cop.Val = CopVal;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateCoprocReg(unsigned CopVal, SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_CoprocReg);
+  static std::unique_ptr<ARMOperand> CreateCoprocReg(unsigned CopVal, SMLoc S,
+                                                     ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_CoprocReg, Parser);
     Op->Cop.Val = CopVal;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateCoprocOption(unsigned Val, SMLoc S,
-                                                        SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_CoprocOption);
+  static std::unique_ptr<ARMOperand>
+  CreateCoprocOption(unsigned Val, SMLoc S, SMLoc E, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_CoprocOption, Parser);
     Op->Cop.Val = Val;
     Op->StartLoc = S;
     Op->EndLoc = E;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateCCOut(unsigned RegNum, SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_CCOut);
+  static std::unique_ptr<ARMOperand> CreateCCOut(unsigned RegNum, SMLoc S,
+                                                 ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_CCOut, Parser);
     Op->Reg.RegNum = RegNum;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateToken(StringRef Str, SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_Token);
+  static std::unique_ptr<ARMOperand> CreateToken(StringRef Str, SMLoc S,
+                                                 ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_Token, Parser);
     Op->Tok.Data = Str.data();
     Op->Tok.Length = Str.size();
     Op->StartLoc = S;
@@ -3676,8 +3704,8 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   static std::unique_ptr<ARMOperand> CreateReg(unsigned RegNum, SMLoc S,
-                                               SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_Register);
+                                               SMLoc E, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_Register, Parser);
     Op->Reg.RegNum = RegNum;
     Op->StartLoc = S;
     Op->EndLoc = E;
@@ -3686,9 +3714,9 @@ class ARMOperand : public MCParsedAsmOperand {
 
   static std::unique_ptr<ARMOperand>
   CreateShiftedRegister(ARM_AM::ShiftOpc ShTy, unsigned SrcReg,
-                        unsigned ShiftReg, unsigned ShiftImm, SMLoc S,
-                        SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_ShiftedRegister);
+                        unsigned ShiftReg, unsigned ShiftImm, SMLoc S, SMLoc E,
+                        ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_ShiftedRegister, Parser);
     Op->RegShiftedReg.ShiftTy = ShTy;
     Op->RegShiftedReg.SrcReg = SrcReg;
     Op->RegShiftedReg.ShiftReg = ShiftReg;
@@ -3700,8 +3728,9 @@ class ARMOperand : public MCParsedAsmOperand {
 
   static std::unique_ptr<ARMOperand>
   CreateShiftedImmediate(ARM_AM::ShiftOpc ShTy, unsigned SrcReg,
-                         unsigned ShiftImm, SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_ShiftedImmediate);
+                         unsigned ShiftImm, SMLoc S, SMLoc E,
+                         ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_ShiftedImmediate, Parser);
     Op->RegShiftedImm.ShiftTy = ShTy;
     Op->RegShiftedImm.SrcReg = SrcReg;
     Op->RegShiftedImm.ShiftImm = ShiftImm;
@@ -3711,8 +3740,9 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   static std::unique_ptr<ARMOperand> CreateShifterImm(bool isASR, unsigned Imm,
-                                                      SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_ShifterImmediate);
+                                                      SMLoc S, SMLoc E,
+                                                      ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_ShifterImmediate, Parser);
     Op->ShifterImm.isASR = isASR;
     Op->ShifterImm.Imm = Imm;
     Op->StartLoc = S;
@@ -3720,9 +3750,9 @@ class ARMOperand : public MCParsedAsmOperand {
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateRotImm(unsigned Imm, SMLoc S,
-                                                  SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_RotateImmediate);
+  static std::unique_ptr<ARMOperand>
+  CreateRotImm(unsigned Imm, SMLoc S, SMLoc E, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_RotateImmediate, Parser);
     Op->RotImm.Imm = Imm;
     Op->StartLoc = S;
     Op->EndLoc = E;
@@ -3730,8 +3760,9 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   static std::unique_ptr<ARMOperand> CreateModImm(unsigned Bits, unsigned Rot,
-                                                  SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_ModifiedImmediate);
+                                                  SMLoc S, SMLoc E,
+                                                  ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_ModifiedImmediate, Parser);
     Op->ModImm.Bits = Bits;
     Op->ModImm.Rot = Rot;
     Op->StartLoc = S;
@@ -3740,17 +3771,20 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   static std::unique_ptr<ARMOperand>
-  CreateConstantPoolImm(const MCExpr *Val, SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_ConstantPoolImmediate);
+  CreateConstantPoolImm(const MCExpr *Val, SMLoc S, SMLoc E,
+                        ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_ConstantPoolImmediate, Parser);
     Op->Imm.Val = Val;
     Op->StartLoc = S;
     Op->EndLoc = E;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand>
-  CreateBitfield(unsigned LSB, unsigned Width, SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_BitfieldDescriptor);
+  static std::unique_ptr<ARMOperand> CreateBitfield(unsigned LSB,
+                                                    unsigned Width, SMLoc S,
+                                                    SMLoc E,
+                                                    ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_BitfieldDescriptor, Parser);
     Op->Bitfield.LSB = LSB;
     Op->Bitfield.Width = Width;
     Op->StartLoc = S;
@@ -3760,7 +3794,7 @@ class ARMOperand : public MCParsedAsmOperand {
 
   static std::unique_ptr<ARMOperand>
   CreateRegList(SmallVectorImpl<std::pair<unsigned, unsigned>> &Regs,
-                SMLoc StartLoc, SMLoc EndLoc) {
+                SMLoc StartLoc, SMLoc EndLoc, ARMAsmParser &Parser) {
     assert(Regs.size() > 0 && "RegList contains no registers?");
     KindTy Kind = k_RegisterList;
 
@@ -3783,7 +3817,7 @@ class ARMOperand : public MCParsedAsmOperand {
 
     assert(llvm::is_sorted(Regs) && "Register list must be sorted by encoding");
 
-    auto Op = std::make_unique<ARMOperand>(Kind);
+    auto Op = std::make_unique<ARMOperand>(Kind, Parser);
     for (const auto &P : Regs)
       Op->Registers.push_back(P.second);
 
@@ -3792,11 +3826,10 @@ class ARMOperand : public MCParsedAsmOperand {
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateVectorList(unsigned RegNum,
-                                                      unsigned Count,
-                                                      bool isDoubleSpaced,
-                                                      SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_VectorList);
+  static std::unique_ptr<ARMOperand>
+  CreateVectorList(unsigned RegNum, unsigned Count, bool isDoubleSpaced,
+                   SMLoc S, SMLoc E, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_VectorList, Parser);
     Op->VectorList.RegNum = RegNum;
     Op->VectorList.Count = Count;
     Op->VectorList.isDoubleSpaced = isDoubleSpaced;
@@ -3807,8 +3840,8 @@ class ARMOperand : public MCParsedAsmOperand {
 
   static std::unique_ptr<ARMOperand>
   CreateVectorListAllLanes(unsigned RegNum, unsigned Count, bool isDoubleSpaced,
-                           SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_VectorListAllLanes);
+                           SMLoc S, SMLoc E, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_VectorListAllLanes, Parser);
     Op->VectorList.RegNum = RegNum;
     Op->VectorList.Count = Count;
     Op->VectorList.isDoubleSpaced = isDoubleSpaced;
@@ -3819,8 +3852,9 @@ class ARMOperand : public MCParsedAsmOperand {
 
   static std::unique_ptr<ARMOperand>
   CreateVectorListIndexed(unsigned RegNum, unsigned Count, unsigned Index,
-                          bool isDoubleSpaced, SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_VectorListIndexed);
+                          bool isDoubleSpaced, SMLoc S, SMLoc E,
+                          ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_VectorListIndexed, Parser);
     Op->VectorList.RegNum = RegNum;
     Op->VectorList.Count = Count;
     Op->VectorList.LaneIndex = Index;
@@ -3830,9 +3864,10 @@ class ARMOperand : public MCParsedAsmOperand {
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand>
-  CreateVectorIndex(unsigned Idx, SMLoc S, SMLoc E, MCContext &Ctx) {
-    auto Op = std::make_unique<ARMOperand>(k_VectorIndex);
+  static std::unique_ptr<ARMOperand> CreateVectorIndex(unsigned Idx, SMLoc S,
+                                                       SMLoc E, MCContext &Ctx,
+                                                       ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_VectorIndex, Parser);
     Op->VectorIndex.Val = Idx;
     Op->StartLoc = S;
     Op->EndLoc = E;
@@ -3840,8 +3875,8 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   static std::unique_ptr<ARMOperand> CreateImm(const MCExpr *Val, SMLoc S,
-                                               SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_Immediate);
+                                               SMLoc E, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_Immediate, Parser);
     Op->Imm.Val = Val;
     Op->StartLoc = S;
     Op->EndLoc = E;
@@ -3851,8 +3886,9 @@ class ARMOperand : public MCParsedAsmOperand {
   static std::unique_ptr<ARMOperand>
   CreateMem(unsigned BaseRegNum, const MCExpr *OffsetImm, unsigned OffsetRegNum,
             ARM_AM::ShiftOpc ShiftType, unsigned ShiftImm, unsigned Alignment,
-            bool isNegative, SMLoc S, SMLoc E, SMLoc AlignmentLoc = SMLoc()) {
-    auto Op = std::make_unique<ARMOperand>(k_Memory);
+            bool isNegative, SMLoc S, SMLoc E, ARMAsmParser &Parser,
+            SMLoc AlignmentLoc = SMLoc()) {
+    auto Op = std::make_unique<ARMOperand>(k_Memory, Parser);
     Op->Memory.BaseRegNum = BaseRegNum;
     Op->Memory.OffsetImm = OffsetImm;
     Op->Memory.OffsetRegNum = OffsetRegNum;
@@ -3868,8 +3904,8 @@ class ARMOperand : public MCParsedAsmOperand {
 
   static std::unique_ptr<ARMOperand>
   CreatePostIdxReg(unsigned RegNum, bool isAdd, ARM_AM::ShiftOpc ShiftTy,
-                   unsigned ShiftImm, SMLoc S, SMLoc E) {
-    auto Op = std::make_unique<ARMOperand>(k_PostIndexRegister);
+                   unsigned ShiftImm, SMLoc S, SMLoc E, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_PostIndexRegister, Parser);
     Op->PostIdxReg.RegNum = RegNum;
     Op->PostIdxReg.isAdd = isAdd;
     Op->PostIdxReg.ShiftTy = ShiftTy;
@@ -3879,9 +3915,9 @@ class ARMOperand : public MCParsedAsmOperand {
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> CreateMemBarrierOpt(ARM_MB::MemBOpt Opt,
-                                                         SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_MemBarrierOpt);
+  static std::unique_ptr<ARMOperand>
+  CreateMemBarrierOpt(ARM_MB::MemBOpt Opt, SMLoc S, ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_MemBarrierOpt, Parser);
     Op->MBOpt.Val = Opt;
     Op->StartLoc = S;
     Op->EndLoc = S;
@@ -3889,8 +3925,9 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   static std::unique_ptr<ARMOperand>
-  CreateInstSyncBarrierOpt(ARM_ISB::InstSyncBOpt Opt, SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_InstSyncBarrierOpt);
+  CreateInstSyncBarrierOpt(ARM_ISB::InstSyncBOpt Opt, SMLoc S,
+                           ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_InstSyncBarrierOpt, Parser);
     Op->ISBOpt.Val = Opt;
     Op->StartLoc = S;
     Op->EndLoc = S;
@@ -3898,33 +3935,36 @@ class ARMOperand : public MCParsedAsmOperand {
   }
 
   static std::unique_ptr<ARMOperand>
-  CreateTraceSyncBarrierOpt(ARM_TSB::TraceSyncBOpt Opt, SMLoc S) {
-    auto Op = std::make_unique<ARMOperand>(k_TraceSyncBarrierOpt);
+  CreateTraceSyncBarrierOpt(ARM_TSB::TraceSyncBOpt Opt, SMLoc S,
+                            ARMAsmParser &Parser) {
+    auto Op = std::make_unique<ARMOperand>(k_TraceSyncBarrierOpt, Parser);
     Op->TSBOpt.Val = Opt;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
-  static std::unique_ptr<ARMOperand> Create...
[truncated]

Copy link

github-actions bot commented Mar 27, 2024

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

Comment on lines -75 to -83
namespace llvm {
struct ARMInstrTable {
MCInstrDesc Insts[4445];
MCOperandInfo OperandInfo[3026];
MCPhysReg ImplicitOps[130];
};
extern const ARMInstrTable ARMDescs;
} // end namespace llvm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a pleasure to remove it! :)

This commit adds a pointer to ARMAsmParser to ARMOperand.
The reason to do this is there where multiple situations where
semsible design choices were avoided because ARMOperand couldn't
access information stored in ARMAsmParser. For instance, sometimes
when calling `addXXOperand` on an ARMOperand information may be
needed from ARMAsmParser regarding feature support and register
classes.

There are two such cleanups in this patch.
This will also hopefully allow for future work cleaning up ARMAsmParser.
@AlfieRichardsArm AlfieRichardsArm force-pushed the arm-operand-arm-parser-reference branch from 29d8064 to dcfaa68 Compare March 28, 2024 11:19
@@ -13003,29 +13047,6 @@ unsigned ARMAsmParser::validateTargetOperandClass(MCParsedAsmOperand &AsmOp,
if (hasV8Ops() && Op.isReg() && Op.getReg() == ARM::SP)
return Match_Success;
return Match_rGPR;
// Note: This mutates the operand which could cause issues for future
// matches if this one fails later.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuck! Thanks for removing this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly I am responsible for this so can't take too much credit

auto DPair = Parser->getDRegFromQReg(Reg.RegNum);
DPair = Parser->getMRI()->getMatchingSuperReg(
DPair, ARM::dsub_0, &ARMMCRegisterClasses[ARM::DPairRegClassID]);
Inst.addOperand(MCOperand::createReg(DPair));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit that I haven't fully followed the logic of how these various cases match up to the code you've removed from the matcher. But structurally it's surely an improvement to avoid that uncontrolled mutation, and if this gets through the full test suite then I'm happy!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this code #83436 (comment) is somewhat helpful context if you are curious

@AlfieRichardsArm AlfieRichardsArm merged commit ff870ae into llvm:main Mar 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants