DiagnosticInfo: Clean up usage of DiagnosticInfoInlineAsm

Currently LLVMContext::emitError emits any error as an "inline asm"
error which does not make any sense. InlineAsm appears to be special,
in that it uses a "LocCookie" from srcloc metadata, which looks like
a parallel mechanism to ordinary source line locations. This meant
that other types of failures had degraded source information reported
when available.

Introduce some new generic error types, and only use inline asm
in the appropriate contexts. The DiagnosticInfo types are still
a bit of a mess, and I'm not sure why DiagnosticInfoWithLocationBase
exists instead of just having an optional DiagnosticLocation in the
base class.

DK_Generic is for any error that derives from an IR level instruction,
and thus can pull debug locations directly from it. DK_GenericWithLoc
is functionally the generic codegen error, since it does not depend
on the IR and instead can construct a DiagnosticLocation from the
MI debug location.

Author: arsenm

llvm/include/llvm/CodeGen/MachineInstr.h

@@ -555,13 +555,18 @@ class MachineInstr
   /// will be dropped.
   void dropDebugNumber() { DebugInstrNum = 0; }
 
-  /// Emit an error referring to the source location of this instruction.
-  /// This should only be used for inline assembly that is somehow
-  /// impossible to compile. Other errors should have been handled much
-  /// earlier.
-  ///
-  /// If this method returns, the caller should try to recover from the error.
-  void emitError(StringRef Msg) const;
+  /// For inline asm, get the !srcloc metadata node if we have it, and decode
+  /// the loc cookie from it.
+  const MDNode *getLocCookieMD() const;
+
+  /// Emit an error referring to the source location of this instruction. This
+  /// should only be used for inline assembly that is somehow impossible to
+  /// compile. Other errors should have been handled much earlier.
+  void emitInlineAsmError(const Twine &ErrMsg) const;
+
+  // Emit an error in the LLVMContext referring to the source location of this
+  // instruction, if available.
+  void emitGenericError(const Twine &ErrMsg) const;
 
   /// Returns the target instruction descriptor of this MachineInstr.
   const MCInstrDesc &getDesc() const { return *MCID; }

llvm/include/llvm/IR/DiagnosticInfo.h

@@ -58,6 +58,8 @@ enum DiagnosticSeverity : char {
 /// Defines the different supported kind of a diagnostic.
 /// This enum should be extended with a new ID for each added concrete subclass.
 enum DiagnosticKind {
+  DK_Generic,
+  DK_GenericWithLoc,
   DK_InlineAsm,
   DK_ResourceLimit,
   DK_StackSize,
@@ -134,6 +136,33 @@ class DiagnosticInfo {
 
 using DiagnosticHandlerFunction = std::function<void(const DiagnosticInfo &)>;
 
+class DiagnosticInfoGeneric : public DiagnosticInfo {
+  const Twine &MsgStr;
+  const Instruction *Inst = nullptr;
+
+public:
+  /// \p MsgStr is the message to be reported to the frontend.
+  /// This class does not copy \p MsgStr, therefore the reference must be valid
+  /// for the whole life time of the Diagnostic.
+  DiagnosticInfoGeneric(const Twine &MsgStr,
+                        DiagnosticSeverity Severity = DS_Error)
+      : DiagnosticInfo(DK_Generic, Severity), MsgStr(MsgStr) {}
+
+  DiagnosticInfoGeneric(const Instruction *I, const Twine &ErrMsg,
+                        DiagnosticSeverity Severity = DS_Warning)
+      : DiagnosticInfo(DK_Generic, Severity), MsgStr(ErrMsg), Inst(I) {}
+
+  const Twine &getMsgStr() const { return MsgStr; }
+  const Instruction *getInstruction() const { return Inst; }
+
+  /// \see DiagnosticInfo::print.
+  void print(DiagnosticPrinter &DP) const override;
+
+  static bool classof(const DiagnosticInfo *DI) {
+    return DI->getKind() == DK_Generic;
+  }
+};
+
 /// Diagnostic information for inline asm reporting.
 /// This is basically a message and an optional location.
 class DiagnosticInfoInlineAsm : public DiagnosticInfo {
@@ -146,21 +175,12 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   const Instruction *Instr = nullptr;
 
 public:
-  /// \p MsgStr is the message to be reported to the frontend.
-  /// This class does not copy \p MsgStr, therefore the reference must be valid
-  /// for the whole life time of the Diagnostic.
-  DiagnosticInfoInlineAsm(const Twine &MsgStr,
-                          DiagnosticSeverity Severity = DS_Error)
-      : DiagnosticInfo(DK_InlineAsm, Severity), MsgStr(MsgStr) {}
-
   /// \p LocCookie if non-zero gives the line number for this report.
   /// \p MsgStr gives the message.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
   DiagnosticInfoInlineAsm(uint64_t LocCookie, const Twine &MsgStr,
-                          DiagnosticSeverity Severity = DS_Error)
-      : DiagnosticInfo(DK_InlineAsm, Severity), LocCookie(LocCookie),
-        MsgStr(MsgStr) {}
+                          DiagnosticSeverity Severity = DS_Error);
 
   /// \p Instr gives the original instruction that triggered the diagnostic.
   /// \p MsgStr gives the message.
@@ -354,6 +374,31 @@ class DiagnosticInfoWithLocationBase : public DiagnosticInfo {
   DiagnosticLocation Loc;
 };
 
+class DiagnosticInfoGenericWithLoc : public DiagnosticInfoWithLocationBase {
+private:
+  /// Message to be reported.
+  const Twine &MsgStr;
+
+public:
+  /// \p MsgStr is the message to be reported to the frontend.
+  /// This class does not copy \p MsgStr, therefore the reference must be valid
+  /// for the whole life time of the Diagnostic.
+  DiagnosticInfoGenericWithLoc(const Twine &MsgStr, const Function &Fn,
+                               const DiagnosticLocation &Loc,
+                               DiagnosticSeverity Severity = DS_Error)
+      : DiagnosticInfoWithLocationBase(DK_GenericWithLoc, Severity, Fn, Loc),
+        MsgStr(MsgStr) {}
+
+  const Twine &getMsgStr() const { return MsgStr; }
+
+  /// \see DiagnosticInfo::print.
+  void print(DiagnosticPrinter &DP) const override;
+
+  static bool classof(const DiagnosticInfo *DI) {
+    return DI->getKind() == DK_GenericWithLoc;
+  }
+};
+
 /// Diagnostic information for stack size etc. reporting.
 /// This is basically a function and a size.
 class DiagnosticInfoResourceLimit : public DiagnosticInfoWithLocationBase {

llvm/include/llvm/IR/LLVMContext.h

@@ -305,7 +305,6 @@ class LLVMContext {
   /// be prepared to drop the erroneous construct on the floor and "not crash".
   /// The generated code need not be correct.  The error message will be
   /// implicitly prefixed with "error: " and should not end with a ".".
-  void emitError(uint64_t LocCookie, const Twine &ErrorStr);
   void emitError(const Instruction *I, const Twine &ErrorStr);
   void emitError(const Twine &ErrorStr);
 

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

@@ -312,10 +312,11 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
           }
         }
         if (Error) {
-          std::string msg;
-          raw_string_ostream Msg(msg);
-          Msg << "invalid operand in inline asm: '" << AsmStr << "'";
-          MMI->getModule()->getContext().emitError(LocCookie, msg);
+          const Function &Fn = MI->getMF()->getFunction();
+          DiagnosticInfoInlineAsm DI(LocCookie,
+                                     "invalid operand in inline asm: '" +
+                                         Twine(AsmStr) + "'");
+          Fn.getContext().diagnose(DI);
         }
       }
       break;
@@ -347,20 +348,11 @@ void AsmPrinter::emitInlineAsm(const MachineInstr *MI) const {
   // enabled, so we use emitRawComment.
   OutStreamer->emitRawComment(MAI->getInlineAsmStart());
 
-  // Get the !srcloc metadata node if we have it, and decode the loc cookie from
-  // it.
-  uint64_t LocCookie = 0;
-  const MDNode *LocMD = nullptr;
-  for (const MachineOperand &MO : llvm::reverse(MI->operands())) {
-    if (MO.isMetadata() && (LocMD = MO.getMetadata()) &&
-        LocMD->getNumOperands() != 0) {
-      if (const ConstantInt *CI =
-              mdconst::dyn_extract<ConstantInt>(LocMD->getOperand(0))) {
-        LocCookie = CI->getZExtValue();
-        break;
-      }
-    }
-  }
+  const MDNode *LocMD = MI->getLocCookieMD();
+  uint64_t LocCookie =
+      LocMD
+          ? mdconst::extract<ConstantInt>(LocMD->getOperand(0))->getZExtValue()
+          : 0;
 
   // Emit the inline asm to a temporary string so we can emit it through
   // EmitInlineAsm.
@@ -397,20 +389,23 @@ void AsmPrinter::emitInlineAsm(const MachineInstr *MI) const {
       Msg += LS;
       Msg += TRI->getRegAsmName(RR);
     }
+
+    const Function &Fn = MF->getFunction();
     const char *Note =
         "Reserved registers on the clobber list may not be "
         "preserved across the asm statement, and clobbering them may "
         "lead to undefined behaviour.";
-    MMI->getModule()->getContext().diagnose(DiagnosticInfoInlineAsm(
-        LocCookie, Msg, DiagnosticSeverity::DS_Warning));
-    MMI->getModule()->getContext().diagnose(
+    LLVMContext &Ctx = Fn.getContext();
+    Ctx.diagnose(DiagnosticInfoInlineAsm(LocCookie, Msg,
+                                         DiagnosticSeverity::DS_Warning));
+    Ctx.diagnose(
         DiagnosticInfoInlineAsm(LocCookie, Note, DiagnosticSeverity::DS_Note));
 
     for (const Register RR : RestrRegs) {
       if (std::optional<std::string> reason =
               TRI->explainReservedReg(*MF, RR)) {
-        MMI->getModule()->getContext().diagnose(DiagnosticInfoInlineAsm(
-            LocCookie, *reason, DiagnosticSeverity::DS_Note));
+        Ctx.diagnose(DiagnosticInfoInlineAsm(LocCookie, *reason,
+                                             DiagnosticSeverity::DS_Note));
       }
     }
   }

llvm/lib/CodeGen/MachineInstr.cpp

@@ -2219,26 +2219,36 @@ MachineInstrExpressionTrait::getHashValue(const MachineInstr* const &MI) {
   return hash_combine_range(HashComponents.begin(), HashComponents.end());
 }
 
-void MachineInstr::emitError(StringRef Msg) const {
+const MDNode *MachineInstr::getLocCookieMD() const {
   // Find the source location cookie.
-  uint64_t LocCookie = 0;
   const MDNode *LocMD = nullptr;
   for (unsigned i = getNumOperands(); i != 0; --i) {
     if (getOperand(i-1).isMetadata() &&
         (LocMD = getOperand(i-1).getMetadata()) &&
         LocMD->getNumOperands() != 0) {
-      if (const ConstantInt *CI =
-              mdconst::dyn_extract<ConstantInt>(LocMD->getOperand(0))) {
-        LocCookie = CI->getZExtValue();
-        break;
-      }
+      if (mdconst::hasa<ConstantInt>(LocMD->getOperand(0)))
+        return LocMD;
     }
   }
 
-  if (const MachineBasicBlock *MBB = getParent())
-    if (const MachineFunction *MF = MBB->getParent())
-      return MF->getFunction().getContext().emitError(LocCookie, Msg);
-  report_fatal_error(Msg);
+  return nullptr;
+}
+
+void MachineInstr::emitInlineAsmError(const Twine &Msg) const {
+  assert(isInlineAsm());
+  const MDNode *LocMD = getLocCookieMD();
+  uint64_t LocCookie =
+      LocMD
+          ? mdconst::extract<ConstantInt>(LocMD->getOperand(0))->getZExtValue()
+          : 0;
+  LLVMContext &Ctx = getMF()->getFunction().getContext();
+  Ctx.diagnose(DiagnosticInfoInlineAsm(LocCookie, Msg));
+}
+
+void MachineInstr::emitGenericError(const Twine &Msg) const {
+  const Function &Fn = getMF()->getFunction();
+  Fn.getContext().diagnose(
+      DiagnosticInfoGenericWithLoc(Msg, Fn, getDebugLoc()));
 }
 
 MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL,

llvm/lib/CodeGen/RegAllocBase.cpp

@@ -127,7 +127,8 @@ void RegAllocBase::allocatePhysRegs() {
       if (AllocOrder.empty())
         report_fatal_error("no registers from class available to allocate");
       else if (MI && MI->isInlineAsm()) {
-        MI->emitError("inline assembly requires more registers than available");
+        MI->emitInlineAsmError(
+            "inline assembly requires more registers than available");
       } else if (MI) {
         LLVMContext &Context =
             MI->getParent()->getParent()->getFunction().getContext();

llvm/lib/CodeGen/RegAllocFast.cpp

@@ -964,9 +964,10 @@ void RegAllocFastImpl::allocVirtReg(MachineInstr &MI, LiveReg &LR,
     // Nothing we can do: Report an error and keep going with an invalid
     // allocation.
     if (MI.isInlineAsm())
-      MI.emitError("inline assembly requires more registers than available");
+      MI.emitInlineAsmError(
+          "inline assembly requires more registers than available");
     else
-      MI.emitError("ran out of registers during register allocation");
+      MI.emitInlineAsmError("ran out of registers during register allocation");
 
     LR.Error = true;
     LR.PhysReg = 0;

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

@@ -319,13 +319,14 @@ getCopyFromParts(SelectionDAG &DAG, const SDLoc &DL, const SDValue *Parts,
 static void diagnosePossiblyInvalidConstraint(LLVMContext &Ctx, const Value *V,
                                               const Twine &ErrMsg) {
   const Instruction *I = dyn_cast_or_null<Instruction>(V);
-  if (!V)
+  if (!I)
     return Ctx.emitError(ErrMsg);
 
-  const char *AsmError = ", possible invalid constraint for vector type";
   if (const CallInst *CI = dyn_cast<CallInst>(I))
-    if (CI->isInlineAsm())
-      return Ctx.emitError(I, ErrMsg + AsmError);
+    if (CI->isInlineAsm()) {
+      return Ctx.diagnose(DiagnosticInfoInlineAsm(
+          *CI, ErrMsg + ", possible invalid constraint for vector type"));
+    }
 
   return Ctx.emitError(I, ErrMsg);
 }
@@ -10509,7 +10510,7 @@ void SelectionDAGBuilder::visitInlineAsm(const CallBase &Call,
 void SelectionDAGBuilder::emitInlineAsmError(const CallBase &Call,
                                              const Twine &Message) {
   LLVMContext &Ctx = *DAG.getContext();
-  Ctx.emitError(&Call, Message);
+  Ctx.diagnose(DiagnosticInfoInlineAsm(Call, Message));
 
   // Make sure we leave the DAG in a valid state
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();

llvm/lib/CodeGen/XRayInstrumentation.cpp

@@ -24,6 +24,7 @@
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/Attributes.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
@@ -211,8 +212,12 @@ bool XRayInstrumentation::runOnMachineFunction(MachineFunction &MF) {
   auto &FirstMI = *FirstMBB.begin();
 
   if (!MF.getSubtarget().isXRaySupported()) {
-    FirstMI.emitError("An attempt to perform XRay instrumentation for an"
-                      " unsupported target.");
+
+    const Function &Fn = FirstMBB.getParent()->getFunction();
+    Fn.getContext().diagnose(DiagnosticInfoUnsupported(
+        Fn, "An attempt to perform XRay instrumentation for an"
+            " unsupported target."));
+
     return false;
   }
 

llvm/lib/IR/DiagnosticInfo.cpp

@@ -48,6 +48,20 @@ int llvm::getNextAvailablePluginDiagnosticKind() {
 
 const char *OptimizationRemarkAnalysis::AlwaysPrint = "";
 
+void DiagnosticInfoGeneric::print(DiagnosticPrinter &DP) const {
+  DP << getMsgStr();
+}
+
+void DiagnosticInfoGenericWithLoc::print(DiagnosticPrinter &DP) const {
+  DP << getLocationStr() << ": " << getMsgStr();
+}
+
+DiagnosticInfoInlineAsm::DiagnosticInfoInlineAsm(uint64_t LocCookie,
+                                                 const Twine &MsgStr,
+                                                 DiagnosticSeverity Severity)
+    : DiagnosticInfo(DK_InlineAsm, Severity), LocCookie(LocCookie),
+      MsgStr(MsgStr) {}
+
 DiagnosticInfoInlineAsm::DiagnosticInfoInlineAsm(const Instruction &I,
                                                  const Twine &MsgStr,
                                                  DiagnosticSeverity Severity)

llvm/lib/IR/LLVMContext.cpp

@@ -219,12 +219,12 @@ void LLVMContext::yield() {
 }
 
 void LLVMContext::emitError(const Twine &ErrorStr) {
-  diagnose(DiagnosticInfoInlineAsm(ErrorStr));
+  diagnose(DiagnosticInfoGeneric(ErrorStr));
 }
 
 void LLVMContext::emitError(const Instruction *I, const Twine &ErrorStr) {
-  assert (I && "Invalid instruction");
-  diagnose(DiagnosticInfoInlineAsm(*I, ErrorStr));
+  assert(I && "Invalid instruction");
+  diagnose(DiagnosticInfoGeneric(I, ErrorStr));
 }
 
 static bool isDiagnosticEnabled(const DiagnosticInfo &DI) {
@@ -283,10 +283,6 @@ void LLVMContext::diagnose(const DiagnosticInfo &DI) {
     exit(1);
 }
 
-void LLVMContext::emitError(uint64_t LocCookie, const Twine &ErrorStr) {
-  diagnose(DiagnosticInfoInlineAsm(LocCookie, ErrorStr));
-}
-
 //===----------------------------------------------------------------------===//
 // Metadata Kind Uniquing
 //===----------------------------------------------------------------------===//