Reland "[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR." #135056

diggerlin · 2025-04-09T17:32:34Z

A new ISD::POISON SDNode is introduced to represent the poison value in the IR, replacing the previous use of ISD::UNDEF

llvmbot · 2025-04-09T17:33:10Z

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-backend-powerpc

@llvm/pr-subscribers-llvm-selectiondag

Author: zhijian lin (diggerlin)

Changes

The PR fix a assert on the PR #125883

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

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp (+1)
(added) llvm/test/CodeGen/RISCV/pr125883.ll (+19)

diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
index 5ed83060e150e..6493e0e6583f0 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
@@ -165,6 +165,7 @@ void DAGTypeLegalizer::SoftenFloatResult(SDNode *N, unsigned ResNo) {
     case ISD::STRICT_UINT_TO_FP:
     case ISD::SINT_TO_FP:
     case ISD::UINT_TO_FP:  R = SoftenFloatRes_XINT_TO_FP(N); break;
+    case ISD::POISON:
     case ISD::UNDEF:       R = SoftenFloatRes_UNDEF(N); break;
     case ISD::VAARG:       R = SoftenFloatRes_VAARG(N); break;
     case ISD::VECREDUCE_FADD:
diff --git a/llvm/test/CodeGen/RISCV/pr125883.ll b/llvm/test/CodeGen/RISCV/pr125883.ll
new file mode 100644
index 0000000000000..f2da05c660be6
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/pr125883.ll
@@ -0,0 +1,19 @@
+; RUN: llc < %s -mtriple=riscv32  | FileCheck %s
+
+define void @b(ptr %p1)  {
+; CHECK:        .cfi_startproc
+; CHECK-NEXT: # %bb.0:                                # %entry
+; CHECK-NEXT:   sb      zero, 7(a0)
+; CHECK-NEXT:   sb      zero, 6(a0)
+; CHECK-NEXT:   sb      zero, 5(a0)
+; CHECK-NEXT:   sb      a0, 4(a0)
+; CHECK-NEXT:   sb      zero, 3(a0)
+; CHECK-NEXT:   sb      zero, 2(a0)
+; CHECK-NEXT:   sb      zero, 1(a0)
+; CHECK-NEXT:   sb      a0, 0(a0)
+; CHECK-NEXT:   ret
+
+entry:
+  store volatile double poison, ptr %p1, align 1
+  ret void
+}

topperc · 2025-04-09T17:37:54Z

The title of this PR does not follow established conventions for commits in LLVM.

nikic

Can you please also fix other cases in this file? Like SoftPromoteHalfRes for example.

Done

RKSimon · 2025-04-09T17:47:06Z

llvm/test/CodeGen/RISCV/pr135056.ll

@@ -0,0 +1,19 @@
+; RUN: llc < %s -mtriple=riscv32  | FileCheck %s


Incorrect filename - prXXXXX.ll is for Problem Reports (a very old name) - which now map to issue numbers - don't use pull request numbers here

I do not think there is an issue the fix, Can I use PR125883?

@diggerlin you seem to have edited Simon's message instead of replying.

You should try to give a descriptive name. Like poison-legalization.ll

arsenm · 2025-04-09T17:47:31Z

Title should be what it functionally fixes

kuhar

This doesn't fix the downstream crash I reported in #125883 (comment)

topperc · 2025-04-09T17:59:49Z

The reported crash came from SoftPromoteHalfResult not from SoftenFloatResult

arsenm · 2025-04-09T18:03:10Z

llvm/test/CodeGen/RISCV/pr135056.ll

+entry:
+  store volatile double poison, ptr %p1, align 1
+  ret void
+}


Test vector cases?

Test vector cases?

I do not think we need a vector test case, The test case has cover the code we changed.

arsenm · 2025-04-09T18:03:33Z

llvm/test/CodeGen/RISCV/pr135056.ll

+; CHECK-NEXT:   ret
+
+entry:
+  store volatile double poison, ptr %p1, align 1


unaligned store not necessary for the test?

diggerlin · 2025-04-09T18:11:03Z

This doesn't fix the downstream crash I reported in #125883 (comment)

can you give me the reproduce test case , I will add the test case into the PR.

arichardson · 2025-04-09T18:21:14Z

This doesn't fix the downstream crash I reported in #125883 (comment)

can you give me the reproduce test case , I will add the test case into the PR.

Shouldn't that just be the same test case but with "half"?

diggerlin · 2025-04-09T19:06:10Z

This doesn't fix the downstream crash I reported in #125883 (comment)

Can you help to try it now ? @kuhar

arichardson · 2025-04-09T20:12:53Z

llvm/test/CodeGen/RISCV/poison-legalization.ll

+define void @b(ptr %p1, ptr %p2)  {
+; CHECK:        .cfi_startproc


Suggested change

define void @b(ptr %p1, ptr %p2) {

; CHECK: .cfi_startproc

define void @b(ptr %p1, ptr %p2) nounwind {

That should remove the .cfi_startproc.

Also I'd suggest one function for each type?

arichardson

I would suggest using update_llc_test_checks.py (and adding nounwind to ensure it doesn't add the cfi information), otherwise this change LGTM.

But since I'm not a SelectionDAG expert please wait for someone else to approve too.

kuhar

Thanks for looking into this.

Now that the base PR is reverted, can you pull in the original change from #125883 to reland this?

diggerlin · 2025-04-10T01:48:12Z

Thanks for looking into this.

Now that the base PR is reverted, can you pull in the original change from #125883 to reland this?

I think we can revert your revert commit and then land the PR after the PR approved.

kuhar · 2025-04-10T02:05:26Z

Reverting the revert would get us back into a broken state. We should reland the original PR with this fix as one commit.

…poison value in the IR. (llvm#125883) A new ISD::POISON SDNode is introduced to represent the `poison value` in the IR, replacing the previous use of ISD::UNDEF.

Legalization

diggerlin · 2025-04-10T14:03:16Z

I updated the patch with the original PR (#125883) and the fix for " [DAGTypeLegalize] Legalize ISD::POISON as ISD::UNDEF for Float Type Legalization" , and change the title and summary.

kuhar

This fixes the RISC-V crash we observed with the first version of the patch. Thanks for taking care of this!

llvm/test/CodeGen/PowerPC/poison-legalization.ll

llvm/include/llvm/CodeGen/SelectionDAGNodes.h

Co-authored-by: Jakub Kuderski <kubakuderski@gmail.com>

…ent the poison value in the IR." (llvm#135056) A new ISD::POISON SDNode is introduced to represent the poison value in the IR, replacing the previous use of ISD::UNDEF

mstorsjo · 2025-04-22T09:00:10Z

FYI, for reference, I've bisected a very elusive and tricky crash down to this commit, when compiling with Clang on Windows/arm64. Unfortunately the crash isn't fully deterministic and seems dependent on what version of Clang I'm using to build Clang, so it's not clear cut what's the root cause. But posting here in case someone else also investigates and looks at bisects that point at this commit.

mstorsjo · 2025-04-25T13:08:03Z

FYI, for reference, I've bisected a very elusive and tricky crash down to this commit, when compiling with Clang on Windows/arm64. Unfortunately the crash isn't fully deterministic and seems dependent on what version of Clang I'm using to build Clang, so it's not clear cut what's the root cause. But posting here in case someone else also investigates and looks at bisects that point at this commit.

Sorry for the noise; this commit was unrelated to the issue. (This commit only uncovered the bug that had been present for a couple weeks before.)

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Apr 9, 2025

diggerlin mentioned this pull request Apr 9, 2025

[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR. #125883

Merged

diggerlin requested review from kuhar, RolandF77, arsenm and topperc April 9, 2025 17:34

diggerlin requested review from nikic, RKSimon and amy-kwan April 9, 2025 17:44

nikic reviewed Apr 9, 2025

View reviewed changes

RKSimon reviewed Apr 9, 2025

View reviewed changes

kuhar reviewed Apr 9, 2025

View reviewed changes

arsenm reviewed Apr 9, 2025

View reviewed changes

diggerlin changed the title ~~fix a assert on the PR 125883~~ [DAGTypeLegalize] Legalize ISD::POISON as ISD::UNDEF for Float Type Legalization Apr 9, 2025

diggerlin requested review from arsenm, nikic, RKSimon and kuhar April 9, 2025 19:03

arichardson reviewed Apr 9, 2025

View reviewed changes

diggerlin requested a review from arichardson April 9, 2025 20:26

llvmbot added the backend:PowerPC label Apr 9, 2025

arichardson approved these changes Apr 9, 2025

View reviewed changes

kuhar reviewed Apr 10, 2025

View reviewed changes

diggerlin added 2 commits April 10, 2025 13:25

[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the …

2d9bdc6

…poison value in the IR. (llvm#125883) A new ISD::POISON SDNode is introduced to represent the `poison value` in the IR, replacing the previous use of ISD::UNDEF.

[DAGTypeLegalize] Legalize ISD::POISON as ISD::UNDEF for Float Type

781c794

Legalization

diggerlin force-pushed the digger/fix-125883 branch from 7087edb to 781c794 Compare April 10, 2025 13:58

llvmbot added backend:AArch64 backend:X86 labels Apr 10, 2025

diggerlin changed the title ~~[DAGTypeLegalize] Legalize ISD::POISON as ISD::UNDEF for Float Type Legalization~~ [SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR. Apr 10, 2025

diggerlin requested a review from kuhar April 10, 2025 14:03

kuhar reviewed Apr 10, 2025

View reviewed changes

llvm/test/CodeGen/PowerPC/poison-legalization.ll Outdated Show resolved Hide resolved

llvm/include/llvm/CodeGen/SelectionDAGNodes.h Show resolved Hide resolved

kuhar changed the title ~~[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR.~~ Reland "[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR." Apr 10, 2025

Update llvm/test/CodeGen/PowerPC/poison-legalization.ll

e2c713b

Co-authored-by: Jakub Kuderski <kubakuderski@gmail.com>

kuhar approved these changes Apr 10, 2025

View reviewed changes

diggerlin merged commit 378ac57 into llvm:main Apr 10, 2025
8 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland "[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR." #135056

Reland "[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR." #135056

diggerlin commented Apr 9, 2025 •

edited

Loading

llvmbot commented Apr 9, 2025 •

edited

Loading

topperc commented Apr 9, 2025 •

edited

Loading

nikic left a comment •

edited by diggerlin

Loading

RKSimon Apr 9, 2025 •

edited by diggerlin

Loading

topperc Apr 9, 2025

arsenm commented Apr 9, 2025

kuhar left a comment

topperc commented Apr 9, 2025

arsenm Apr 9, 2025

diggerlin Apr 9, 2025 •

edited

Loading

arsenm Apr 9, 2025

diggerlin commented Apr 9, 2025 •

edited

Loading

arichardson commented Apr 9, 2025

diggerlin commented Apr 9, 2025 •

edited

Loading

arichardson Apr 9, 2025

arichardson left a comment

kuhar left a comment

diggerlin commented Apr 10, 2025 •

edited

Loading

kuhar commented Apr 10, 2025 •

edited

Loading

diggerlin commented Apr 10, 2025

kuhar left a comment

mstorsjo commented Apr 22, 2025

mstorsjo commented Apr 25, 2025

		@@ -0,0 +1,19 @@
		; RUN: llc < %s -mtriple=riscv32 \| FileCheck %s

	define void @b(ptr %p1, ptr %p2) {
	; CHECK: .cfi_startproc
	define void @b(ptr %p1, ptr %p2) nounwind {

Reland "[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR." #135056

Reland "[SelectionDAG] Introducing a new ISD::POISON SDNode to represent the poison value in the IR." #135056

Conversation

diggerlin commented Apr 9, 2025 • edited Loading

llvmbot commented Apr 9, 2025 • edited Loading

topperc commented Apr 9, 2025 • edited Loading

nikic left a comment • edited by diggerlin Loading

Choose a reason for hiding this comment

RKSimon Apr 9, 2025 • edited by diggerlin Loading

Choose a reason for hiding this comment

topperc Apr 9, 2025

Choose a reason for hiding this comment

arsenm commented Apr 9, 2025

kuhar left a comment

Choose a reason for hiding this comment

topperc commented Apr 9, 2025

arsenm Apr 9, 2025

Choose a reason for hiding this comment

diggerlin Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

arsenm Apr 9, 2025

Choose a reason for hiding this comment

diggerlin commented Apr 9, 2025 • edited Loading

arichardson commented Apr 9, 2025

diggerlin commented Apr 9, 2025 • edited Loading

arichardson Apr 9, 2025

Choose a reason for hiding this comment

arichardson left a comment

Choose a reason for hiding this comment

kuhar left a comment

Choose a reason for hiding this comment

diggerlin commented Apr 10, 2025 • edited Loading

kuhar commented Apr 10, 2025 • edited Loading

diggerlin commented Apr 10, 2025

kuhar left a comment

Choose a reason for hiding this comment

mstorsjo commented Apr 22, 2025

mstorsjo commented Apr 25, 2025

diggerlin commented Apr 9, 2025 •

edited

Loading

llvmbot commented Apr 9, 2025 •

edited

Loading

topperc commented Apr 9, 2025 •

edited

Loading

nikic left a comment •

edited by diggerlin

Loading

RKSimon Apr 9, 2025 •

edited by diggerlin

Loading

diggerlin Apr 9, 2025 •

edited

Loading

diggerlin commented Apr 9, 2025 •

edited

Loading

diggerlin commented Apr 9, 2025 •

edited

Loading

diggerlin commented Apr 10, 2025 •

edited

Loading

kuhar commented Apr 10, 2025 •

edited

Loading