Skip to content
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

Add diagnostic help for inline asm operand constraint 'H' #88248

Closed
wants to merge 3 commits into from

Conversation

mahesh-attarde
Copy link
Contributor

Using inline asm operand constraint 'H' with constants does not reflect helpful error.
With addition of this message operands those are not offsetable, will report exact root cause.
Originally https://reviews.llvm.org/D35890

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 10, 2024

@llvm/pr-subscribers-backend-loongarch
@llvm/pr-subscribers-backend-webassembly
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-backend-powerpc
@llvm/pr-subscribers-backend-sparc
@llvm/pr-subscribers-backend-msp430

@llvm/pr-subscribers-backend-arm

Author: None (mahesh-attarde)

Changes

Using inline asm operand constraint 'H' with constants does not reflect helpful error.
With addition of this message operands those are not offsetable, will report exact root cause.
Originally https://reviews.llvm.org/D35890


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

37 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/AsmPrinter.h (+9-3)
  • (modified) llvm/include/llvm/IR/InlineAsm.h (+15-1)
  • (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp (+33-21)
  • (modified) llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp (+30-18)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp (+12-9)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/ARM/ARMAsmPrinter.cpp (+32-29)
  • (modified) llvm/lib/Target/ARM/ARMAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/AVR/AVRAsmPrinter.cpp (+15-11)
  • (modified) llvm/lib/Target/BPF/BPFAsmPrinter.cpp (+8-5)
  • (modified) llvm/lib/Target/CSKY/CSKYAsmPrinter.cpp (+7-4)
  • (modified) llvm/lib/Target/CSKY/CSKYAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/Hexagon/HexagonAsmPrinter.cpp (+9-8)
  • (modified) llvm/lib/Target/Hexagon/HexagonAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/Lanai/LanaiAsmPrinter.cpp (+15-12)
  • (modified) llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp (+16-14)
  • (modified) llvm/lib/Target/LoongArch/LoongArchAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/M68k/M68kAsmPrinter.cpp (+5-3)
  • (modified) llvm/lib/Target/M68k/M68kAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp (+8-5)
  • (modified) llvm/lib/Target/Mips/MipsAsmPrinter.cpp (+26-23)
  • (modified) llvm/lib/Target/Mips/MipsAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp (+6-4)
  • (modified) llvm/lib/Target/NVPTX/NVPTXAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp (+14-10)
  • (modified) llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp (+19-15)
  • (modified) llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp (+11-6)
  • (modified) llvm/lib/Target/Sparc/SparcAsmPrinter.cpp (+33-16)
  • (modified) llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp (+5-4)
  • (modified) llvm/lib/Target/SystemZ/SystemZAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/VE/VEAsmPrinter.cpp (+9-6)
  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp (+12-12)
  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/X86/X86AsmPrinter.cpp (+42-19)
  • (modified) llvm/lib/Target/X86/X86AsmPrinter.h (+7-2)
  • (modified) llvm/lib/Target/XCore/XCoreAsmPrinter.cpp (+8-5)
  • (modified) llvm/test/CodeGen/SPARC/inlineasm-bad.ll (+14-1)
diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h
index 81c3e4be95e9ff..3c29b0133c5fcf 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinter.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinter.h
@@ -817,9 +817,15 @@ class AsmPrinter : public MachineFunctionPass {
   /// Print the specified operand of MI, an INLINEASM instruction, using the
   /// specified assembler variant.  Targets should override this to format as
   /// appropriate.  This method can return true if the operand is erroneous.
-  virtual bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                               const char *ExtraCode, raw_ostream &OS);
-
+  virtual AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI,
+                                              unsigned OpNo,
+                                              const char *ExtraCode,
+                                              raw_ostream &OS);
+
+  /// Print Operand Constraint Error related helpful message
+  virtual void diagnoseAsmOperandError(LLVMContext &C,
+                                       const AsmOperandErrorCode,
+                                       const char *AsmStr, uint64_t Loc);
   /// Print the specified operand of MI, an INLINEASM instruction, using the
   /// specified assembler variant as an address. Targets should override this to
   /// format as appropriate.  This method can return true if the operand is
diff --git a/llvm/include/llvm/IR/InlineAsm.h b/llvm/include/llvm/IR/InlineAsm.h
index e5f506e5694daf..989bbe47006219 100644
--- a/llvm/include/llvm/IR/InlineAsm.h
+++ b/llvm/include/llvm/IR/InlineAsm.h
@@ -528,6 +528,20 @@ class InlineAsm final : public Value {
   }
 };
 
+/// Inline Asm specifies input & output constraint which can
+/// specifiy target specific criteria for operand. If this criteria
+/// does not match, we must throw error.
+/// NO_ERROR represents Operand constraints are valid/applicable
+/// OPERAND_ERROR represents some constraint(unspecified) failed
+/// UNKNOWN_MODIFIER_ERROR represents use of unknown char constraint
+/// CONSTRAINT_<char>_ERROR represents error regarding constraint.
+enum class AsmOperandErrorCode {
+  NO_ERROR = 0,
+  OPERAND_ERROR,
+  UNKNOWN_MODIFIER_ERROR,
+  CONSTRAINT_H_ERROR,
+};
+
 } // end namespace llvm
 
-#endif // LLVM_IR_INLINEASM_H
+#endif // LLVM_IR_INLINEASM_H
\ No newline at end of file
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
index d0ef3e5a19391c..2aa7347ffc3ff6 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
@@ -273,7 +273,7 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
         unsigned OpNo = InlineAsm::MIOp_FirstOperand;
 
         bool Error = false;
-
+        AsmOperandErrorCode OpErrorCode = AsmOperandErrorCode::NO_ERROR;
         // Scan to find the machine operand number for the operand.
         for (; Val; --Val) {
           if (OpNo >= MI->getNumOperands())
@@ -306,15 +306,15 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
             Error = AP->PrintAsmMemoryOperand(
                 MI, OpNo, Modifier[0] ? Modifier : nullptr, OS);
           } else {
-            Error = AP->PrintAsmOperand(MI, OpNo,
-                                        Modifier[0] ? Modifier : nullptr, OS);
+            OpErrorCode = AP->PrintAsmOperand(
+                MI, OpNo, Modifier[0] ? Modifier : nullptr, OS);
           }
         }
-        if (Error) {
-          std::string msg;
-          raw_string_ostream Msg(msg);
-          Msg << "invalid operand in inline asm: '" << AsmStr << "'";
-          MMI->getModule()->getContext().emitError(LocCookie, Msg.str());
+        if (Error || (OpErrorCode != AsmOperandErrorCode::NO_ERROR)) {
+          if (Error)
+            OpErrorCode = AsmOperandErrorCode::OPERAND_ERROR;
+          AP->diagnoseAsmOperandError(MMI->getModule()->getContext(),
+                                      OpErrorCode, AsmStr, LocCookie);
         }
       }
       break;
@@ -461,50 +461,62 @@ void AsmPrinter::PrintSymbolOperand(const MachineOperand &MO, raw_ostream &OS) {
   printOffset(MO.getOffset(), OS);
 }
 
+void AsmPrinter::diagnoseAsmOperandError(LLVMContext &C,
+                                         const AsmOperandErrorCode ErrCode,
+                                         const char *AsmStr,
+                                         const uint64_t Loc) {
+  std::string msg;
+  raw_string_ostream Msg(msg);
+  Msg << "invalid operand in inline asm: '" << AsmStr << "'";
+  C.emitError(Loc, Msg.str());
+}
 /// PrintAsmOperand - Print the specified operand of MI, an INLINEASM
 /// instruction, using the specified assembler variant.  Targets should
 /// override this to format as appropriate for machine specific ExtraCodes
 /// or when the arch-independent handling would be too complex otherwise.
-bool AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                                 const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                unsigned OpNo,
+                                                const char *ExtraCode,
+                                                raw_ostream &O) {
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
-    if (ExtraCode[1] != 0) return true; // Unknown modifier.
+    if (ExtraCode[1] != 0)
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     // https://gcc.gnu.org/onlinedocs/gccint/Output-Template.html
     const MachineOperand &MO = MI->getOperand(OpNo);
     switch (ExtraCode[0]) {
     default:
-      return true;  // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
     case 'a': // Print as memory address.
       if (MO.isReg()) {
         PrintAsmMemoryOperand(MI, OpNo, nullptr, O);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       [[fallthrough]]; // GCC allows '%a' to behave like '%c' with immediates.
     case 'c': // Substitute immediate value without immediate syntax
       if (MO.isImm()) {
         O << MO.getImm();
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       if (MO.isGlobal()) {
         PrintSymbolOperand(MO, O);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     case 'n':  // Negate the immediate constant.
       if (!MO.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << -MO.getImm();
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 's':  // The GCC deprecated s modifier
       if (!MO.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << ((32 - MO.getImm()) & 31);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
   }
-  return true;
+  return AsmOperandErrorCode::OPERAND_ERROR;
 }
 
 bool AsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
diff --git a/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp b/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
index f6ccd0ecfdc893..cdc54ca545be88 100644
--- a/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ b/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -176,8 +176,9 @@ class AArch64AsmPrinter : public AsmPrinter {
                           const TargetRegisterClass *RC, unsigned AltName,
                           raw_ostream &O);
 
-  bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                       const char *ExtraCode, raw_ostream &O) override;
+  AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
+                                      const char *ExtraCode,
+                                      raw_ostream &O) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNum,
                              const char *ExtraCode, raw_ostream &O) override;
 
@@ -915,33 +916,38 @@ bool AArch64AsmPrinter::printAsmRegInClass(const MachineOperand &MO,
   return false;
 }
 
-bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                                        const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                       unsigned OpNum,
+                                                       const char *ExtraCode,
+                                                       raw_ostream &O) {
   const MachineOperand &MO = MI->getOperand(OpNum);
 
   // First try the generic code, which knows about modifiers like 'c' and 'n'.
-  if (!AsmPrinter::PrintAsmOperand(MI, OpNum, ExtraCode, O))
-    return false;
+  if (AsmPrinter::PrintAsmOperand(MI, OpNum, ExtraCode, O) ==
+      AsmOperandErrorCode::NO_ERROR)
+    return AsmOperandErrorCode::NO_ERROR;
 
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
     if (ExtraCode[1] != 0)
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     default:
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
     case 'w':      // Print W register
     case 'x':      // Print X register
       if (MO.isReg())
-        return printAsmMRegister(MO, ExtraCode[0], O);
+        return (printAsmMRegister(MO, ExtraCode[0], O)
+                    ? AsmOperandErrorCode::OPERAND_ERROR
+                    : AsmOperandErrorCode::NO_ERROR);
       if (MO.isImm() && MO.getImm() == 0) {
         unsigned Reg = ExtraCode[0] == 'w' ? AArch64::WZR : AArch64::XZR;
         O << AArch64InstPrinter::getRegisterName(Reg);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'b': // Print B register.
     case 'h': // Print H register.
     case 's': // Print S register.
@@ -970,12 +976,14 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
           RC = &AArch64::ZPRRegClass;
           break;
         default:
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         }
-        return printAsmRegInClass(MO, RC, AArch64::NoRegAltName, O);
+        return (printAsmRegInClass(MO, RC, AArch64::NoRegAltName, O)
+                    ? AsmOperandErrorCode::OPERAND_ERROR
+                    : AsmOperandErrorCode::NO_ERROR);
       }
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
   }
 
@@ -987,11 +995,13 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     // If this is a w or x register, print an x register.
     if (AArch64::GPR32allRegClass.contains(Reg) ||
         AArch64::GPR64allRegClass.contains(Reg))
-      return printAsmMRegister(MO, 'x', O);
+      return (printAsmMRegister(MO, 'x', O) ? AsmOperandErrorCode::OPERAND_ERROR
+                                            : AsmOperandErrorCode::NO_ERROR);
 
     // If this is an x register tuple, print an x register.
     if (AArch64::GPR64x8ClassRegClass.contains(Reg))
-      return printAsmMRegister(MO, 't', O);
+      return (printAsmMRegister(MO, 't', O) ? AsmOperandErrorCode::OPERAND_ERROR
+                                            : AsmOperandErrorCode::NO_ERROR);
 
     unsigned AltName = AArch64::NoRegAltName;
     const TargetRegisterClass *RegClass;
@@ -1007,11 +1017,13 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     }
 
     // If this is a b, h, s, d, or q register, print it as a v register.
-    return printAsmRegInClass(MO, RegClass, AltName, O);
+    return (printAsmRegInClass(MO, RegClass, AltName, O)
+                ? AsmOperandErrorCode::OPERAND_ERROR
+                : AsmOperandErrorCode::NO_ERROR);
   }
 
   printOperand(MI, OpNum, O);
-  return false;
+  return AsmOperandErrorCode::NO_ERROR;
 }
 
 bool AArch64AsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI,
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
index 052b231d62a3eb..5b60a210d08588 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
@@ -1240,21 +1240,24 @@ void AMDGPUAsmPrinter::getAmdKernelCode(amd_kernel_code_t &Out,
   Out.kernarg_segment_alignment = Log2(std::max(Align(16), MaxKernArgAlign));
 }
 
-bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                                       const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                      unsigned OpNo,
+                                                      const char *ExtraCode,
+                                                      raw_ostream &O) {
   // First try the generic code, which knows about modifiers like 'c' and 'n'.
-  if (!AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, O))
-    return false;
+  if (AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, O) ==
+      AsmOperandErrorCode::NO_ERROR)
+    return AsmOperandErrorCode::NO_ERROR;
 
   if (ExtraCode && ExtraCode[0]) {
     if (ExtraCode[1] != 0)
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     case 'r':
       break;
     default:
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     }
   }
 
@@ -1263,7 +1266,7 @@ bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
   if (MO.isReg()) {
     AMDGPUInstPrinter::printRegOperand(MO.getReg(), O,
                                        *MF->getSubtarget().getRegisterInfo());
-    return false;
+    return AsmOperandErrorCode::NO_ERROR;
   } else if (MO.isImm()) {
     int64_t Val = MO.getImm();
     if (AMDGPU::isInlinableIntLiteral(Val)) {
@@ -1275,9 +1278,9 @@ bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
     } else {
       O << format("0x%" PRIx64, static_cast<uint64_t>(Val));
     }
-    return false;
+    return AsmOperandErrorCode::NO_ERROR;
   }
-  return true;
+  return AsmOperandErrorCode::OPERAND_ERROR;
 }
 
 void AMDGPUAsmPrinter::getAnalysisUsage(AnalysisUsage &AU) const {
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
index b8b2718d293e69..5a3032ba6075ff 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
@@ -125,8 +125,9 @@ class AMDGPUAsmPrinter final : public AsmPrinter {
 
   void emitEndOfAsmFile(Module &M) override;
 
-  bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                       const char *ExtraCode, raw_ostream &O) override;
+  AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
+                                      const char *ExtraCode,
+                                      raw_ostream &O) override;
 
 protected:
   void getAnalysisUsage(AnalysisUsage &AU) const override;
diff --git a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
index 642739a29d6b06..9fa0c2b7ecff7c 100644
--- a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
+++ b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
@@ -283,11 +283,14 @@ GetARMJTIPICJumpTableLabel(unsigned uid) const {
   return OutContext.getOrCreateSymbol(Name);
 }
 
-bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                                    const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                   unsigned OpNum,
+                                                   const char *ExtraCode,
+                                                   raw_ostream &O) {
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
-    if (ExtraCode[1] != 0) return true; // Unknown modifier.
+    if (ExtraCode[1] != 0)
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     default:
@@ -296,7 +299,7 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     case 'P': // Print a VFP double precision register.
     case 'q': // Print a NEON quad precision register.
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'y': // Print a VFP single precision register as indexed double.
       if (MI->getOperand(OpNum).isReg()) {
         MCRegister Reg = MI->getOperand(OpNum).getReg().asMCReg();
@@ -308,23 +311,23 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
             continue;
           bool Lane0 = TRI->getSubReg(SR, ARM::ssub_0) == Reg;
           O << ARMInstPrinter::getRegisterName(SR) << (Lane0 ? "[0]" : "[1]");
-          return false;
+          return AsmOperandErrorCode::NO_ERROR;
         }
       }
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     case 'B': // Bitwise inverse of integer or symbol without a preceding #.
       if (!MI->getOperand(OpNum).isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << ~(MI->getOperand(OpNum).getImm());
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'L': // The low 16 bits of an immediate constant.
       if (!MI->getOperand(OpNum).isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << (MI->getOperand(OpNum).getImm() & 0xffff);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'M': { // A register range suitable for LDM/STM.
       if (!MI->getOperand(OpNum).isReg())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &MO = MI->getOperand(OpNum);
       Register RegBegin = MO.getReg();
       // This takes advantage of the 2 operand-ness of ldm/stm and that we've
@@ -352,15 +355,15 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
 
       O << "}";
 
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
     case 'R': // The most significant register of a pair.
     case 'Q': { // The least significant register of a pair.
       if (OpNum == 0)
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &FlagsOP = MI->getOperand(OpNum - 1);
       if (!FlagsOP.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       InlineAsm::Flag F(FlagsOP.getImm());
 
       // This operand may not be the one that actually provides the register. If
@@ -398,64 +401,64 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
       if (F.hasRegClassConstraint(RC) &&
           ARM::GPRPairRegClass.hasSubClassEq(TRI->getRegClass(RC))) {
         if (NumVals != 1)
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         const MachineOperand &MO = MI->getOperand(OpNum);
         if (!MO.isReg())
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
         Register Reg =
             TRI->getSubReg(MO.getReg(), FirstHalf ? ARM::gsub_0 : ARM::gsub_1);
         O << ARMInstPrinter::getRegisterName(Reg);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       if (NumVals != 2)
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       unsigned RegOp = FirstHalf ? OpNum : OpNum + 1;
       if (RegOp >= MI->getNumOperands())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &MO = MI->getOperand(RegOp);
       if (!MO.isReg())...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 10, 2024

@llvm/pr-subscribers-backend-aarch64

Author: None (mahesh-attarde)

Changes

Using inline asm operand constraint 'H' with constants does not reflect helpful error.
With addition of this message operands those are not offsetable, will report exact root cause.
Originally https://reviews.llvm.org/D35890


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

37 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/AsmPrinter.h (+9-3)
  • (modified) llvm/include/llvm/IR/InlineAsm.h (+15-1)
  • (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp (+33-21)
  • (modified) llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp (+30-18)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp (+12-9)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/ARM/ARMAsmPrinter.cpp (+32-29)
  • (modified) llvm/lib/Target/ARM/ARMAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/AVR/AVRAsmPrinter.cpp (+15-11)
  • (modified) llvm/lib/Target/BPF/BPFAsmPrinter.cpp (+8-5)
  • (modified) llvm/lib/Target/CSKY/CSKYAsmPrinter.cpp (+7-4)
  • (modified) llvm/lib/Target/CSKY/CSKYAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/Hexagon/HexagonAsmPrinter.cpp (+9-8)
  • (modified) llvm/lib/Target/Hexagon/HexagonAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/Lanai/LanaiAsmPrinter.cpp (+15-12)
  • (modified) llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp (+16-14)
  • (modified) llvm/lib/Target/LoongArch/LoongArchAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/M68k/M68kAsmPrinter.cpp (+5-3)
  • (modified) llvm/lib/Target/M68k/M68kAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp (+8-5)
  • (modified) llvm/lib/Target/Mips/MipsAsmPrinter.cpp (+26-23)
  • (modified) llvm/lib/Target/Mips/MipsAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp (+6-4)
  • (modified) llvm/lib/Target/NVPTX/NVPTXAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp (+14-10)
  • (modified) llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp (+19-15)
  • (modified) llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp (+11-6)
  • (modified) llvm/lib/Target/Sparc/SparcAsmPrinter.cpp (+33-16)
  • (modified) llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp (+5-4)
  • (modified) llvm/lib/Target/SystemZ/SystemZAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/VE/VEAsmPrinter.cpp (+9-6)
  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp (+12-12)
  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/X86/X86AsmPrinter.cpp (+42-19)
  • (modified) llvm/lib/Target/X86/X86AsmPrinter.h (+7-2)
  • (modified) llvm/lib/Target/XCore/XCoreAsmPrinter.cpp (+8-5)
  • (modified) llvm/test/CodeGen/SPARC/inlineasm-bad.ll (+14-1)
diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h
index 81c3e4be95e9ff..3c29b0133c5fcf 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinter.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinter.h
@@ -817,9 +817,15 @@ class AsmPrinter : public MachineFunctionPass {
   /// Print the specified operand of MI, an INLINEASM instruction, using the
   /// specified assembler variant.  Targets should override this to format as
   /// appropriate.  This method can return true if the operand is erroneous.
-  virtual bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                               const char *ExtraCode, raw_ostream &OS);
-
+  virtual AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI,
+                                              unsigned OpNo,
+                                              const char *ExtraCode,
+                                              raw_ostream &OS);
+
+  /// Print Operand Constraint Error related helpful message
+  virtual void diagnoseAsmOperandError(LLVMContext &C,
+                                       const AsmOperandErrorCode,
+                                       const char *AsmStr, uint64_t Loc);
   /// Print the specified operand of MI, an INLINEASM instruction, using the
   /// specified assembler variant as an address. Targets should override this to
   /// format as appropriate.  This method can return true if the operand is
diff --git a/llvm/include/llvm/IR/InlineAsm.h b/llvm/include/llvm/IR/InlineAsm.h
index e5f506e5694daf..989bbe47006219 100644
--- a/llvm/include/llvm/IR/InlineAsm.h
+++ b/llvm/include/llvm/IR/InlineAsm.h
@@ -528,6 +528,20 @@ class InlineAsm final : public Value {
   }
 };
 
+/// Inline Asm specifies input & output constraint which can
+/// specifiy target specific criteria for operand. If this criteria
+/// does not match, we must throw error.
+/// NO_ERROR represents Operand constraints are valid/applicable
+/// OPERAND_ERROR represents some constraint(unspecified) failed
+/// UNKNOWN_MODIFIER_ERROR represents use of unknown char constraint
+/// CONSTRAINT_<char>_ERROR represents error regarding constraint.
+enum class AsmOperandErrorCode {
+  NO_ERROR = 0,
+  OPERAND_ERROR,
+  UNKNOWN_MODIFIER_ERROR,
+  CONSTRAINT_H_ERROR,
+};
+
 } // end namespace llvm
 
-#endif // LLVM_IR_INLINEASM_H
+#endif // LLVM_IR_INLINEASM_H
\ No newline at end of file
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
index d0ef3e5a19391c..2aa7347ffc3ff6 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
@@ -273,7 +273,7 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
         unsigned OpNo = InlineAsm::MIOp_FirstOperand;
 
         bool Error = false;
-
+        AsmOperandErrorCode OpErrorCode = AsmOperandErrorCode::NO_ERROR;
         // Scan to find the machine operand number for the operand.
         for (; Val; --Val) {
           if (OpNo >= MI->getNumOperands())
@@ -306,15 +306,15 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
             Error = AP->PrintAsmMemoryOperand(
                 MI, OpNo, Modifier[0] ? Modifier : nullptr, OS);
           } else {
-            Error = AP->PrintAsmOperand(MI, OpNo,
-                                        Modifier[0] ? Modifier : nullptr, OS);
+            OpErrorCode = AP->PrintAsmOperand(
+                MI, OpNo, Modifier[0] ? Modifier : nullptr, OS);
           }
         }
-        if (Error) {
-          std::string msg;
-          raw_string_ostream Msg(msg);
-          Msg << "invalid operand in inline asm: '" << AsmStr << "'";
-          MMI->getModule()->getContext().emitError(LocCookie, Msg.str());
+        if (Error || (OpErrorCode != AsmOperandErrorCode::NO_ERROR)) {
+          if (Error)
+            OpErrorCode = AsmOperandErrorCode::OPERAND_ERROR;
+          AP->diagnoseAsmOperandError(MMI->getModule()->getContext(),
+                                      OpErrorCode, AsmStr, LocCookie);
         }
       }
       break;
@@ -461,50 +461,62 @@ void AsmPrinter::PrintSymbolOperand(const MachineOperand &MO, raw_ostream &OS) {
   printOffset(MO.getOffset(), OS);
 }
 
+void AsmPrinter::diagnoseAsmOperandError(LLVMContext &C,
+                                         const AsmOperandErrorCode ErrCode,
+                                         const char *AsmStr,
+                                         const uint64_t Loc) {
+  std::string msg;
+  raw_string_ostream Msg(msg);
+  Msg << "invalid operand in inline asm: '" << AsmStr << "'";
+  C.emitError(Loc, Msg.str());
+}
 /// PrintAsmOperand - Print the specified operand of MI, an INLINEASM
 /// instruction, using the specified assembler variant.  Targets should
 /// override this to format as appropriate for machine specific ExtraCodes
 /// or when the arch-independent handling would be too complex otherwise.
-bool AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                                 const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                unsigned OpNo,
+                                                const char *ExtraCode,
+                                                raw_ostream &O) {
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
-    if (ExtraCode[1] != 0) return true; // Unknown modifier.
+    if (ExtraCode[1] != 0)
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     // https://gcc.gnu.org/onlinedocs/gccint/Output-Template.html
     const MachineOperand &MO = MI->getOperand(OpNo);
     switch (ExtraCode[0]) {
     default:
-      return true;  // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
     case 'a': // Print as memory address.
       if (MO.isReg()) {
         PrintAsmMemoryOperand(MI, OpNo, nullptr, O);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       [[fallthrough]]; // GCC allows '%a' to behave like '%c' with immediates.
     case 'c': // Substitute immediate value without immediate syntax
       if (MO.isImm()) {
         O << MO.getImm();
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       if (MO.isGlobal()) {
         PrintSymbolOperand(MO, O);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     case 'n':  // Negate the immediate constant.
       if (!MO.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << -MO.getImm();
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 's':  // The GCC deprecated s modifier
       if (!MO.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << ((32 - MO.getImm()) & 31);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
   }
-  return true;
+  return AsmOperandErrorCode::OPERAND_ERROR;
 }
 
 bool AsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
diff --git a/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp b/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
index f6ccd0ecfdc893..cdc54ca545be88 100644
--- a/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ b/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -176,8 +176,9 @@ class AArch64AsmPrinter : public AsmPrinter {
                           const TargetRegisterClass *RC, unsigned AltName,
                           raw_ostream &O);
 
-  bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                       const char *ExtraCode, raw_ostream &O) override;
+  AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
+                                      const char *ExtraCode,
+                                      raw_ostream &O) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNum,
                              const char *ExtraCode, raw_ostream &O) override;
 
@@ -915,33 +916,38 @@ bool AArch64AsmPrinter::printAsmRegInClass(const MachineOperand &MO,
   return false;
 }
 
-bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                                        const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                       unsigned OpNum,
+                                                       const char *ExtraCode,
+                                                       raw_ostream &O) {
   const MachineOperand &MO = MI->getOperand(OpNum);
 
   // First try the generic code, which knows about modifiers like 'c' and 'n'.
-  if (!AsmPrinter::PrintAsmOperand(MI, OpNum, ExtraCode, O))
-    return false;
+  if (AsmPrinter::PrintAsmOperand(MI, OpNum, ExtraCode, O) ==
+      AsmOperandErrorCode::NO_ERROR)
+    return AsmOperandErrorCode::NO_ERROR;
 
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
     if (ExtraCode[1] != 0)
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     default:
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
     case 'w':      // Print W register
     case 'x':      // Print X register
       if (MO.isReg())
-        return printAsmMRegister(MO, ExtraCode[0], O);
+        return (printAsmMRegister(MO, ExtraCode[0], O)
+                    ? AsmOperandErrorCode::OPERAND_ERROR
+                    : AsmOperandErrorCode::NO_ERROR);
       if (MO.isImm() && MO.getImm() == 0) {
         unsigned Reg = ExtraCode[0] == 'w' ? AArch64::WZR : AArch64::XZR;
         O << AArch64InstPrinter::getRegisterName(Reg);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'b': // Print B register.
     case 'h': // Print H register.
     case 's': // Print S register.
@@ -970,12 +976,14 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
           RC = &AArch64::ZPRRegClass;
           break;
         default:
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         }
-        return printAsmRegInClass(MO, RC, AArch64::NoRegAltName, O);
+        return (printAsmRegInClass(MO, RC, AArch64::NoRegAltName, O)
+                    ? AsmOperandErrorCode::OPERAND_ERROR
+                    : AsmOperandErrorCode::NO_ERROR);
       }
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
   }
 
@@ -987,11 +995,13 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     // If this is a w or x register, print an x register.
     if (AArch64::GPR32allRegClass.contains(Reg) ||
         AArch64::GPR64allRegClass.contains(Reg))
-      return printAsmMRegister(MO, 'x', O);
+      return (printAsmMRegister(MO, 'x', O) ? AsmOperandErrorCode::OPERAND_ERROR
+                                            : AsmOperandErrorCode::NO_ERROR);
 
     // If this is an x register tuple, print an x register.
     if (AArch64::GPR64x8ClassRegClass.contains(Reg))
-      return printAsmMRegister(MO, 't', O);
+      return (printAsmMRegister(MO, 't', O) ? AsmOperandErrorCode::OPERAND_ERROR
+                                            : AsmOperandErrorCode::NO_ERROR);
 
     unsigned AltName = AArch64::NoRegAltName;
     const TargetRegisterClass *RegClass;
@@ -1007,11 +1017,13 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     }
 
     // If this is a b, h, s, d, or q register, print it as a v register.
-    return printAsmRegInClass(MO, RegClass, AltName, O);
+    return (printAsmRegInClass(MO, RegClass, AltName, O)
+                ? AsmOperandErrorCode::OPERAND_ERROR
+                : AsmOperandErrorCode::NO_ERROR);
   }
 
   printOperand(MI, OpNum, O);
-  return false;
+  return AsmOperandErrorCode::NO_ERROR;
 }
 
 bool AArch64AsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI,
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
index 052b231d62a3eb..5b60a210d08588 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
@@ -1240,21 +1240,24 @@ void AMDGPUAsmPrinter::getAmdKernelCode(amd_kernel_code_t &Out,
   Out.kernarg_segment_alignment = Log2(std::max(Align(16), MaxKernArgAlign));
 }
 
-bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                                       const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                      unsigned OpNo,
+                                                      const char *ExtraCode,
+                                                      raw_ostream &O) {
   // First try the generic code, which knows about modifiers like 'c' and 'n'.
-  if (!AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, O))
-    return false;
+  if (AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, O) ==
+      AsmOperandErrorCode::NO_ERROR)
+    return AsmOperandErrorCode::NO_ERROR;
 
   if (ExtraCode && ExtraCode[0]) {
     if (ExtraCode[1] != 0)
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     case 'r':
       break;
     default:
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     }
   }
 
@@ -1263,7 +1266,7 @@ bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
   if (MO.isReg()) {
     AMDGPUInstPrinter::printRegOperand(MO.getReg(), O,
                                        *MF->getSubtarget().getRegisterInfo());
-    return false;
+    return AsmOperandErrorCode::NO_ERROR;
   } else if (MO.isImm()) {
     int64_t Val = MO.getImm();
     if (AMDGPU::isInlinableIntLiteral(Val)) {
@@ -1275,9 +1278,9 @@ bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
     } else {
       O << format("0x%" PRIx64, static_cast<uint64_t>(Val));
     }
-    return false;
+    return AsmOperandErrorCode::NO_ERROR;
   }
-  return true;
+  return AsmOperandErrorCode::OPERAND_ERROR;
 }
 
 void AMDGPUAsmPrinter::getAnalysisUsage(AnalysisUsage &AU) const {
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
index b8b2718d293e69..5a3032ba6075ff 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
@@ -125,8 +125,9 @@ class AMDGPUAsmPrinter final : public AsmPrinter {
 
   void emitEndOfAsmFile(Module &M) override;
 
-  bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                       const char *ExtraCode, raw_ostream &O) override;
+  AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
+                                      const char *ExtraCode,
+                                      raw_ostream &O) override;
 
 protected:
   void getAnalysisUsage(AnalysisUsage &AU) const override;
diff --git a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
index 642739a29d6b06..9fa0c2b7ecff7c 100644
--- a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
+++ b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
@@ -283,11 +283,14 @@ GetARMJTIPICJumpTableLabel(unsigned uid) const {
   return OutContext.getOrCreateSymbol(Name);
 }
 
-bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                                    const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                   unsigned OpNum,
+                                                   const char *ExtraCode,
+                                                   raw_ostream &O) {
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
-    if (ExtraCode[1] != 0) return true; // Unknown modifier.
+    if (ExtraCode[1] != 0)
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     default:
@@ -296,7 +299,7 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     case 'P': // Print a VFP double precision register.
     case 'q': // Print a NEON quad precision register.
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'y': // Print a VFP single precision register as indexed double.
       if (MI->getOperand(OpNum).isReg()) {
         MCRegister Reg = MI->getOperand(OpNum).getReg().asMCReg();
@@ -308,23 +311,23 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
             continue;
           bool Lane0 = TRI->getSubReg(SR, ARM::ssub_0) == Reg;
           O << ARMInstPrinter::getRegisterName(SR) << (Lane0 ? "[0]" : "[1]");
-          return false;
+          return AsmOperandErrorCode::NO_ERROR;
         }
       }
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     case 'B': // Bitwise inverse of integer or symbol without a preceding #.
       if (!MI->getOperand(OpNum).isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << ~(MI->getOperand(OpNum).getImm());
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'L': // The low 16 bits of an immediate constant.
       if (!MI->getOperand(OpNum).isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << (MI->getOperand(OpNum).getImm() & 0xffff);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'M': { // A register range suitable for LDM/STM.
       if (!MI->getOperand(OpNum).isReg())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &MO = MI->getOperand(OpNum);
       Register RegBegin = MO.getReg();
       // This takes advantage of the 2 operand-ness of ldm/stm and that we've
@@ -352,15 +355,15 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
 
       O << "}";
 
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
     case 'R': // The most significant register of a pair.
     case 'Q': { // The least significant register of a pair.
       if (OpNum == 0)
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &FlagsOP = MI->getOperand(OpNum - 1);
       if (!FlagsOP.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       InlineAsm::Flag F(FlagsOP.getImm());
 
       // This operand may not be the one that actually provides the register. If
@@ -398,64 +401,64 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
       if (F.hasRegClassConstraint(RC) &&
           ARM::GPRPairRegClass.hasSubClassEq(TRI->getRegClass(RC))) {
         if (NumVals != 1)
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         const MachineOperand &MO = MI->getOperand(OpNum);
         if (!MO.isReg())
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
         Register Reg =
             TRI->getSubReg(MO.getReg(), FirstHalf ? ARM::gsub_0 : ARM::gsub_1);
         O << ARMInstPrinter::getRegisterName(Reg);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       if (NumVals != 2)
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       unsigned RegOp = FirstHalf ? OpNum : OpNum + 1;
       if (RegOp >= MI->getNumOperands())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &MO = MI->getOperand(RegOp);
       if (!MO.isReg())...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 10, 2024

@llvm/pr-subscribers-backend-risc-v

Author: None (mahesh-attarde)

Changes

Using inline asm operand constraint 'H' with constants does not reflect helpful error.
With addition of this message operands those are not offsetable, will report exact root cause.
Originally https://reviews.llvm.org/D35890


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

37 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/AsmPrinter.h (+9-3)
  • (modified) llvm/include/llvm/IR/InlineAsm.h (+15-1)
  • (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp (+33-21)
  • (modified) llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp (+30-18)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp (+12-9)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/ARM/ARMAsmPrinter.cpp (+32-29)
  • (modified) llvm/lib/Target/ARM/ARMAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/AVR/AVRAsmPrinter.cpp (+15-11)
  • (modified) llvm/lib/Target/BPF/BPFAsmPrinter.cpp (+8-5)
  • (modified) llvm/lib/Target/CSKY/CSKYAsmPrinter.cpp (+7-4)
  • (modified) llvm/lib/Target/CSKY/CSKYAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/Hexagon/HexagonAsmPrinter.cpp (+9-8)
  • (modified) llvm/lib/Target/Hexagon/HexagonAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/Lanai/LanaiAsmPrinter.cpp (+15-12)
  • (modified) llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp (+16-14)
  • (modified) llvm/lib/Target/LoongArch/LoongArchAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/M68k/M68kAsmPrinter.cpp (+5-3)
  • (modified) llvm/lib/Target/M68k/M68kAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp (+8-5)
  • (modified) llvm/lib/Target/Mips/MipsAsmPrinter.cpp (+26-23)
  • (modified) llvm/lib/Target/Mips/MipsAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp (+6-4)
  • (modified) llvm/lib/Target/NVPTX/NVPTXAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp (+14-10)
  • (modified) llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp (+19-15)
  • (modified) llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp (+11-6)
  • (modified) llvm/lib/Target/Sparc/SparcAsmPrinter.cpp (+33-16)
  • (modified) llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp (+5-4)
  • (modified) llvm/lib/Target/SystemZ/SystemZAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/VE/VEAsmPrinter.cpp (+9-6)
  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp (+12-12)
  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h (+3-2)
  • (modified) llvm/lib/Target/X86/X86AsmPrinter.cpp (+42-19)
  • (modified) llvm/lib/Target/X86/X86AsmPrinter.h (+7-2)
  • (modified) llvm/lib/Target/XCore/XCoreAsmPrinter.cpp (+8-5)
  • (modified) llvm/test/CodeGen/SPARC/inlineasm-bad.ll (+14-1)
diff --git a/llvm/include/llvm/CodeGen/AsmPrinter.h b/llvm/include/llvm/CodeGen/AsmPrinter.h
index 81c3e4be95e9ff..3c29b0133c5fcf 100644
--- a/llvm/include/llvm/CodeGen/AsmPrinter.h
+++ b/llvm/include/llvm/CodeGen/AsmPrinter.h
@@ -817,9 +817,15 @@ class AsmPrinter : public MachineFunctionPass {
   /// Print the specified operand of MI, an INLINEASM instruction, using the
   /// specified assembler variant.  Targets should override this to format as
   /// appropriate.  This method can return true if the operand is erroneous.
-  virtual bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                               const char *ExtraCode, raw_ostream &OS);
-
+  virtual AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI,
+                                              unsigned OpNo,
+                                              const char *ExtraCode,
+                                              raw_ostream &OS);
+
+  /// Print Operand Constraint Error related helpful message
+  virtual void diagnoseAsmOperandError(LLVMContext &C,
+                                       const AsmOperandErrorCode,
+                                       const char *AsmStr, uint64_t Loc);
   /// Print the specified operand of MI, an INLINEASM instruction, using the
   /// specified assembler variant as an address. Targets should override this to
   /// format as appropriate.  This method can return true if the operand is
diff --git a/llvm/include/llvm/IR/InlineAsm.h b/llvm/include/llvm/IR/InlineAsm.h
index e5f506e5694daf..989bbe47006219 100644
--- a/llvm/include/llvm/IR/InlineAsm.h
+++ b/llvm/include/llvm/IR/InlineAsm.h
@@ -528,6 +528,20 @@ class InlineAsm final : public Value {
   }
 };
 
+/// Inline Asm specifies input & output constraint which can
+/// specifiy target specific criteria for operand. If this criteria
+/// does not match, we must throw error.
+/// NO_ERROR represents Operand constraints are valid/applicable
+/// OPERAND_ERROR represents some constraint(unspecified) failed
+/// UNKNOWN_MODIFIER_ERROR represents use of unknown char constraint
+/// CONSTRAINT_<char>_ERROR represents error regarding constraint.
+enum class AsmOperandErrorCode {
+  NO_ERROR = 0,
+  OPERAND_ERROR,
+  UNKNOWN_MODIFIER_ERROR,
+  CONSTRAINT_H_ERROR,
+};
+
 } // end namespace llvm
 
-#endif // LLVM_IR_INLINEASM_H
+#endif // LLVM_IR_INLINEASM_H
\ No newline at end of file
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
index d0ef3e5a19391c..2aa7347ffc3ff6 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
@@ -273,7 +273,7 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
         unsigned OpNo = InlineAsm::MIOp_FirstOperand;
 
         bool Error = false;
-
+        AsmOperandErrorCode OpErrorCode = AsmOperandErrorCode::NO_ERROR;
         // Scan to find the machine operand number for the operand.
         for (; Val; --Val) {
           if (OpNo >= MI->getNumOperands())
@@ -306,15 +306,15 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
             Error = AP->PrintAsmMemoryOperand(
                 MI, OpNo, Modifier[0] ? Modifier : nullptr, OS);
           } else {
-            Error = AP->PrintAsmOperand(MI, OpNo,
-                                        Modifier[0] ? Modifier : nullptr, OS);
+            OpErrorCode = AP->PrintAsmOperand(
+                MI, OpNo, Modifier[0] ? Modifier : nullptr, OS);
           }
         }
-        if (Error) {
-          std::string msg;
-          raw_string_ostream Msg(msg);
-          Msg << "invalid operand in inline asm: '" << AsmStr << "'";
-          MMI->getModule()->getContext().emitError(LocCookie, Msg.str());
+        if (Error || (OpErrorCode != AsmOperandErrorCode::NO_ERROR)) {
+          if (Error)
+            OpErrorCode = AsmOperandErrorCode::OPERAND_ERROR;
+          AP->diagnoseAsmOperandError(MMI->getModule()->getContext(),
+                                      OpErrorCode, AsmStr, LocCookie);
         }
       }
       break;
@@ -461,50 +461,62 @@ void AsmPrinter::PrintSymbolOperand(const MachineOperand &MO, raw_ostream &OS) {
   printOffset(MO.getOffset(), OS);
 }
 
+void AsmPrinter::diagnoseAsmOperandError(LLVMContext &C,
+                                         const AsmOperandErrorCode ErrCode,
+                                         const char *AsmStr,
+                                         const uint64_t Loc) {
+  std::string msg;
+  raw_string_ostream Msg(msg);
+  Msg << "invalid operand in inline asm: '" << AsmStr << "'";
+  C.emitError(Loc, Msg.str());
+}
 /// PrintAsmOperand - Print the specified operand of MI, an INLINEASM
 /// instruction, using the specified assembler variant.  Targets should
 /// override this to format as appropriate for machine specific ExtraCodes
 /// or when the arch-independent handling would be too complex otherwise.
-bool AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                                 const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                unsigned OpNo,
+                                                const char *ExtraCode,
+                                                raw_ostream &O) {
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
-    if (ExtraCode[1] != 0) return true; // Unknown modifier.
+    if (ExtraCode[1] != 0)
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     // https://gcc.gnu.org/onlinedocs/gccint/Output-Template.html
     const MachineOperand &MO = MI->getOperand(OpNo);
     switch (ExtraCode[0]) {
     default:
-      return true;  // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
     case 'a': // Print as memory address.
       if (MO.isReg()) {
         PrintAsmMemoryOperand(MI, OpNo, nullptr, O);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       [[fallthrough]]; // GCC allows '%a' to behave like '%c' with immediates.
     case 'c': // Substitute immediate value without immediate syntax
       if (MO.isImm()) {
         O << MO.getImm();
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       if (MO.isGlobal()) {
         PrintSymbolOperand(MO, O);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     case 'n':  // Negate the immediate constant.
       if (!MO.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << -MO.getImm();
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 's':  // The GCC deprecated s modifier
       if (!MO.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << ((32 - MO.getImm()) & 31);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
   }
-  return true;
+  return AsmOperandErrorCode::OPERAND_ERROR;
 }
 
 bool AsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
diff --git a/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp b/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
index f6ccd0ecfdc893..cdc54ca545be88 100644
--- a/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ b/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -176,8 +176,9 @@ class AArch64AsmPrinter : public AsmPrinter {
                           const TargetRegisterClass *RC, unsigned AltName,
                           raw_ostream &O);
 
-  bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                       const char *ExtraCode, raw_ostream &O) override;
+  AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
+                                      const char *ExtraCode,
+                                      raw_ostream &O) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNum,
                              const char *ExtraCode, raw_ostream &O) override;
 
@@ -915,33 +916,38 @@ bool AArch64AsmPrinter::printAsmRegInClass(const MachineOperand &MO,
   return false;
 }
 
-bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                                        const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                       unsigned OpNum,
+                                                       const char *ExtraCode,
+                                                       raw_ostream &O) {
   const MachineOperand &MO = MI->getOperand(OpNum);
 
   // First try the generic code, which knows about modifiers like 'c' and 'n'.
-  if (!AsmPrinter::PrintAsmOperand(MI, OpNum, ExtraCode, O))
-    return false;
+  if (AsmPrinter::PrintAsmOperand(MI, OpNum, ExtraCode, O) ==
+      AsmOperandErrorCode::NO_ERROR)
+    return AsmOperandErrorCode::NO_ERROR;
 
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
     if (ExtraCode[1] != 0)
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     default:
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
     case 'w':      // Print W register
     case 'x':      // Print X register
       if (MO.isReg())
-        return printAsmMRegister(MO, ExtraCode[0], O);
+        return (printAsmMRegister(MO, ExtraCode[0], O)
+                    ? AsmOperandErrorCode::OPERAND_ERROR
+                    : AsmOperandErrorCode::NO_ERROR);
       if (MO.isImm() && MO.getImm() == 0) {
         unsigned Reg = ExtraCode[0] == 'w' ? AArch64::WZR : AArch64::XZR;
         O << AArch64InstPrinter::getRegisterName(Reg);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'b': // Print B register.
     case 'h': // Print H register.
     case 's': // Print S register.
@@ -970,12 +976,14 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
           RC = &AArch64::ZPRRegClass;
           break;
         default:
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         }
-        return printAsmRegInClass(MO, RC, AArch64::NoRegAltName, O);
+        return (printAsmRegInClass(MO, RC, AArch64::NoRegAltName, O)
+                    ? AsmOperandErrorCode::OPERAND_ERROR
+                    : AsmOperandErrorCode::NO_ERROR);
       }
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
   }
 
@@ -987,11 +995,13 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     // If this is a w or x register, print an x register.
     if (AArch64::GPR32allRegClass.contains(Reg) ||
         AArch64::GPR64allRegClass.contains(Reg))
-      return printAsmMRegister(MO, 'x', O);
+      return (printAsmMRegister(MO, 'x', O) ? AsmOperandErrorCode::OPERAND_ERROR
+                                            : AsmOperandErrorCode::NO_ERROR);
 
     // If this is an x register tuple, print an x register.
     if (AArch64::GPR64x8ClassRegClass.contains(Reg))
-      return printAsmMRegister(MO, 't', O);
+      return (printAsmMRegister(MO, 't', O) ? AsmOperandErrorCode::OPERAND_ERROR
+                                            : AsmOperandErrorCode::NO_ERROR);
 
     unsigned AltName = AArch64::NoRegAltName;
     const TargetRegisterClass *RegClass;
@@ -1007,11 +1017,13 @@ bool AArch64AsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     }
 
     // If this is a b, h, s, d, or q register, print it as a v register.
-    return printAsmRegInClass(MO, RegClass, AltName, O);
+    return (printAsmRegInClass(MO, RegClass, AltName, O)
+                ? AsmOperandErrorCode::OPERAND_ERROR
+                : AsmOperandErrorCode::NO_ERROR);
   }
 
   printOperand(MI, OpNum, O);
-  return false;
+  return AsmOperandErrorCode::NO_ERROR;
 }
 
 bool AArch64AsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI,
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
index 052b231d62a3eb..5b60a210d08588 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
@@ -1240,21 +1240,24 @@ void AMDGPUAsmPrinter::getAmdKernelCode(amd_kernel_code_t &Out,
   Out.kernarg_segment_alignment = Log2(std::max(Align(16), MaxKernArgAlign));
 }
 
-bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                                       const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                      unsigned OpNo,
+                                                      const char *ExtraCode,
+                                                      raw_ostream &O) {
   // First try the generic code, which knows about modifiers like 'c' and 'n'.
-  if (!AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, O))
-    return false;
+  if (AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, O) ==
+      AsmOperandErrorCode::NO_ERROR)
+    return AsmOperandErrorCode::NO_ERROR;
 
   if (ExtraCode && ExtraCode[0]) {
     if (ExtraCode[1] != 0)
-      return true; // Unknown modifier.
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     case 'r':
       break;
     default:
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     }
   }
 
@@ -1263,7 +1266,7 @@ bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
   if (MO.isReg()) {
     AMDGPUInstPrinter::printRegOperand(MO.getReg(), O,
                                        *MF->getSubtarget().getRegisterInfo());
-    return false;
+    return AsmOperandErrorCode::NO_ERROR;
   } else if (MO.isImm()) {
     int64_t Val = MO.getImm();
     if (AMDGPU::isInlinableIntLiteral(Val)) {
@@ -1275,9 +1278,9 @@ bool AMDGPUAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
     } else {
       O << format("0x%" PRIx64, static_cast<uint64_t>(Val));
     }
-    return false;
+    return AsmOperandErrorCode::NO_ERROR;
   }
-  return true;
+  return AsmOperandErrorCode::OPERAND_ERROR;
 }
 
 void AMDGPUAsmPrinter::getAnalysisUsage(AnalysisUsage &AU) const {
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
index b8b2718d293e69..5a3032ba6075ff 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
@@ -125,8 +125,9 @@ class AMDGPUAsmPrinter final : public AsmPrinter {
 
   void emitEndOfAsmFile(Module &M) override;
 
-  bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
-                       const char *ExtraCode, raw_ostream &O) override;
+  AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
+                                      const char *ExtraCode,
+                                      raw_ostream &O) override;
 
 protected:
   void getAnalysisUsage(AnalysisUsage &AU) const override;
diff --git a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
index 642739a29d6b06..9fa0c2b7ecff7c 100644
--- a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
+++ b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
@@ -283,11 +283,14 @@ GetARMJTIPICJumpTableLabel(unsigned uid) const {
   return OutContext.getOrCreateSymbol(Name);
 }
 
-bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
-                                    const char *ExtraCode, raw_ostream &O) {
+AsmOperandErrorCode ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI,
+                                                   unsigned OpNum,
+                                                   const char *ExtraCode,
+                                                   raw_ostream &O) {
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
-    if (ExtraCode[1] != 0) return true; // Unknown modifier.
+    if (ExtraCode[1] != 0)
+      return AsmOperandErrorCode::UNKNOWN_MODIFIER_ERROR; // Unknown modifier.
 
     switch (ExtraCode[0]) {
     default:
@@ -296,7 +299,7 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
     case 'P': // Print a VFP double precision register.
     case 'q': // Print a NEON quad precision register.
       printOperand(MI, OpNum, O);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'y': // Print a VFP single precision register as indexed double.
       if (MI->getOperand(OpNum).isReg()) {
         MCRegister Reg = MI->getOperand(OpNum).getReg().asMCReg();
@@ -308,23 +311,23 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
             continue;
           bool Lane0 = TRI->getSubReg(SR, ARM::ssub_0) == Reg;
           O << ARMInstPrinter::getRegisterName(SR) << (Lane0 ? "[0]" : "[1]");
-          return false;
+          return AsmOperandErrorCode::NO_ERROR;
         }
       }
-      return true;
+      return AsmOperandErrorCode::OPERAND_ERROR;
     case 'B': // Bitwise inverse of integer or symbol without a preceding #.
       if (!MI->getOperand(OpNum).isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << ~(MI->getOperand(OpNum).getImm());
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'L': // The low 16 bits of an immediate constant.
       if (!MI->getOperand(OpNum).isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       O << (MI->getOperand(OpNum).getImm() & 0xffff);
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     case 'M': { // A register range suitable for LDM/STM.
       if (!MI->getOperand(OpNum).isReg())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &MO = MI->getOperand(OpNum);
       Register RegBegin = MO.getReg();
       // This takes advantage of the 2 operand-ness of ldm/stm and that we've
@@ -352,15 +355,15 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
 
       O << "}";
 
-      return false;
+      return AsmOperandErrorCode::NO_ERROR;
     }
     case 'R': // The most significant register of a pair.
     case 'Q': { // The least significant register of a pair.
       if (OpNum == 0)
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &FlagsOP = MI->getOperand(OpNum - 1);
       if (!FlagsOP.isImm())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       InlineAsm::Flag F(FlagsOP.getImm());
 
       // This operand may not be the one that actually provides the register. If
@@ -398,64 +401,64 @@ bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
       if (F.hasRegClassConstraint(RC) &&
           ARM::GPRPairRegClass.hasSubClassEq(TRI->getRegClass(RC))) {
         if (NumVals != 1)
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         const MachineOperand &MO = MI->getOperand(OpNum);
         if (!MO.isReg())
-          return true;
+          return AsmOperandErrorCode::OPERAND_ERROR;
         const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
         Register Reg =
             TRI->getSubReg(MO.getReg(), FirstHalf ? ARM::gsub_0 : ARM::gsub_1);
         O << ARMInstPrinter::getRegisterName(Reg);
-        return false;
+        return AsmOperandErrorCode::NO_ERROR;
       }
       if (NumVals != 2)
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       unsigned RegOp = FirstHalf ? OpNum : OpNum + 1;
       if (RegOp >= MI->getNumOperands())
-        return true;
+        return AsmOperandErrorCode::OPERAND_ERROR;
       const MachineOperand &MO = MI->getOperand(RegOp);
       if (!MO.isReg())...
[truncated]

Copy link
Collaborator

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

There are a number of code formatting issues here

NO_ERROR = 0,
OPERAND_ERROR,
UNKNOWN_MODIFIER_ERROR,
CONSTRAINT_H_ERROR,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't we just make the constraint code available to the error printing code? Having one of these for every possible case sounds like a headache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All constraint codes related to inline asm are defined in same file.
Will adding in AsmPrinter.h be reasonable ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The constraint H seems too specific. Why not just let this be an arbitrary string carried by Error?

@@ -1,3 +1,4 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can UTC actually update error messages?...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. will drop this

@@ -13,8 +14,20 @@ entry:
}

; CHECK-label:test_twinword_error
; CHECK: error: invalid operand in inline asm: 'rd %asr5, ${0:L}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are there now three copies of all 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.

697dd93#r140730056
I reached out to author, he intended it.

switch (EC) {
default:
break;
case AsmOperandErrorCode::CONSTRAINT_H_ERROR:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see where the X86 backend ever returns 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.

I did not understand comment.
From Here, we are first printing some generic diagnostic present without this patch. e,g, regarding "invalid Operand with Src Location" , Since target can have constraint code specific to architecture, I used specialized X86AsmPrinter::diagnoseAsmOperandError to just provide helpful diagnostics for 'H;

Copy link
Collaborator

Choose a reason for hiding this comment

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

But H doesn't return this error on X86, so this is dead code. Which you would know if you wrote a test for this code (something else that's lacking from this PR).

@jrtc27
Copy link
Collaborator

jrtc27 commented Apr 10, 2024

Originally https://reviews.llvm.org/D35890

This is a very different implementation to that

@mahesh-attarde
Copy link
Contributor Author

Originally https://reviews.llvm.org/D35890

This is a very different implementation to that

Diagnostic for constraint H was required from this PR. It got pushed out for long, there were code changes which did not merge.
I think this is 3rd attempt

@mahesh-attarde
Copy link
Contributor Author

There are a number of code formatting issues here

I am not getting any updates on git clang format. Do you mean enum NO_ERROR -> NOERROR ?

raw_string_ostream Msg(msg);
Msg << "invalid operand in inline asm: '" << AsmStr << "'";
C.emitError(Loc, Msg.str());
}
/// PrintAsmOperand - Print the specified operand of MI, an INLINEASM
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// PrintAsmOperand - Print the specified operand of MI, an INLINEASM
/// PrintAsmOperand - Print the specified operand of MI, an INLINEASM

break;
}
C.emitError(Msg.str());
}
/// PrintAsmOperand - Print out an operand for an inline asm expression.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// PrintAsmOperand - Print out an operand for an inline asm expression.
/// PrintAsmOperand - Print out an operand for an inline asm expression.

break;
}
C.emitError(Msg.str());
}
/// PrintAsmOperand - Print out an operand for an inline asm expression.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// PrintAsmOperand - Print out an operand for an inline asm expression.
/// PrintAsmOperand - Print out an operand for an inline asm expression.

; CHECK: error: Hi part of pair should point to an even-numbered register
; CHECK: error: (note that in some cases it might be necessary to manually bind the input/output registers instead of relying on automatic allocation)
; CHECK: (note that in some cases it might be necessary to manually bind the input/output registers instead of relying on automatic allocation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lost the error:

@@ -13,8 +14,20 @@ entry:
}

; CHECK-label:test_twinword_error
; CHECK: error: invalid operand in inline asm: 'rd %asr5, ${0:L}
; CHECK: srlx ${0:L}, 32, ${0:H}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why's this on a new line?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's because the testcase has a \0A in it?

} // end namespace llvm

#endif // LLVM_IR_INLINEASM_H
#endif // LLVM_IR_INLINEASM_H
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing newline

"bind the input/output registers instead of relying on "
"automatic allocation)");
return true;
return AsmOperandErrorCode::CONSTRAINT_H_ERROR;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is for H and L, not just H...

; CHECK: error: (note that in some cases it might be necessary to manually bind the input/output registers instead of relying on automatic allocation)
; CHECK: (note that in some cases it might be necessary to manually bind the input/output registers instead of relying on automatic allocation)

; CHECK: error: invalid operand in inline asm: 'rd %asr5, ${0:L}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koachan I updated Sparc Test I mentioned here 697dd93#r140773364
Can you check once ?

virtual bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
const char *ExtraCode, raw_ostream &OS);

virtual AsmOperandErrorCode PrintAsmOperand(const MachineInstr *MI,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally the mechanical change to use AsmOperandErrorCode instead of bool should be done first, before the behavior change

@mahesh-attarde
Copy link
Contributor Author

Discontinuing considering impact and time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants