[AMDGPU] SelDAG: fix lowering of undefined workitem intrinsics #126058

ro-i · 2025-02-06T12:53:11Z

GlobalISel already handles undefined workitem.id.{x,y,z} intrinsics, SelDAG failed in AMDGPUISelLowering.cpp due to a failed assertion in AMDGPUTargetLowering::loadInputValue: Arg && "Attempting to load missing argument". This commit changes the behavior of SelDAG to instead use a zero constant.

This LLVM defect was identified via the AMD Fuzzing project.

GlobalISel already handles undefined workitem.id.{x,y,z} intrinsics, SelDAG failed in AMDGPUISelLowering.cpp due to a failed assertion in `AMDGPUTargetLowering::loadInputValue`: `Arg && "Attempting to load missing argument"`. This commit changes the behavior of SelDAG to instead use a zero constant. This LLVM defect was identified via the AMD Fuzzing project.

llvmbot · 2025-02-06T12:53:47Z

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-amdgpu

Author: Robert Imschweiler (ro-i)

Changes

GlobalISel already handles undefined workitem.id.{x,y,z} intrinsics, SelDAG failed in AMDGPUISelLowering.cpp due to a failed assertion in AMDGPUTargetLowering::loadInputValue: Arg && "Attempting to load missing argument". This commit changes the behavior of SelDAG to instead use a zero constant.

This LLVM defect was identified via the AMD Fuzzing project.

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

2 Files Affected:

(modified) llvm/lib/Target/AMDGPU/SIISelLowering.cpp (+20-3)
(added) llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-undefined-attr.ll (+36)

diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index b632c50dae0e359..47654890ff50e74 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -8790,11 +8790,28 @@ SDValue SITargetLowering::LowerINTRINSIC_WO_CHAIN(SDValue Op,
                              AMDGPUFunctionArgInfo::LDS_KERNEL_ID);
   }
   case Intrinsic::amdgcn_workitem_id_x:
-    return lowerWorkitemID(DAG, Op, 0, MFI->getArgInfo().WorkItemIDX);
+    if (!MFI->getArgInfo().WorkItemIDX) {
+      // It's undefined behavior if a function marked with the amdgpu-no-*
+      // attributes uses the corresponding intrinsic.
+      return DAG.getConstant(0, SDLoc(Op),
+                             EVT::getIntegerVT(*DAG.getContext(), 32));
+    } else {
+      return lowerWorkitemID(DAG, Op, 0, MFI->getArgInfo().WorkItemIDX);
+    }
   case Intrinsic::amdgcn_workitem_id_y:
-    return lowerWorkitemID(DAG, Op, 1, MFI->getArgInfo().WorkItemIDY);
+    if (!MFI->getArgInfo().WorkItemIDY) {
+      return DAG.getConstant(0, SDLoc(Op),
+                             EVT::getIntegerVT(*DAG.getContext(), 32));
+    } else {
+      return lowerWorkitemID(DAG, Op, 1, MFI->getArgInfo().WorkItemIDY);
+    }
   case Intrinsic::amdgcn_workitem_id_z:
-    return lowerWorkitemID(DAG, Op, 2, MFI->getArgInfo().WorkItemIDZ);
+    if (!MFI->getArgInfo().WorkItemIDZ) {
+      return DAG.getConstant(0, SDLoc(Op),
+                             EVT::getIntegerVT(*DAG.getContext(), 32));
+    } else {
+      return lowerWorkitemID(DAG, Op, 2, MFI->getArgInfo().WorkItemIDZ);
+    }
   case Intrinsic::amdgcn_wavefrontsize:
     return DAG.getConstant(MF.getSubtarget<GCNSubtarget>().getWavefrontSize(),
                            SDLoc(Op), MVT::i32);
diff --git a/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-undefined-attr.ll b/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-undefined-attr.ll
new file mode 100644
index 000000000000000..0e6a448fcae5a96
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-undefined-attr.ll
@@ -0,0 +1,36 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=amdgcn -mcpu=gfx942 -O0 --global-isel=false -o - %s | FileCheck %s
+
+declare i32 @llvm.amdgcn.workitem.id.x() #0
+declare i32 @llvm.amdgcn.workitem.id.y() #0
+declare i32 @llvm.amdgcn.workitem.id.z() #0
+
+define amdgpu_ps i32 @test_workitem_id_x() #1 {
+; CHECK-LABEL: test_workitem_id_x:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    s_mov_b32 s0, 0
+; CHECK-NEXT:    ; return to shader part epilog
+  %id = call i32 @llvm.amdgcn.workitem.id.x()
+  ret i32 %id
+}
+
+define amdgpu_ps i32 @test_workitem_id_y() #1 {
+; CHECK-LABEL: test_workitem_id_y:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    s_mov_b32 s0, 0
+; CHECK-NEXT:    ; return to shader part epilog
+  %id = call i32 @llvm.amdgcn.workitem.id.y()
+  ret i32 %id
+}
+
+define amdgpu_ps i32 @test_workitem_id_z() #1 {
+; CHECK-LABEL: test_workitem_id_z:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    s_mov_b32 s0, 0
+; CHECK-NEXT:    ; return to shader part epilog
+  %id = call i32 @llvm.amdgcn.workitem.id.z()
+  ret i32 %id
+}
+
+attributes #0 = { nounwind readnone }
+attributes #1 = { nounwind }

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-undefined-attr.ll

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

arsenm · 2025-02-07T11:14:04Z

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id.ll

+; UNDEF-LABEL: undefined_workitem_z_only:
+; UNDEF:       ; %bb.0:
+; UNDEF-NEXT:    s_endpgm
+  %id.z = call i32 @llvm.amdgcn.workitem.id.z()


This test isn't showing anything. It needs to have a real use of the value. Plus it doesn't have any of the attributes

I changed the tests and moved them to a separate file because they need to use a different calling convention than amdgpu_kernel so that the workitem.id registers are not defined. For the SelDAG case, I need -O0, otherwise everything is optimized away (which seems maybe a bit weird).

shiltian · 2025-02-07T20:48:50Z

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-unsupported-calling-convention.ll

+declare i32 @llvm.amdgcn.workitem.id.y()
+declare i32 @llvm.amdgcn.workitem.id.z()
+
+define amdgpu_ps void @undefined_workitems(ptr %p, ptr %q, ptr %r) {


I don't see this function is marked with amdgpu-no-* attribute. Is it implied by the CC?

I assumed so since:

GlobalISel behaves this way, too.

the docs for “amdgpu-no-workitem-id-x” say: "Indicates the function does not depend on the value of the llvm.amdgcn.workitem.id.x intrinsic. [...] The backend can generally infer this during code generation, so typically there is no benefit to frontends marking functions with this."

the (spare) docs on CC specify for amdgpu_ps, for example: "Used for Mesa/AMDPAL pixel shaders. ..TODO:: Describe." which led me to assume that it does not guarantee the Initial Kernel Execution State, as described for amdgpu_kernel.

shiltian

LGTM with one nit.

shiltian · 2025-02-12T16:33:59Z

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-unsupported-calling-convention.ll

@@ -0,0 +1,61 @@
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=amdgcn -mcpu=gfx942 -O0 -stop-after=si-fix-sgpr-copies -o - %s | FileCheck --check-prefix=SelDAG %s


Wonder why you want to stop it after si-fix-sgpr-copies instead of something earlier? I suppose right after amdgpu-isel would be sufficient.

works too (see my latest push) :) It just doesn't show the IMPLICIT_DEFs explicitly.

arsenm · 2025-02-13T08:17:13Z

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-unsupported-calling-convention.ll

+; RUN: llc -mtriple=amdgcn -mcpu=gfx942 -O0 -stop-after=amdgpu-isel -o - %s | FileCheck --check-prefix=SelDAG %s
+; RUN: llc -global-isel -mtriple=amdgcn -mcpu=gfx942 -stop-after=legalizer -o - %s | FileCheck --check-prefix=GlobalISel %s


Check prefixes should be all capitalized. MIR tests should be used as sparingly as possible, and in this case you should just run to the end and check the final ISA. If you were checking the MIR, these should check at a common point after selection

Ok, done. But then I only get an empty block (so %bb.0: s_endpgm). At least for GlobalIsel (no matter if -O0 or not) and also for SelDAG if I remove the -O0.

that's what you expect, the store of undef was deleted

Yes. Sorry, I guess I misunderstood what you said on another PR: that one should be able to see something directly in the tests. I've understood this to mean that one should be able to more or less explicitly see the undefs and not only see the resulting code where everything is optimized away.

arsenm · 2025-02-13T08:17:30Z

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id-unsupported-calling-convention.ll

+  ; GlobalISel-NEXT:   G_STORE [[DEF]](s32), [[MV2]](p0) :: (store (s32) into %ir.r)
+  ; GlobalISel-NEXT:   S_ENDPGM 0
+  %id.x = call i32 @llvm.amdgcn.workitem.id.x()
+  store i32 %id.x, ptr %p


use addrspace(1) pointers

done. What is the difference in this case?

Different instructions, but overall global is the simplest address space

ro-i · 2025-02-13T10:15:40Z

I pushed the changes to my existing branch. Should I make another PR?

arsenm · 2025-02-13T10:35:40Z

I pushed the changes to my existing branch. Should I make another PR?

Yes, can't do anything with the merged one

Follow-up fix for llvm#126058.

…126058) GlobalISel already handles undefined workitem.id.{x,y,z} intrinsics, SelDAG failed in AMDGPUISelLowering.cpp due to a failed assertion in `AMDGPUTargetLowering::loadInputValue`: `Arg && "Attempting to load missing argument"`. This commit changes the behavior of SelDAG to instead use a zero constant. This LLVM defect was identified via the AMD Fuzzing project.

@arsenm

#127041) Follow-up fix for #126058. (@arsenm)

…126058) GlobalISel already handles undefined workitem.id.{x,y,z} intrinsics, SelDAG failed in AMDGPUISelLowering.cpp due to a failed assertion in `AMDGPUTargetLowering::loadInputValue`: `Arg && "Attempting to load missing argument"`. This commit changes the behavior of SelDAG to instead use a zero constant. This LLVM defect was identified via the AMD Fuzzing project.

@arsenm

llvm#127041) Follow-up fix for llvm#126058. (@arsenm)

…126058) GlobalISel already handles undefined workitem.id.{x,y,z} intrinsics, SelDAG failed in AMDGPUISelLowering.cpp due to a failed assertion in `AMDGPUTargetLowering::loadInputValue`: `Arg && "Attempting to load missing argument"`. This commit changes the behavior of SelDAG to instead use a zero constant. This LLVM defect was identified via the AMD Fuzzing project.

@arsenm

llvm#127041) Follow-up fix for llvm#126058. (@arsenm)

llvmbot added the backend:AMDGPU label Feb 6, 2025

arsenm reviewed Feb 6, 2025

View reviewed changes

implement feedback

b921023

llvmbot added the llvm:globalisel label Feb 7, 2025

arsenm reviewed Feb 7, 2025

View reviewed changes

llvm/lib/Target/AMDGPU/SIISelLowering.cpp Outdated Show resolved Hide resolved

arsenm reviewed Feb 7, 2025

View reviewed changes

implement feedback

6ee6adb

shiltian reviewed Feb 7, 2025

View reviewed changes

shiltian approved these changes Feb 12, 2025

View reviewed changes

implement feedback

7fed314

shiltian merged commit bcba311 into llvm:main Feb 12, 2025
8 checks passed

arsenm reviewed Feb 13, 2025

View reviewed changes

ro-i added a commit to ro-i/llvm-project that referenced this pull request Feb 13, 2025

[AMDGPU] Fix llvm.amdgcn.workitem.id-unsupported-calling-convention.ll

c8f6769

Follow-up fix for llvm#126058.

ro-i mentioned this pull request Feb 13, 2025

[AMDGPU] Fix llvm.amdgcn.workitem.id-unsupported-calling-convention.ll #127041

Merged

arsenm pushed a commit that referenced this pull request Feb 13, 2025

[AMDGPU] Fix llvm.amdgcn.workitem.id-unsupported-calling-convention.ll (

41e49fa

#127041) Follow-up fix for #126058. (@arsenm)

joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025

[AMDGPU] Fix llvm.amdgcn.workitem.id-unsupported-calling-convention.ll (

fb17edb

llvm#127041) Follow-up fix for llvm#126058. (@arsenm)

sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025

[AMDGPU] Fix llvm.amdgcn.workitem.id-unsupported-calling-convention.ll (

bdf1e5e

llvm#127041) Follow-up fix for llvm#126058. (@arsenm)

		@@ -0,0 +1,61 @@
		; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
		; RUN: llc -mtriple=amdgcn -mcpu=gfx942 -O0 -stop-after=si-fix-sgpr-copies -o - %s \| FileCheck --check-prefix=SelDAG %s

		; RUN: llc -mtriple=amdgcn -mcpu=gfx942 -O0 -stop-after=amdgpu-isel -o - %s \| FileCheck --check-prefix=SelDAG %s
		; RUN: llc -global-isel -mtriple=amdgcn -mcpu=gfx942 -stop-after=legalizer -o - %s \| FileCheck --check-prefix=GlobalISel %s

[AMDGPU] SelDAG: fix lowering of undefined workitem intrinsics #126058

[AMDGPU] SelDAG: fix lowering of undefined workitem intrinsics #126058

Uh oh!

Conversation

ro-i commented Feb 6, 2025

Uh oh!

llvmbot commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shiltian left a comment

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!

arsenm Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

ro-i commented Feb 13, 2025

Uh oh!

arsenm commented Feb 13, 2025

Uh oh!

Uh oh!

llvmbot commented Feb 6, 2025 •

edited

Loading

ro-i Feb 7, 2025 •

edited

Loading

arsenm Feb 13, 2025 •

edited

Loading