[AMDGPU] Change handling of unsupported non-compute shaders with HSA #126798

ro-i · 2025-02-11T21:32:27Z

Previous handling in SITargetLowering::LowerFormalArguments only reported a diagnostic message and continued execution by returning a non-usable SDValue. This results in llvm crashing later with an unrelated error. This commit changes the detection of an unsupported non-compute shader to be a fatal error right away.

As an example situation, take the usage of an amdgpu_ps function and the amdgcn-unknown-amdhsa target triple.

define amdgpu_ps void @foo(ptr %p, i32 %i) {
        store i32 %i, ptr %p
        ret void
}

Compiling this code (with llc -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx942, for example) fails with:

error: <unknown>:0:0: in function foo void (ptr, i32): unsupported non-compute shaders with HSA

llc:
[...]/git/trunk21.0/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11790:
void llvm::SelectionDAGISel::LowerArguments(const llvm::Function&):
Assertion `InVals.size() == Ins.size() && "LowerFormalArguments didn't emit the correct number of values!"' failed.
[...]

Previous handling in `SITargetLowering::LowerFormalArguments` only reported a diagnostic message and continued execution by returning a non-usable `SDValue`. This results in llvm crashing later with an unrelated error. This commit changes the detection of an unsupported non-compute shader to be a fatal error right away.

llvmbot · 2025-02-11T21:33:04Z

@llvm/pr-subscribers-backend-amdgpu

Author: Robert Imschweiler (ro-i)

Changes

Previous handling in SITargetLowering::LowerFormalArguments only reported a diagnostic message and continued execution by returning a non-usable SDValue. This results in llvm crashing later with an unrelated error. This commit changes the detection of an unsupported non-compute shader to be a fatal error right away.

As an example situation, take the usage of an amdgpu_ps function and the amdgcn-unknown-amdhsa target triple.

define amdgpu_ps void @<!-- -->foo(ptr %p, i32 %i) {
        store i32 %i, ptr %p
        ret void
}

Compiling this code (with llc -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx942, for example) fails with:

error: &lt;unknown&gt;:0:0: in function foo void (ptr, i32): unsupported non-compute shaders with HSA

llc:
[...]/git/trunk21.0/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11790:
void llvm::SelectionDAGISel::LowerArguments(const llvm::Function&amp;):
Assertion `InVals.size() == Ins.size() &amp;&amp; "LowerFormalArguments didn't emit the correct number of values!"' failed.
[...]

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

2 Files Affected:

(modified) llvm/lib/Target/AMDGPU/SIISelLowering.cpp (+1-4)
(removed) llvm/test/CodeGen/AMDGPU/no-hsa-graphics-shaders.ll (-19)

diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index b632c50dae0e3..746bde4de5e9f 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -2824,10 +2824,7 @@ SDValue SITargetLowering::LowerFormalArguments(
   SIMachineFunctionInfo *Info = MF.getInfo<SIMachineFunctionInfo>();
 
   if (Subtarget->isAmdHsaOS() && AMDGPU::isGraphics(CallConv)) {
-    DiagnosticInfoUnsupported NoGraphicsHSA(
-        Fn, "unsupported non-compute shaders with HSA", DL.getDebugLoc());
-    DAG.getContext()->diagnose(NoGraphicsHSA);
-    return DAG.getEntryNode();
+    report_fatal_error("unsupported non-compute shaders with HSA");
   }
 
   SmallVector<ISD::InputArg, 16> Splits;
diff --git a/llvm/test/CodeGen/AMDGPU/no-hsa-graphics-shaders.ll b/llvm/test/CodeGen/AMDGPU/no-hsa-graphics-shaders.ll
deleted file mode 100644
index ee6a578c72859..0000000000000
--- a/llvm/test/CodeGen/AMDGPU/no-hsa-graphics-shaders.ll
+++ /dev/null
@@ -1,19 +0,0 @@
-; RUN: not llc -mtriple=amdgcn-unknown-amdhsa < %s 2>&1 | FileCheck %s
-
-; CHECK: in function pixel_s{{.*}}: unsupported non-compute shaders with HSA
-define amdgpu_ps void @pixel_shader() #0 {
-  ret void
-}
-
-; CHECK: in function vertex_s{{.*}}: unsupported non-compute shaders with HSA
-define amdgpu_vs void @vertex_shader() #0 {
-  ret void
-}
-
-; CHECK: in function geometry_s{{.*}}: unsupported non-compute shaders with HSA
-define amdgpu_gs void @geometry_shader() #0 {
-  ret void
-}
-
-!llvm.module.flags = !{!0}
-!0 = !{i32 1, !"amdhsa_code_object_version", i32 400}

arsenm · 2025-02-12T05:19:13Z

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

-        Fn, "unsupported non-compute shaders with HSA", DL.getDebugLoc());
-    DAG.getContext()->diagnose(NoGraphicsHSA);
-    return DAG.getEntryNode();
+    report_fatal_error("unsupported non-compute shaders with HSA");


This should not be a fatal error. You should adjust the return value to be the correct type by using the correct set of undef values

What is the point? There will be no valid output anyways. Trying to validate even invalid output takes a lot of effort and will ityself be fragile because that code is rarely executed/tested.

arsenm · 2025-02-12T05:19:21Z

llvm/test/CodeGen/AMDGPU/no-hsa-graphics-shaders.ll

@@ -1,19 +0,0 @@
-; RUN: not llc -mtriple=amdgcn-unknown-amdhsa < %s 2>&1 | FileCheck %s
-
-; CHECK: in function pixel_s{{.*}}: unsupported non-compute shaders with HSA


We must maintain test coverage for error cases

I wanted to adapt the test, but FileCheck didn't seem to be able to check for the LLVM ERROR ... line (first line of output/stderr). So I assumed that it may not be possible to check for fatal errors. But now the situation is different anyway :)

Did you try not --crash?

Yeah, in the meantime, I found this by searching through similar tests (basically grep -rl "LLVM ERROR" ^^). However, it still somehow skipped the first line, even though stderr was redirected to stdout. But this is no longer an issue since I removed the fatal error again, based on the feedback I received here.

shiltian · 2025-02-12T06:16:29Z

My .02 is, we should not fix all fuzzer issues, even though they might crash the compiler. When we run certain pass, it is safe to assume certain things, but fuzzer breaks these assumptions on purpose by directly feeding the pass with broken input. Those errors can't reach the pass if the input runs through a pipeline. For them, I think we should capture them in verifier instead of making every single pass to do certain jobs of the verifier, which doesn't make too much sense to me.

arsenm · 2025-02-12T06:22:26Z

My .02 is, we should not fix all fuzzer issues, even though they might crash the compiler. When we run certain pass, it is safe to assume certain things, but fuzzer breaks these assumptions on purpose by directly feeding the pass with broken input. Those errors can't reach the pass if the input runs through a pipeline. For them, I think we should capture them in verifier instead of making every single pass to do certain jobs of the verifier, which doesn't make too much sense to me.

This one is actually really super annoying and we should fix it properly. This also doesn't really belong in the IR verifier.

ro-i · 2025-02-12T13:34:48Z

This one is actually really super annoying and we should fix it properly.

I pushed some changes. Do you think they are going in the right direction?

Edit: annoying because it shouldn't be fixed? Or annoying because it is worth fixing but tricky?

github-actions · 2025-02-12T13:35:03Z

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

shiltian · 2025-02-12T14:10:04Z

This one is actually really super annoying and we should fix it properly. This also doesn't really belong in the IR verifier.

My reading is, amdgpu_ps functions should not exist for amdhsa OS. If it does, the input IR is broken for amdhsa. The IR itself might not be really invalid, but just under certain circumstances. I'm not very sure that whether IR verifier is supposed to be target agnostic. For example, is the IR still valid (from the verifier's perspective) if I invoke opt or llc with a x86 IR (assume the IR has some x86 dedicated stuff) but the target triple is amdgcn?

arsenm · 2025-02-12T16:43:41Z

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

@@ -2933,7 +2937,7 @@ SDValue SITargetLowering::LowerFormalArguments(

  for (unsigned i = 0, e = Ins.size(), ArgIdx = 0; i != e; ++i) {
    const ISD::InputArg &Arg = Ins[i];
-    if (Arg.isOrigArg() && Skipped[Arg.getOrigArgIndex()]) {
+    if ((Arg.isOrigArg() && Skipped[Arg.getOrigArgIndex()]) || IsUnsupportedHsa) {


If you want to keep this as it is with the separated handling down here, I'd go with a broader IsError or something.
However, I think it would be cleaner to just have a separate loop populating InVals in the error case with the undefs

However, I think it would be cleaner to just have a separate loop populating InVals in the error case with the undefs

When I do this:

if (Subtarget->isAmdHsaOS() && AMDGPU::isGraphics(CallConv)) { DiagnosticInfoUnsupported NoGraphicsHSA( Fn, "unsupported non-compute shaders with HSA", DL.getDebugLoc()); DAG.getContext()->diagnose(NoGraphicsHSA); for (unsigned i = 0, e = Ins.size(); i != e; ++i) { const ISD::InputArg &Arg = Ins[i]; InVals.push_back(DAG.getUNDEF(Arg.VT)); } return Chain; }

I get the following error later:
llc: ~/git/trunk21.0/llvm-project/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:945: void llvm::SIMachineFunctionInfo::setScratchRSrcReg(llvm::Register): Assertion `Reg != 0 && "Should never be unset"' failed.
I suspect the problem is that the other initialization stuff isn't done because of the early return.

Or are you thinking about something else?

Oh right, the other special input setup stuff still needs to happen. I suppose you can rename the variable

Although in that case we should stop requiring the CC lowering to set up the scratch resource. It's really a fixed constant

Oh right, the other special input setup stuff still needs to happen. I suppose you can rename the variable

Already done :)

Although in that case we should stop requiring the CC lowering to set up the scratch resource. It's really a fixed constant

Hm, the function seems to do a lot of things (which I don't fully understand yet) in the case of IsGraphics and IsEntryFunc. Is it possible to pull all this into the error handling without too much code duplication?

llvm/test/CodeGen/AMDGPU/no-hsa-graphics-shaders.ll

@foo

…lvm#126798) Previous handling in `SITargetLowering::LowerFormalArguments` only reported a diagnostic message and continued execution by returning a non-usable `SDValue`. This results in llvm crashing later with an unrelated error. This commit changes the detection of an unsupported non-compute shader to be a fatal error right away. As an example situation, take the usage of an `amdgpu_ps` function and the `amdgcn-unknown-amdhsa` target triple. ``` define amdgpu_ps void @foo(ptr %p, i32 %i) { store i32 %i, ptr %p ret void } ``` Compiling this code (with `llc -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx942`, for example) fails with: ``` error: <unknown>:0:0: in function foo void (ptr, i32): unsupported non-compute shaders with HSA llc: [...]/git/trunk21.0/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11790: void llvm::SelectionDAGISel::LowerArguments(const llvm::Function&): Assertion `InVals.size() == Ins.size() && "LowerFormalArguments didn't emit the correct number of values!"' failed. [...] ```

@foo

…lvm#126798) Previous handling in `SITargetLowering::LowerFormalArguments` only reported a diagnostic message and continued execution by returning a non-usable `SDValue`. This results in llvm crashing later with an unrelated error. This commit changes the detection of an unsupported non-compute shader to be a fatal error right away. As an example situation, take the usage of an `amdgpu_ps` function and the `amdgcn-unknown-amdhsa` target triple. ``` define amdgpu_ps void @foo(ptr %p, i32 %i) { store i32 %i, ptr %p ret void } ``` Compiling this code (with `llc -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx942`, for example) fails with: ``` error: <unknown>:0:0: in function foo void (ptr, i32): unsupported non-compute shaders with HSA llc: [...]/git/trunk21.0/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11790: void llvm::SelectionDAGISel::LowerArguments(const llvm::Function&): Assertion `InVals.size() == Ins.size() && "LowerFormalArguments didn't emit the correct number of values!"' failed. [...] ```

llvmbot added the backend:AMDGPU label Feb 11, 2025

arsenm reviewed Feb 12, 2025

View reviewed changes

implement feedback

891ba84

fix formatting

61161b8

arsenm reviewed Feb 12, 2025

View reviewed changes

llvm/test/CodeGen/AMDGPU/no-hsa-graphics-shaders.ll Outdated Show resolved Hide resolved

llvm/test/CodeGen/AMDGPU/no-hsa-graphics-shaders.ll Outdated Show resolved Hide resolved

implement feedback

a221782

arsenm approved these changes Feb 13, 2025

View reviewed changes

arsenm merged commit 0da8d0f into llvm:main Feb 13, 2025
8 checks passed

ro-i deleted the unsupported-hsa-fatal branch February 21, 2025 20:10

		@@ -1,19 +0,0 @@
		; RUN: not llc -mtriple=amdgcn-unknown-amdhsa < %s 2>&1 \| FileCheck %s

		; CHECK: in function pixel_s{{.*}}: unsupported non-compute shaders with HSA

[AMDGPU] Change handling of unsupported non-compute shaders with HSA #126798

[AMDGPU] Change handling of unsupported non-compute shaders with HSA #126798

Uh oh!

Conversation

ro-i commented Feb 11, 2025

Uh oh!

llvmbot commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shiltian commented Feb 12, 2025

Uh oh!

arsenm commented Feb 12, 2025

Uh oh!

ro-i commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiltian commented Feb 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ro-i commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading