[DAGCombiner] Ensure poison-generating flags are stripped in `freeze` op #114582

antoniofrighetto · 2024-11-01T17:50:15Z

When combining freeze in DAG, poison-generating flags were previously not fully removed as intended. This change explicitly drops these flags from freeze operand, ensuring a fixed, non-poisonous value at this point in computation.

Previous buildbot failures: https://lab.llvm.org/buildbot/#/builders/145/builds/2872/steps/11/logs/stdio, https://lab.llvm.org/buildbot/#/builders/95/builds/5659/steps/11/logs/stdio.

When combining `freeze` in DAG, poison-generating flags were previously not fully removed as intended. This change explicitly drops these flags from `freeze` operand, ensuring a fixed, non-poisonous value at this point in computation.

llvmbot · 2024-11-01T17:50:48Z

@llvm/pr-subscribers-backend-arm

Author: Antonio Frighetto (antoniofrighetto)

Changes

When combining freeze in DAG, poison-generating flags were previously not fully removed as intended. This change explicitly drops these flags from freeze operand, ensuring a fixed, non-poisonous value at this point in computation.

Previous buildbot failures: https://lab.llvm.org/buildbot/#/builders/145/builds/2872/steps/11/logs/stdio, https://lab.llvm.org/buildbot/#/builders/95/builds/5659/steps/11/logs/stdio.

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

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+1)
(added) llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll (+40)

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 7eef09e55101d0..c24ab99d75de65 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -16151,6 +16151,7 @@ SDValue DAGCombiner::visitFREEZE(SDNode *N) {
                              SVN->getMask());
   } else {
     // NOTE: this strips poison generating flags.
+    N0->dropFlags(SDNodeFlags::PoisonGeneratingFlags);
     R = DAG.getNode(N0.getOpcode(), SDLoc(N0), N0->getVTList(), Ops);
   }
   assert(DAG.isGuaranteedNotToBeUndefOrPoison(R, /*PoisonOnly*/ false) &&
diff --git a/llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll b/llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll
new file mode 100644
index 00000000000000..fbb3feee67b29b
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll
@@ -0,0 +1,40 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=arm-linux-gnueabi -mcpu=arm1022e -mattr=armv5te -o - %s | FileCheck %s
+
+; Ensure poison-generating flags are stripped by the time a freeze operand is visited.
+
+@g_ptr = global ptr null, align 4
+
+define ptr @drop_flags(i32 noundef %numentries, i64 %cond, i64 %arg) local_unnamed_addr #0 {
+; CHECK-LABEL: drop_flags:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    ldm sp, {r1, r12}
+; CHECK-NEXT:    subs r1, r2, r1
+; CHECK-NEXT:    sbcs r1, r3, r12
+; CHECK-NEXT:    movlo r0, r2
+; CHECK-NEXT:    cmp r0, #0
+; CHECK-NEXT:    ldr r0, .LCPI0_0
+; CHECK-NEXT:    ldr r0, [r0]
+; CHECK-NEXT:    bx lr
+; CHECK-NEXT:    .p2align 2
+; CHECK-NEXT:  @ %bb.1:
+; CHECK-NEXT:  .LCPI0_0:
+; CHECK-NEXT:    .long g_ptr
+entry:
+  %cmp4 = icmp samesign ult i64 %cond, %arg
+  %conv6 = trunc nuw i64 %cond to i32
+  %spec.select = select i1 %cmp4, i32 %conv6, i32 %numentries
+  %spec.select.fr = freeze i32 %spec.select
+  %cmpz = icmp eq i32 %spec.select.fr, 0
+  br i1 %cmpz, label %bb.end, label %bb.false
+
+bb.false:                                       ; preds = %entry
+  %2 = tail call range(i32 0, 33) i32 @llvm.ctlz.i32(i32 %spec.select.fr, i1 true)
+  br label %bb.end
+
+bb.end:                                         ; preds = %entry, %bb.false
+  %3 = load ptr, ptr @g_ptr, align 4
+  ret ptr %3
+}
+
+declare i32 @llvm.ctlz.i32(i32, i1)

llvmbot · 2024-11-01T17:50:48Z

@llvm/pr-subscribers-llvm-selectiondag

Author: Antonio Frighetto (antoniofrighetto)

Changes

When combining freeze in DAG, poison-generating flags were previously not fully removed as intended. This change explicitly drops these flags from freeze operand, ensuring a fixed, non-poisonous value at this point in computation.

Previous buildbot failures: https://lab.llvm.org/buildbot/#/builders/145/builds/2872/steps/11/logs/stdio, https://lab.llvm.org/buildbot/#/builders/95/builds/5659/steps/11/logs/stdio.

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

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+1)
(added) llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll (+40)

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 7eef09e55101d0..c24ab99d75de65 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -16151,6 +16151,7 @@ SDValue DAGCombiner::visitFREEZE(SDNode *N) {
                              SVN->getMask());
   } else {
     // NOTE: this strips poison generating flags.
+    N0->dropFlags(SDNodeFlags::PoisonGeneratingFlags);
     R = DAG.getNode(N0.getOpcode(), SDLoc(N0), N0->getVTList(), Ops);
   }
   assert(DAG.isGuaranteedNotToBeUndefOrPoison(R, /*PoisonOnly*/ false) &&
diff --git a/llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll b/llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll
new file mode 100644
index 00000000000000..fbb3feee67b29b
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll
@@ -0,0 +1,40 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=arm-linux-gnueabi -mcpu=arm1022e -mattr=armv5te -o - %s | FileCheck %s
+
+; Ensure poison-generating flags are stripped by the time a freeze operand is visited.
+
+@g_ptr = global ptr null, align 4
+
+define ptr @drop_flags(i32 noundef %numentries, i64 %cond, i64 %arg) local_unnamed_addr #0 {
+; CHECK-LABEL: drop_flags:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    ldm sp, {r1, r12}
+; CHECK-NEXT:    subs r1, r2, r1
+; CHECK-NEXT:    sbcs r1, r3, r12
+; CHECK-NEXT:    movlo r0, r2
+; CHECK-NEXT:    cmp r0, #0
+; CHECK-NEXT:    ldr r0, .LCPI0_0
+; CHECK-NEXT:    ldr r0, [r0]
+; CHECK-NEXT:    bx lr
+; CHECK-NEXT:    .p2align 2
+; CHECK-NEXT:  @ %bb.1:
+; CHECK-NEXT:  .LCPI0_0:
+; CHECK-NEXT:    .long g_ptr
+entry:
+  %cmp4 = icmp samesign ult i64 %cond, %arg
+  %conv6 = trunc nuw i64 %cond to i32
+  %spec.select = select i1 %cmp4, i32 %conv6, i32 %numentries
+  %spec.select.fr = freeze i32 %spec.select
+  %cmpz = icmp eq i32 %spec.select.fr, 0
+  br i1 %cmpz, label %bb.end, label %bb.false
+
+bb.false:                                       ; preds = %entry
+  %2 = tail call range(i32 0, 33) i32 @llvm.ctlz.i32(i32 %spec.select.fr, i1 true)
+  br label %bb.end
+
+bb.end:                                         ; preds = %entry, %bb.false
+  %3 = load ptr, ptr @g_ptr, align 4
+  ret ptr %3
+}
+
+declare i32 @llvm.ctlz.i32(i32, i1)

arsenm · 2024-11-01T17:52:42Z

llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll

+
+@g_ptr = global ptr null, align 4
+
+define ptr @drop_flags(i32 noundef %numentries, i64 %cond, i64 %arg) local_unnamed_addr #0 {


Don't need local_unnamed_addr, and #0 is dead

arsenm · 2024-11-01T17:56:34Z

llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll

@@ -0,0 +1,40 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=arm-linux-gnueabi -mcpu=arm1022e -mattr=armv5te -o - %s | FileCheck %s


Pretty sure this would need to be +armv5te, does this print a parse warning?

It's quite weird as I didn't get any warning, this definitely needs to be fixed. Dropped it as turned to be not needed (gets exercized anyways).

RKSimon · 2024-11-01T18:13:04Z

When combining freeze in DAG, poison-generating flags were previously not fully removed as intended. This change explicitly drops these flags from freeze operand, ensuring a fixed, non-poisonous value at this point in computation.

Sorry if I've missed something (long day), but where was it intended that the poison flags should be removed?

antoniofrighetto · 2024-11-01T18:22:23Z

When combining freeze in DAG, poison-generating flags were previously not fully removed as intended. This change explicitly drops these flags from freeze operand, ensuring a fixed, non-poisonous value at this point in computation.

Sorry if I've missed something (long day), but where was it intended that the poison flags should be removed?

I might be missing something, but if I read this correctly, the line following the comment would be supposed to strip the flags from the current node (which doesn't carry any flags), and getNode doesn't actually drop any flags. To be correct, it seems that no flags are set in FlagsInserter in the current DAG, except that samesign is there.

arsenm · 2024-11-01T18:25:48Z

llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll

+  br i1 %cmpz, label %bb.end, label %bb.false
+
+bb.false:                                       ; preds = %entry
+  %2 = tail call range(i32 0, 33) i32 @llvm.ctlz.i32(i32 %spec.select.fr, i1 true)


This is unused?

Test simplified.

dtcxzyw · 2024-11-02T08:06:07Z

llvm/test/CodeGen/ARM/dagcombine-drop-flags-freeze.ll

+  ret i1 %cmpz
+}
+
+declare i32 @llvm.ctlz.i32(i32, i1)


dtcxzyw · 2024-11-02T08:28:34Z

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

@@ -16151,6 +16151,7 @@ SDValue DAGCombiner::visitFREEZE(SDNode *N) {
                             SVN->getMask());
  } else {
    // NOTE: this strips poison generating flags.
+    N0->dropFlags(SDNodeFlags::PoisonGeneratingFlags);


I don't think it is a correct fix. The following getNode creates a new copy without flags. However, it is CSEed to N0 in SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, SDVTList VTList, ArrayRef<SDValue> Ops, const SDNodeFlags Flags).

We should intersect flags after CSE here:

llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Lines 10321 to 10322 in 92daad2

if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP))

return SDValue(E, 0);

llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Lines 10527 to 10528 in 92daad2

if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP))

return SDValue(E, 0);

I will post a fix later.

getNode creates a copy, but N0 is still the original node. It is indeed CSE'd, but the flag has been dropped by the time it gets CSE'd.

but the flag has been dropped by the time it gets CSE'd.

I printed the node returned by FindNodeOrInsertPos and got select_cc samesign. The flags are not dropped. It should be fixed by #114650.

Quite strange. I just tried again and confirm the node returned here:

llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Lines 10321 to 10322 in 89a8c71

if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP))

return SDValue(E, 0);

is i32 = select_cc t12, t28, t16, t2, setult:ch, immediately after visiting freeze in DAGCombiner, nullptr otherwise. I'm happy with your patch too.

baseline: f1e1055

diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp index 7eef09e55101..b5bc6d925b41 100644 --- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp @@ -16150,8 +16150,12 @@ SDValue DAGCombiner::visitFREEZE(SDNode *N) { R = DAG.getVectorShuffle(N0.getValueType(), SDLoc(N0), Ops[0], Ops[1], SVN->getMask()); } else { + errs() << "N0: "; + N0->dump(); // NOTE: this strips poison generating flags. R = DAG.getNode(N0.getOpcode(), SDLoc(N0), N0->getVTList(), Ops); + errs() << "R: "; + R->dump(); } assert(DAG.isGuaranteedNotToBeUndefOrPoison(R, /*PoisonOnly*/ false) && "Can't create node that may be undef/poison!"); diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp index d5cdd7163d79..a533ac910588 100644 --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp @@ -10318,8 +10318,11 @@ SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT, AddNodeIDNode(ID, Opcode, VTs, Ops); void *IP = nullptr; - if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP)) + if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP)) { + errs() << "E: "; + E->dump(); return SDValue(E, 0); + } N = newSDNode<SDNode>(Opcode, DL.getIROrder(), DL.getDebugLoc(), VTs); createOperands(N, Ops);

N0: t27: i32 = select_cc samesign t12, t28, t16, t2, setult:ch E: t27: i32 = select_cc samesign t12, t28, t16, t2, setult:ch R: t27: i32 = select_cc samesign t12, t28, t16, t2, setult:ch

antoniofrighetto · 2024-11-02T13:22:45Z

Closing this as favoured approach in #114650.

antoniofrighetto requested review from arsenm, nikic, dtcxzyw and topperc November 1, 2024 17:50

llvmbot added backend:ARM llvm:SelectionDAG SelectionDAGISel as well labels Nov 1, 2024

arsenm reviewed Nov 1, 2024

View reviewed changes

!fixup drop dead attribute

f731a72

arsenm reviewed Nov 1, 2024

View reviewed changes

!fixup drop -mattr

0c5e0a2

arsenm reviewed Nov 1, 2024

View reviewed changes

!fixup simplify test

1c5a0fc

antoniofrighetto mentioned this pull request Nov 1, 2024

[SelectionDAG] Add preliminary plumbing for samesign flag #112354

Merged

dtcxzyw requested changes Nov 2, 2024

View reviewed changes

dtcxzyw mentioned this pull request Nov 2, 2024

[SDAG] Intersect poison-generating flags after CSE #114650

Merged

antoniofrighetto closed this Nov 2, 2024


		@g_ptr = global ptr null, align 4

		define ptr @drop_flags(i32 noundef %numentries, i64 %cond, i64 %arg) local_unnamed_addr #0 {

		@@ -0,0 +1,40 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
		; RUN: llc -mtriple=arm-linux-gnueabi -mcpu=arm1022e -mattr=armv5te -o - %s \| FileCheck %s

	if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP))
	return SDValue(E, 0);

[DAGCombiner] Ensure poison-generating flags are stripped in freeze op #114582

[DAGCombiner] Ensure poison-generating flags are stripped in freeze op #114582

Uh oh!

Conversation

antoniofrighetto commented Nov 1, 2024

Uh oh!

llvmbot commented Nov 1, 2024

Uh oh!

llvmbot commented Nov 1, 2024

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!

RKSimon commented Nov 1, 2024

Uh oh!

antoniofrighetto commented Nov 1, 2024

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!

dtcxzyw Nov 2, 2024 • 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!

antoniofrighetto Nov 2, 2024 • 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!

antoniofrighetto commented Nov 2, 2024

Uh oh!

Uh oh!

[DAGCombiner] Ensure poison-generating flags are stripped in `freeze` op #114582

[DAGCombiner] Ensure poison-generating flags are stripped in `freeze` op #114582

dtcxzyw Nov 2, 2024 •

edited

Loading

antoniofrighetto Nov 2, 2024 •

edited

Loading