Skip to content

[SelectionDAG] Avoid redundant node visits in EnforceNodeIdInvariant #164834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[SelectionDAG] Avoid redundant node visits in EnforceNodeIdInvariant #164834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a406b29
[SelectionDAG] Avoid redundant node visits in EnforceNodeIdInvariant
mohammadamin382 Oct 23, 2025
0494f78
Merge branch 'llvm:main' into main
mohammadamin382 Oct 23, 2025
980a7a4
Add tests for EnforceNodeIdInvariant in AArch64
mohammadamin382 Oct 23, 2025
96cb32c
Add tests for EnforceNodeIdInvariant in DAGs
mohammadamin382 Oct 23, 2025
f2a8764
Merge branch 'llvm:main' into main
mohammadamin382 Oct 23, 2025
310b69a
Merge branch 'main' into main
mohammadamin382 Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,13 +1236,15 @@ class ISelUpdater : public SelectionDAG::DAGUpdateListener {
// functions.
void SelectionDAGISel::EnforceNodeIdInvariant(SDNode *Node) {
SmallVector<SDNode *, 4> Nodes;
SmallPtrSet<SDNode *, 16> Visited;
Nodes.push_back(Node);
Visited.insert(Node);

while (!Nodes.empty()) {
SDNode *N = Nodes.pop_back_val();
for (auto *U : N->users()) {
auto UId = U->getNodeId();
if (UId > 0) {
if (UId > 0 && Visited.insert(U).second) {
Copy link
Collaborator

@topperc topperc Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the UId > 0 check be false the next time we try to visit the node?

Copy link
Collaborator

@topperc topperc Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code to this and the assert never failed.

      if (UId > 0) {
        assert(Visited.insert(U).second);

Copy link
Author

@mohammadamin382 mohammadamin382 Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right and I should have tested this more carefully

I mistakenly thought nodes could be pushed multiple times before being
popped but InvalidateNodeId runs immediately when we first see the node
so UId becomes negative and the second check automatically fails

The visited set is redundant because the ID invalidation already prevents
revisits

Thanks for catching this - the assert test is a clean way to verify it

InvalidateNodeId(U);
Nodes.push_back(U);
}
Expand Down
119 changes: 119 additions & 0 deletions llvm/test/CodeGen/AArch64/test-enforce-nodeid-invariant.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs | FileCheck %s
; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu -verify-machineinstrs | FileCheck %s

; Test that EnforceNodeIdInvariant correctly handles nodes
; users and prevents redundant invalidation visits. This test creates a
; DAG structure with high fan-in where multiple nodes share common users.
; should visit each node exactly once.

define i32 @test_diamond_dag(i32 %a, i32 %b) {
; CHECK-LABEL: test_diamond_dag:
entry:
%add1 = add i32 %a, %b
%add2 = add i32 %a, %b
%mul1 = mul i32 %add1, %add2
%mul2 = mul i32 %add1, %add2
%result = add i32 %mul1, %mul2
ret i32 %result
}

define i64 @test_deep_dag(i64 %x) {
; CHECK-LABEL: test_deep_dag:
entry:
%a = add i64 %x, 1
%b = add i64 %x, 2
%c = add i64 %a, %b
%d = add i64 %a, %b
%e = mul i64 %c, %d
%f = mul i64 %c, %d
%g = add i64 %e, %f
ret i64 %g
}

define i32 @test_wide_dag(i32 %v) {
; CHECK-LABEL: test_wide_dag:
entry:
%a = add i32 %v, 1
%b = add i32 %v, 2
%c = add i32 %v, 3
%d = add i32 %v, 4
%ab = add i32 %a, %b
%cd = add i32 %c, %d
%ac = add i32 %a, %c
%bd = add i32 %b, %d
%t1 = mul i32 %ab, %cd
%t2 = mul i32 %ac, %bd
%result = add i32 %t1, %t2
ret i32 %result
}

define <4 x i32> @test_vector_dag(<4 x i32> %vec1, <4 x i32> %vec2) {
; CHECK-LABEL: test_vector_dag:
entry:
%add1 = add <4 x i32> %vec1, %vec2
%add2 = add <4 x i32> %vec1, %vec2
%mul1 = mul <4 x i32> %add1, %add2
%mul2 = mul <4 x i32> %add1, %add2
%result = add <4 x i32> %mul1, %mul2
ret <4 x i32> %result
}

define i32 @test_chain_with_shared_users(i32 %x, i32 %y, i32 %z) {
; CHECK-LABEL: test_chain_with_shared_users:
entry:
%a = add i32 %x, %y
%b = add i32 %y, %z
%c = mul i32 %a, %b
%d = mul i32 %a, %b
%e = add i32 %c, %d
%f = sub i32 %c, %d
%g = mul i32 %e, %f
ret i32 %g
}

define i64 @test_complex_sharing(i64 %p1, i64 %p2, i64 %p3) {
; CHECK-LABEL: test_complex_sharing:
entry:
%n1 = add i64 %p1, %p2
%n2 = add i64 %p2, %p3
%n3 = add i64 %p1, %p3
%u1 = mul i64 %n1, %n2
%u2 = mul i64 %n1, %n3
%u3 = mul i64 %n2, %n3
%s1 = add i64 %u1, %u2
%s2 = add i64 %u2, %u3
%s3 = add i64 %u1, %u3
%f1 = mul i64 %s1, %s2
%f2 = mul i64 %s2, %s3
%result = add i64 %f1, %f2
ret i64 %result
}

define i32 @test_multiple_paths_to_same_user(i32 %input) {
; CHECK-LABEL: test_multiple_paths_to_same_user:
entry:
%a = add i32 %input, 1
%b = add i32 %input, 2
%c = add i32 %input, 3
%ab = mul i32 %a, %b
%bc = mul i32 %b, %c
%ac = mul i32 %a, %c
%shared = add i32 %ab, %bc
%shared2 = add i32 %ac, %shared
ret i32 %shared2
}

define i64 @test_reconvergent_paths(i64 %val) {
; CHECK-LABEL: test_reconvergent_paths:
entry:
%l1a = add i64 %val, 10
%l1b = add i64 %val, 20
%l2a = mul i64 %l1a, 2
%l2b = mul i64 %l1b, 3
%l2c = mul i64 %l1a, 4
%l2d = mul i64 %l1b, 5
%l3a = add i64 %l2a, %l2b
%l3b = add i64 %l2c, %l2d
%merge = mul i64 %l3a, %l3b
ret i64 %merge
}
119 changes: 119 additions & 0 deletions llvm/test/CodeGen/X86/test-enforce-nodeid-invariant.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs | FileCheck %s
; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu -verify-machineinstrs | FileCheck %s

; Test that EnforceNodeIdInvariant correctly handles nodes
; users and prevents redundant invalidation visits. This test creates a
; DAG structure with high fan-in where multiple nodes share common users.
; should visit each node exactly once.

define i32 @test_diamond_dag(i32 %a, i32 %b) {
; CHECK-LABEL: test_diamond_dag:
entry:
%add1 = add i32 %a, %b
%add2 = add i32 %a, %b
%mul1 = mul i32 %add1, %add2
%mul2 = mul i32 %add1, %add2
%result = add i32 %mul1, %mul2
ret i32 %result
}

define i64 @test_deep_dag(i64 %x) {
; CHECK-LABEL: test_deep_dag:
entry:
%a = add i64 %x, 1
%b = add i64 %x, 2
%c = add i64 %a, %b
%d = add i64 %a, %b
%e = mul i64 %c, %d
%f = mul i64 %c, %d
%g = add i64 %e, %f
ret i64 %g
}

define i32 @test_wide_dag(i32 %v) {
; CHECK-LABEL: test_wide_dag:
entry:
%a = add i32 %v, 1
%b = add i32 %v, 2
%c = add i32 %v, 3
%d = add i32 %v, 4
%ab = add i32 %a, %b
%cd = add i32 %c, %d
%ac = add i32 %a, %c
%bd = add i32 %b, %d
%t1 = mul i32 %ab, %cd
%t2 = mul i32 %ac, %bd
%result = add i32 %t1, %t2
ret i32 %result
}

define <4 x i32> @test_vector_dag(<4 x i32> %vec1, <4 x i32> %vec2) {
; CHECK-LABEL: test_vector_dag:
entry:
%add1 = add <4 x i32> %vec1, %vec2
%add2 = add <4 x i32> %vec1, %vec2
%mul1 = mul <4 x i32> %add1, %add2
%mul2 = mul <4 x i32> %add1, %add2
%result = add <4 x i32> %mul1, %mul2
ret <4 x i32> %result
}

define i32 @test_chain_with_shared_users(i32 %x, i32 %y, i32 %z) {
; CHECK-LABEL: test_chain_with_shared_users:
entry:
%a = add i32 %x, %y
%b = add i32 %y, %z
%c = mul i32 %a, %b
%d = mul i32 %a, %b
%e = add i32 %c, %d
%f = sub i32 %c, %d
%g = mul i32 %e, %f
ret i32 %g
}

define i64 @test_complex_sharing(i64 %p1, i64 %p2, i64 %p3) {
; CHECK-LABEL: test_complex_sharing:
entry:
%n1 = add i64 %p1, %p2
%n2 = add i64 %p2, %p3
%n3 = add i64 %p1, %p3
%u1 = mul i64 %n1, %n2
%u2 = mul i64 %n1, %n3
%u3 = mul i64 %n2, %n3
%s1 = add i64 %u1, %u2
%s2 = add i64 %u2, %u3
%s3 = add i64 %u1, %u3
%f1 = mul i64 %s1, %s2
%f2 = mul i64 %s2, %s3
%result = add i64 %f1, %f2
ret i64 %result
}

define i32 @test_multiple_paths_to_same_user(i32 %input) {
; CHECK-LABEL: test_multiple_paths_to_same_user:
entry:
%a = add i32 %input, 1
%b = add i32 %input, 2
%c = add i32 %input, 3
%ab = mul i32 %a, %b
%bc = mul i32 %b, %c
%ac = mul i32 %a, %c
%shared = add i32 %ab, %bc
%shared2 = add i32 %ac, %shared
ret i32 %shared2
}

define i64 @test_reconvergent_paths(i64 %val) {
; CHECK-LABEL: test_reconvergent_paths:
entry:
%l1a = add i64 %val, 10
%l1b = add i64 %val, 20
%l2a = mul i64 %l1a, 2
%l2b = mul i64 %l1b, 3
%l2c = mul i64 %l1a, 4
%l2d = mul i64 %l1b, 5
%l3a = add i64 %l2a, %l2b
%l3b = add i64 %l2c, %l2d
%merge = mul i64 %l3a, %l3b
ret i64 %merge
}