From 2d57dec58c2d7e30c93856f61870abcbf73ef784 Mon Sep 17 00:00:00 2001 From: pvanhout Date: Thu, 8 May 2025 10:03:17 +0200 Subject: [PATCH 1/3] (reland) [GlobalISel] Diagnose inline assembly constraint lowering errors (#135782) The initial patch 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: --- 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. --- .../CodeGen/GlobalISel/InlineAsmLowering.cpp | 74 ++++++++++--------- .../AArch64/GlobalISel/arm64-fallback.ll | 3 +- .../GlobalISel/inline-asm-lowering-diags.ll | 30 ++++++++ 3 files changed, 73 insertions(+), 34 deletions(-) create mode 100644 llvm/test/CodeGen/AMDGPU/GlobalISel/inline-asm-lowering-diags.ll 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(Call.getArgOperand(ArgNo)); if (isa(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 +} From 693434d627709006d5688f9cc3eeb124e87e6eb6 Mon Sep 17 00:00:00 2001 From: pvanhout Date: Thu, 8 May 2025 11:06:29 +0200 Subject: [PATCH 2/3] Write t.out --- llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll b/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll index 544c873882ed9..5be74fd832af1 100644 --- a/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll +++ b/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll @@ -1,4 +1,4 @@ -; RUN: llc -O0 -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' -verify-machineinstrs %s -o - 2> %t.err +; RUN: llc -O0 -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' -verify-machineinstrs %s -o %t.out 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 From 638df60f810d74695c0ff6099ebd0b8b20972d14 Mon Sep 17 00:00:00 2001 From: pvanhout Date: Thu, 8 May 2025 11:14:26 +0200 Subject: [PATCH 3/3] remove whitespace change --- llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll | 1 - 1 file changed, 1 deletion(-) diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll b/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll index 5be74fd832af1..29c320da6c0a7 100644 --- a/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll +++ b/llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll @@ -2,7 +2,6 @@ ; 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.