(reland) [GlobalISel] Diagnose inline assembly constraint lowering errors

[Pierre-vh]

The initial patch (#135782 caused issues because it emits an error, and llc is sensitive to it.
It also caused compiler-rt/lib/scudo/standalone/tests/wrappers_cpp_test.cpp to fail.

Use warnings instead + reject lowering. That way, the fallback path is used without llc/clang returning a failure code.
If fallback isn't enabled then the warnings provide context as to why lowering failed.

Original commit description for #135782:

Instead of printing something to dbgs (which is not visible to all users),
emit a diagnostic like the DAG does. We still crash later because we fail to
select the inline assembly, but at least now users will know why it's crashing.

In a future patch we could also recover from the error like the DAG does, so the
lowering can keep going until it either crashes or gives a different error later.

[Pierre-vh]

• (reland) [GlobalISel] Diagnose inline assembly constraint lowering errors #139049 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-aarch64

Author: Pierre van Houtryve (Pierre-vh)

Changes

The initial patch (#135782 caused issues because it emits an error, and llc is sensitive to it.
It also caused compiler-rt/lib/scudo/standalone/tests/wrappers_cpp_test.cpp to fail.

Use warnings instead + reject lowering. That way, the fallback path is used without llc/clang returning a failure code.
If fallback isn't enabled then the warnings provide context as to why lowering failed.

Original commit description for #135782:

Instead of printing something to dbgs (which is not visible to all users),
emit a diagnostic like the DAG does. We still crash later because we fail to
select the inline assembly, but at least now users will know why it's crashing.

In a future patch we could also recover from the error like the DAG does, so the
lowering can keep going until it either crashes or gives a different error later.

---

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

3 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp (+41-33)
• (modified) llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll (+2-1)
• (added) llvm/test/CodeGen/AMDGPU/GlobalISel/inline-asm-lowering-diags.ll (+30)

diff --git a/llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp b/llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
index 81f25b21a0409..ec3fc47e82b9a 100644
--- a/llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
@@ -16,6 +16,7 @@
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetLowering.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Module.h"
 
 #define DEBUG_TYPE "inline-asm-lowering"
@@ -231,6 +232,19 @@ bool InlineAsmLowering::lowerInlineAsm(
   TargetLowering::AsmOperandInfoVector TargetConstraints =
       TLI->ParseConstraints(DL, TRI, Call);
 
+  const auto ConstraintError = [&](const GISelAsmOperandInfo &Info, Twine Msg) {
+    // Use warnings in combination with a "return false" to trigger the fallback
+    // path. If fallback isn't enabled, then another error will be emitted later
+    // and the warnings will provide context as to why the error occured.
+    LLVMContext &Ctx = MIRBuilder.getContext();
+    Ctx.diagnose(DiagnosticInfoInlineAsm(
+        Call, "invalid constraint '" + Info.ConstraintCode + "': " + Msg,
+        DS_Warning));
+    // TODO: If we could detect that the fallback isn't enabled, we could
+    // recover here by defining all result registers as G_IMPLICIT_DEF.
+    return false;
+  };
+
   ExtraFlags ExtraInfo(Call);
   unsigned ArgNo = 0; // ArgNo - The argument of the CallInst.
   unsigned ResNo = 0; // ResNo - The result number of the next output.
@@ -243,8 +257,8 @@ bool InlineAsmLowering::lowerInlineAsm(
       OpInfo.CallOperandVal = const_cast<Value *>(Call.getArgOperand(ArgNo));
 
       if (isa<BasicBlock>(OpInfo.CallOperandVal)) {
-        LLVM_DEBUG(dbgs() << "Basic block input operands not supported yet\n");
-        return false;
+        return ConstraintError(OpInfo,
+                               "basic block input operands not supported yet");
       }
 
       Type *OpTy = OpInfo.CallOperandVal->getType();
@@ -258,9 +272,8 @@ bool InlineAsmLowering::lowerInlineAsm(
 
       // FIXME: Support aggregate input operands
       if (!OpTy->isSingleValueType()) {
-        LLVM_DEBUG(
-            dbgs() << "Aggregate input operands are not supported yet\n");
-        return false;
+        return ConstraintError(OpInfo,
+                               "aggregate input operands not supported yet");
       }
 
       OpInfo.ConstraintVT =
@@ -344,9 +357,8 @@ bool InlineAsmLowering::lowerInlineAsm(
 
         // Find a register that we can use.
         if (OpInfo.Regs.empty()) {
-          LLVM_DEBUG(dbgs()
-                     << "Couldn't allocate output register for constraint\n");
-          return false;
+          return ConstraintError(
+              OpInfo, "could not allocate output register for constraint");
         }
 
         // Add information to the INLINEASM instruction to know that this
@@ -389,13 +401,13 @@ bool InlineAsmLowering::lowerInlineAsm(
 
         const InlineAsm::Flag MatchedOperandFlag(Inst->getOperand(InstFlagIdx).getImm());
         if (MatchedOperandFlag.isMemKind()) {
-          LLVM_DEBUG(dbgs() << "Matching input constraint to mem operand not "
-                               "supported. This should be target specific.\n");
-          return false;
+          return ConstraintError(
+              OpInfo,
+              "matching input constraint to mem operand not supported; this "
+              "should be target specific");
         }
         if (!MatchedOperandFlag.isRegDefKind() && !MatchedOperandFlag.isRegDefEarlyClobberKind()) {
-          LLVM_DEBUG(dbgs() << "Unknown matching constraint\n");
-          return false;
+          return ConstraintError(OpInfo, "unknown matching constraint");
         }
 
         // We want to tie input to register in next operand.
@@ -425,9 +437,10 @@ bool InlineAsmLowering::lowerInlineAsm(
 
       if (OpInfo.ConstraintType == TargetLowering::C_Other &&
           OpInfo.isIndirect) {
-        LLVM_DEBUG(dbgs() << "Indirect input operands with unknown constraint "
-                             "not supported yet\n");
-        return false;
+        return ConstraintError(
+            OpInfo,
+            "indirect input operands with unknown constraint not supported "
+            "yet");
       }
 
       if (OpInfo.ConstraintType == TargetLowering::C_Immediate ||
@@ -437,9 +450,7 @@ bool InlineAsmLowering::lowerInlineAsm(
         if (!lowerAsmOperandForConstraint(OpInfo.CallOperandVal,
                                           OpInfo.ConstraintCode, Ops,
                                           MIRBuilder)) {
-          LLVM_DEBUG(dbgs() << "Don't support constraint: "
-                            << OpInfo.ConstraintCode << " yet\n");
-          return false;
+          return ConstraintError(OpInfo, "unsupported constraint");
         }
 
         assert(Ops.size() > 0 &&
@@ -456,9 +467,8 @@ bool InlineAsmLowering::lowerInlineAsm(
       if (OpInfo.ConstraintType == TargetLowering::C_Memory) {
 
         if (!OpInfo.isIndirect) {
-          LLVM_DEBUG(dbgs()
-                     << "Cannot indirectify memory input operands yet\n");
-          return false;
+          return ConstraintError(
+              OpInfo, "indirect memory input operands are not supported yet");
         }
 
         assert(OpInfo.isIndirect && "Operand must be indirect to be a mem!");
@@ -482,18 +492,15 @@ bool InlineAsmLowering::lowerInlineAsm(
              "Unknown constraint type!");
 
       if (OpInfo.isIndirect) {
-        LLVM_DEBUG(dbgs() << "Can't handle indirect register inputs yet "
-                             "for constraint '"
-                          << OpInfo.ConstraintCode << "'\n");
-        return false;
+        return ConstraintError(
+            OpInfo, "indirect register inputs are not supported yet");
       }
 
       // Copy the input into the appropriate registers.
       if (OpInfo.Regs.empty()) {
-        LLVM_DEBUG(
-            dbgs()
-            << "Couldn't allocate input register for register constraint\n");
-        return false;
+        return ConstraintError(
+            OpInfo,
+            "could not allocate input register for register constraint");
       }
 
       unsigned NumRegs = OpInfo.Regs.size();
@@ -503,9 +510,10 @@ bool InlineAsmLowering::lowerInlineAsm(
              "source registers");
 
       if (NumRegs > 1) {
-        LLVM_DEBUG(dbgs() << "Input operands with multiple input registers are "
-                             "not supported yet\n");
-        return false;
+        return ConstraintError(
+            OpInfo,
+            "input operands with multiple input registers are not supported "
+            "yet");
       }
 
       InlineAsm::Flag Flag(InlineAsm::Kind::RegUse, NumRegs);
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll b/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll
index 29c320da6c0a7..544c873882ed9 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll
@@ -1,7 +1,8 @@
-; RUN: llc -O0 -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' -verify-machineinstrs %s -o %t.out 2> %t.err
+; RUN: llc -O0 -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' -verify-machineinstrs %s -o - 2> %t.err
 ; RUN: FileCheck %s --check-prefix=FALLBACK-WITH-REPORT-OUT < %t.out
 ; RUN: FileCheck %s --check-prefix=FALLBACK-WITH-REPORT-ERR < %t.err
 ; RUN: not --crash llc -global-isel -mtriple aarch64_be %s -o - 2>&1 | FileCheck %s --check-prefix=BIG-ENDIAN
+
 ; This file checks that the fallback path to selection dag works.
 ; The test is fragile in the sense that it must be updated to expose
 ; something that fails with global-isel.
diff --git a/llvm/test/CodeGen/AMDGPU/GlobalISel/inline-asm-lowering-diags.ll b/llvm/test/CodeGen/AMDGPU/GlobalISel/inline-asm-lowering-diags.ll
new file mode 100644
index 0000000000000..897e281521966
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/GlobalISel/inline-asm-lowering-diags.ll
@@ -0,0 +1,30 @@
+; RUN: not llc -mtriple=amdgcn -mcpu=fiji -O0 -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' %s -o - 2>&1 | FileCheck %s
+
+; CHECK: warning: invalid constraint '': aggregate input operands not supported yet
+define amdgpu_kernel void @aggregates([4 x i8] %val) {
+  tail call void asm sideeffect "s_nop", "r"([4 x i8] %val)
+  ret void
+}
+
+; CHECK: warning: invalid constraint '{s999}': could not allocate output register for constraint
+define amdgpu_kernel void @bad_output() {
+  tail call i32 asm sideeffect "s_nop", "={s999}"()
+  ret void
+}
+
+; CHECK: warning: invalid constraint '{s998}': could not allocate input register for register constraint
+define amdgpu_kernel void @bad_input() {
+  tail call void asm sideeffect "s_nop", "{s998}"(i32 poison)
+  ret void
+}
+; CHECK: warning: invalid constraint '{s997}': indirect register inputs are not supported yet
+define amdgpu_kernel void @indirect_input() {
+  tail call void asm sideeffect "s_nop", "*{s997}"(ptr elementtype(i32) poison)
+  ret void
+}
+
+; CHECK: warning: invalid constraint 'i': unsupported constraint
+define amdgpu_kernel void @badimm() {
+  tail call void asm sideeffect "s_nop", "i"(i32 poison)
+  ret void
+}

[arsenm]

This is broken, t.out is no longer written

[Pierre-vh]

Merge activity

• May 8, 7:21 AM EDT: A user started a stack merge that includes this pull request via Graphite.
• May 8, 7:22 AM EDT: @Pierre-vh merged this pull request with Graphite.

[Pierre-vh]

@vitalybuka The sanitizer aarch64 buildbot keeps failing on this. It appears to build with globalisel, -Werr and it uses an unsupported constraint in its inline asm.

This is just exposing a bug in the testcase IMO. I'd like to avoid reverting the patch again. How can we proceed?