[NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number #82110

ManuelJBrito · 2024-02-17T14:22:48Z

Cyclic dependencies in NewGVN can result in miscompilation and termination issues.
Example:

define void @main(i1 %c1, i1 %c2, i32 %x) {
entry:
  br i1 %c1, label %L, label %end

L:
  %d.1 = phi i8 [ undef, %entry ], [ -1, %L ]
  %conv = sext i8 %d.1 to i32
  %xor = xor i32 %x, %conv
  %neg = xor i32 %xor, -1
  call void @foo(i32 %neg)
  br label %L

end:
  ret void
}

%d.1 = phi i8 [ undef, %entry ], [ -1, %L ] -> undef (ignores unreachable backedge)
%conv = sext i8 %d.1 to i32 -> 0 (refined by InstSimplify)
%xor = xor i32 %x, %conv -> %x
%neg = xor i32 %xor, -1 -> xor i32 %x, -1

edge (%L, %L) is marked as reachable => %d.1 is added to the worklist

%d.1 = phi i8 [ undef, %entry ], [ -1, %L ] -> -1 (ignores undef)
%conv = sext i8 %d.1 to i32 -> -1 
%xor = xor i32 %x, %conv -> xor i32 %x, -1 (Leader is %neg)
%neg = xor i32 %xor, -1 -> xor i32 %x, -1 ( %xor is replaced by %neg => WRONG)

The problem is the cyclic dependence:

%neg depends on %xor because it's an operand.
%xor depends on %neg because it's the class leader.

This issue also leads to infinite evaluation loops, where NewGVN never converges due to the cyclic dependence.

This patch ensures that the Class Leader is always the member with the lowest RPO number. This ensures that the Class Leader is processed before all other members, making the cyclic dependence impossible.

This fixes #35683.

Regressions:

'simp-to-self.ll' regresses due to a known limitation in the way NewGVN and InstSimplify interact. With the new leader, InstSimplify does not know that %conv is 1 and fails to simplify.

llvmbot · 2024-02-17T14:23:18Z

@llvm/pr-subscribers-llvm-transforms

Author: None (ManuelJBrito)

Changes

Cyclic dependencies in NewGVN can result in miscompilation and termination issues.
Example:

define void @<!-- -->main(i1 %c1, i1 %c2, i32 %x) {
entry:
  br i1 %c1, label %L, label %end

L:
  %d.1 = phi i8 [ undef, %entry ], [ -1, %L ]
  %conv = sext i8 %d.1 to i32
  %xor = xor i32 %x, %conv
  %neg = xor i32 %xor, -1
  call void @<!-- -->foo(i32 %neg)
  br label %L

end:
  ret void
}

%d.1 = phi i8 [ undef, %entry ], [ -1, %L ] -&gt; undef (ignores unreachable backedge)
%conv = sext i8 %d.1 to i32 -&gt; 0 (refined by InstSimplify)
%xor = xor i32 %x, %conv -&gt; %x
%neg = xor i32 %xor, -1 -&gt; xor i32 %x, -1

edge (%L, %L) is marked as reachable =&gt; %d.1 is added to the worklist

%d.1 = phi i8 [ undef, %entry ], [ -1, %L ] -&gt; -1 (ignores undef)
%conv = sext i8 %d.1 to i32 -&gt; -1 
%xor = xor i32 %x, %conv -&gt; xor i32 %x, -1 (Leader is %neg)
%neg = xor i32 %xor, -1 -&gt; xor i32 %x, -1 ( %xor is replaced by %neg =&gt; WRONG)

The problem is the cyclic dependence:

%neg depends on %xor because it's an operand.
%xor depends on %neg because it's the class leader.

This issue also leads to infinite evaluation loops, where NewGVN never converges due to the cyclic dependence.

This patch ensures that the Class Leader is always the member with the lowest RPO number. This ensures that the Class Leader is processed before all other members, making the cyclic dependence impossible.

This fixes #35683 and #69211.

Regressions:

'simp-to-self.ll' regresses due to a known limitation in the way NewGVN and InstSimplify interact. With the new leader, InstSimplify does not know that %conv is 1 and fails to simplify.

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

4 Files Affected:

(modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+32-17)
(renamed) llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll (+2-3)
(added) llvm/test/Transforms/NewGVN/pr69211.ll (+58)
(modified) llvm/test/Transforms/NewGVN/simp-to-self.ll (+16-5)

diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 19ac9526b5f88b..6770ffd1244098 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -297,7 +297,8 @@ class CongruenceClass {
   using MemoryMemberSet = SmallPtrSet<const MemoryMemberType *, 2>;
 
   explicit CongruenceClass(unsigned ID) : ID(ID) {}
-  CongruenceClass(unsigned ID, Value *Leader, const Expression *E)
+  CongruenceClass(unsigned ID, std::pair<Value *, unsigned int> Leader,
+                  const Expression *E)
       : ID(ID), RepLeader(Leader), DefiningExpr(E) {}
 
   unsigned getID() const { return ID; }
@@ -311,15 +312,21 @@ class CongruenceClass {
   }
 
   // Leader functions
-  Value *getLeader() const { return RepLeader; }
-  void setLeader(Value *Leader) { RepLeader = Leader; }
+  Value *getLeader() const { return RepLeader.first; }
+  void setLeader(std::pair<Value *, unsigned int> Leader) {
+    RepLeader = Leader;
+  }
   const std::pair<Value *, unsigned int> &getNextLeader() const {
     return NextLeader;
   }
   void resetNextLeader() { NextLeader = {nullptr, ~0}; }
-  void addPossibleNextLeader(std::pair<Value *, unsigned int> LeaderPair) {
-    if (LeaderPair.second < NextLeader.second)
+  void addPossibleLeader(std::pair<Value *, unsigned int> LeaderPair) {
+    if (LeaderPair.second < RepLeader.second) {
+      NextLeader = RepLeader;
+      RepLeader = LeaderPair;
+    } else if (LeaderPair.second < NextLeader.second) {
       NextLeader = LeaderPair;
+    }
   }
 
   Value *getStoredValue() const { return RepStoredValue; }
@@ -374,9 +381,10 @@ class CongruenceClass {
     if (this == Other)
       return true;
 
-    if (std::tie(StoreCount, RepLeader, RepStoredValue, RepMemoryAccess) !=
-        std::tie(Other->StoreCount, Other->RepLeader, Other->RepStoredValue,
-                 Other->RepMemoryAccess))
+    if (std::tie(StoreCount, RepLeader.first, RepStoredValue,
+                 RepMemoryAccess) !=
+        std::tie(Other->StoreCount, Other->RepLeader.first,
+                 Other->RepStoredValue, Other->RepMemoryAccess))
       return false;
     if (DefiningExpr != Other->DefiningExpr)
       if (!DefiningExpr || !Other->DefiningExpr ||
@@ -393,7 +401,7 @@ class CongruenceClass {
   unsigned ID;
 
   // Representative leader.
-  Value *RepLeader = nullptr;
+  std::pair<Value *, unsigned int> RepLeader = {nullptr, ~0U};
 
   // The most dominating leader after our current leader, because the member set
   // is not sorted and is expensive to keep sorted all the time.
@@ -731,7 +739,13 @@ class NewGVN {
 
   // Congruence class handling.
   CongruenceClass *createCongruenceClass(Value *Leader, const Expression *E) {
-    auto *result = new CongruenceClass(NextCongruenceNum++, Leader, E);
+    unsigned LeaderDFS = 0;
+    if (!Leader)
+      LeaderDFS = ~0;
+    else if (auto *I = dyn_cast<Instruction>(Leader))
+      LeaderDFS = InstrToDFSNum(I);
+    auto *result =
+        new CongruenceClass(NextCongruenceNum++, {Leader, LeaderDFS}, E);
     CongruenceClasses.emplace_back(result);
     return result;
   }
@@ -2245,7 +2259,8 @@ void NewGVN::moveValueToNewCongruenceClass(Instruction *I, const Expression *E,
   NewClass->insert(I);
 
   if (NewClass->getLeader() != I)
-    NewClass->addPossibleNextLeader({I, InstrToDFSNum(I)});
+    NewClass->addPossibleLeader({I, InstrToDFSNum(I)});
+
   // Handle our special casing of stores.
   if (auto *SI = dyn_cast<StoreInst>(I)) {
     OldClass->decStoreCount();
@@ -2269,7 +2284,7 @@ void NewGVN::moveValueToNewCongruenceClass(Instruction *I, const Expression *E,
                           << " because store joined class\n");
         // If we changed the leader, we have to mark it changed because we don't
         // know what it will do to symbolic evaluation.
-        NewClass->setLeader(SI);
+        NewClass->setLeader({SI, InstrToDFSNum(SI)});
       }
       // We rely on the code below handling the MemoryAccess change.
     }
@@ -2315,7 +2330,8 @@ void NewGVN::moveValueToNewCongruenceClass(Instruction *I, const Expression *E,
       if (OldClass->getStoredValue())
         OldClass->setStoredValue(nullptr);
     }
-    OldClass->setLeader(getNextValueLeader(OldClass));
+    OldClass->setLeader({getNextValueLeader(OldClass),
+                         InstrToDFSNum(getNextValueLeader(OldClass))});
     OldClass->resetNextLeader();
     markValueLeaderChangeTouched(OldClass);
   }
@@ -2354,15 +2370,15 @@ void NewGVN::performCongruenceFinding(Instruction *I, const Expression *E) {
 
       // Constants and variables should always be made the leader.
       if (const auto *CE = dyn_cast<ConstantExpression>(E)) {
-        NewClass->setLeader(CE->getConstantValue());
+        NewClass->setLeader({CE->getConstantValue(), 0});
       } else if (const auto *SE = dyn_cast<StoreExpression>(E)) {
         StoreInst *SI = SE->getStoreInst();
-        NewClass->setLeader(SI);
+        NewClass->setLeader({SI, InstrToDFSNum(SI)});
         NewClass->setStoredValue(SE->getStoredValue());
         // The RepMemoryAccess field will be filled in properly by the
         // moveValueToNewCongruenceClass call.
       } else {
-        NewClass->setLeader(I);
+        NewClass->setLeader({I, InstrToDFSNum(I)});
       }
       assert(!isa<VariableExpression>(E) &&
              "VariableExpression should have been handled already");
@@ -3253,7 +3269,6 @@ void NewGVN::verifyMemoryCongruency() const {
         return ReachableEdges.count(
                    {FirstMP->getIncomingBlock(U), FirstMP->getBlock()}) &&
                isa<MemoryDef>(U);
-
       };
       // All arguments should in the same class, ignoring unreachable arguments
       auto FilteredPhiArgs =
diff --git a/llvm/test/Transforms/NewGVN/todo-pr36335-phi-undef.ll b/llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll
similarity index 86%
rename from llvm/test/Transforms/NewGVN/todo-pr36335-phi-undef.ll
rename to llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll
index f3f3d2bccb68e2..839e7175b813c7 100644
--- a/llvm/test/Transforms/NewGVN/todo-pr36335-phi-undef.ll
+++ b/llvm/test/Transforms/NewGVN/pr36335-phi-undef.ll
@@ -1,8 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -passes=newgvn -S %s | FileCheck %s
 
-; TODO: NewGVN currently miscomiles the function below. PR36335.
-
 declare void @foo(i32)
 
 define void @main(i1 %c1, i1 %c2, i32 %x) {
@@ -11,7 +9,8 @@ define void @main(i1 %c1, i1 %c2, i32 %x) {
 ; CHECK-NEXT:    br i1 [[C1:%.*]], label [[L:%.*]], label [[END:%.*]]
 ; CHECK:       L:
 ; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[X:%.*]], -1
-; CHECK-NEXT:    call void @foo(i32 [[XOR]])
+; CHECK-NEXT:    [[NEG:%.*]] = xor i32 [[XOR]], -1
+; CHECK-NEXT:    call void @foo(i32 [[NEG]])
 ; CHECK-NEXT:    br label [[L]]
 ; CHECK:       end:
 ; CHECK-NEXT:    ret void
diff --git a/llvm/test/Transforms/NewGVN/pr69211.ll b/llvm/test/Transforms/NewGVN/pr69211.ll
new file mode 100644
index 00000000000000..dd6a0ec438657f
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/pr69211.ll
@@ -0,0 +1,58 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=newgvn -S | FileCheck %s
+
+define i32 @main() {
+; CHECK-LABEL: @main(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_BODY9:%.*]]
+; CHECK:       for.body9:
+; CHECK-NEXT:    [[INDVARS_IV:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[TMP0:%.*]], [[FOR_BODY9]] ]
+; CHECK-NEXT:    [[TMP0]] = add i64 1, [[INDVARS_IV]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[TMP0]], 1
+; CHECK-NEXT:    br label [[FOR_BODY9]]
+;
+entry:
+  br label %for.body9
+
+for.body9:                                        ; preds = %for.body9, %entry
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body9 ]
+  %0 = add i64 1, %indvars.iv
+  %1 = add i64 %0, 1
+  %2 = trunc i64 %1 to i32
+  %indvars.iv.next = add i64 %indvars.iv, 1
+  br label %for.body9
+}
+
+declare void @dummy(i64)
+
+define i32 @foo() {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
+; CHECK:       for.body:
+; CHECK-NEXT:    [[INDVARS_IV:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[TMP0:%.*]], [[FOR_BODY]] ]
+; CHECK-NEXT:    [[TMP0]] = add i64 [[INDVARS_IV]], 1
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[TMP0]], 1
+; CHECK-NEXT:    call void @dummy(i64 [[TMP1]])
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp slt i64 [[TMP0]], 1000
+; CHECK-NEXT:    br i1 [[TOBOOL]], label [[FOR_BODY]], label [[END:%.*]]
+; CHECK:       end:
+; CHECK-NEXT:    ret i32 0
+;
+entry:
+  br label %for.body
+
+for.body:                                        ; preds = %for.body, %entry
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+  %0 = add i64 %indvars.iv, 1
+  %1 = add i64 %0, 1
+  call void @dummy(i64 %1)
+
+  %indvars.iv.next = add i64 %indvars.iv, 1
+  %tobool = icmp slt i64 %indvars.iv.next, 1000
+  br i1 %tobool, label %for.body, label %end
+
+end:
+  ret i32 0
+}
+
diff --git a/llvm/test/Transforms/NewGVN/simp-to-self.ll b/llvm/test/Transforms/NewGVN/simp-to-self.ll
index fb8a01962959bc..97d70b52103b36 100644
--- a/llvm/test/Transforms/NewGVN/simp-to-self.ll
+++ b/llvm/test/Transforms/NewGVN/simp-to-self.ll
@@ -1,13 +1,24 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
 ; RUN: opt -S < %s -passes=newgvn | FileCheck %s
 
-; CHECK-LABEL: for.cond:
-; CHECK-NEXT:    %lv = load i32, ptr @a
-; CHECK-NEXT:    %bf.clear = and i32 %lv, -131072
-; CHECK-NEXT:    %bf.set = or i32 1, %bf.clear
-; CHECK-NEXT:    br i1 %bc, label %for.cond, label %exit
 @a = external global i64
 
 define void @fn1(i1 %bc) {
+; CHECK-LABEL: define void @fn1(
+; CHECK-SAME: i1 [[BC:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_COND:%.*]]
+; CHECK:       for.cond:
+; CHECK-NEXT:    [[LV:%.*]] = load i32, ptr @a, align 4
+; CHECK-NEXT:    [[BF_CLEAR:%.*]] = and i32 [[LV]], -131072
+; CHECK-NEXT:    [[BF_SET:%.*]] = or i32 1, [[BF_CLEAR]]
+; CHECK-NEXT:    [[BF_CLEAR_1:%.*]] = and i32 [[BF_SET]], -131072
+; CHECK-NEXT:    [[BF_SET_1:%.*]] = or i32 1, [[BF_CLEAR_1]]
+; CHECK-NEXT:    br i1 [[BC]], label [[FOR_COND]], label [[EXIT:%.*]]
+; CHECK:       exit:
+; CHECK-NEXT:    store i32 [[BF_SET_1]], ptr @a, align 4
+; CHECK-NEXT:    ret void
+;
 entry:
   br label %for.cond

nunoplopes · 2024-02-18T12:51:36Z

The buildbot complains about 2 failing tests:

Transforms/NewGVN/br-identical.ll
Transforms/NewGVN/completeness.ll

llvm/lib/Transforms/Scalar/NewGVN.cpp

nunoplopes · 2024-02-18T13:01:57Z

The change looks good in general. But the buildbot failures need to be investigated first.

alinas

Minor comments inline.

llvm/lib/Transforms/Scalar/NewGVN.cpp

github-actions · 2024-03-01T11:40:47Z

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

ManuelJBrito · 2024-03-05T11:41:49Z

The failing tests were because the members of the class need to be added to the worklist when the leader changes.
Also I removed the phi-of-ops test case. Phi of ops does not use the standard class leader. It goes through the members of the class looking for a dominating leader for the phi translated instruction. Adding the min RPO requirement to this causes some regressions.

ManuelJBrito · 2024-07-23T15:58:40Z

ping

nunoplopes

LGTM

ManuelJBrito requested review from fhahn, alinas and nunoplopes February 17, 2024 14:22

llvmbot added the llvm:transforms label Feb 17, 2024

ManuelJBrito linked an issue Feb 17, 2024 that may be closed by this pull request

clang: 18: Assertion `ProcessedCount[V] < 100 && "Seem to have processed the same Value a lot"' failed. #69211

Open

nunoplopes reviewed Feb 18, 2024

View reviewed changes

llvm/lib/Transforms/Scalar/NewGVN.cpp Outdated Show resolved Hide resolved

alinas reviewed Feb 27, 2024

View reviewed changes

llvm/lib/Transforms/Scalar/NewGVN.cpp Show resolved Hide resolved

llvm/lib/Transforms/Scalar/NewGVN.cpp Show resolved Hide resolved

llvm/lib/Transforms/Scalar/NewGVN.cpp Show resolved Hide resolved

llvm/lib/Transforms/Scalar/NewGVN.cpp Show resolved Hide resolved

ManuelJBrito force-pushed the RPOLeader branch from f3e1ec2 to 16217ce Compare July 12, 2024 00:12

RPO Leader

dec6f01

ManuelJBrito force-pushed the RPOLeader branch from 16217ce to dec6f01 Compare July 13, 2024 15:18

ManuelJBrito removed a link to an issue Jul 13, 2024

clang: 18: Assertion `ProcessedCount[V] < 100 && "Seem to have processed the same Value a lot"' failed. #69211

Open

nunoplopes approved these changes Aug 10, 2024

View reviewed changes

ManuelJBrito merged commit 829992c into llvm:main Aug 14, 2024
8 checks passed

ManuelJBrito mentioned this pull request May 26, 2025

[NewGVN] Fix cyclic dependency in phi-of-ops #141510

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number #82110

[NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number #82110

Uh oh!

ManuelJBrito commented Feb 17, 2024 •

edited

Loading

Uh oh!

llvmbot commented Feb 17, 2024

Uh oh!

nunoplopes commented Feb 18, 2024

Uh oh!

Uh oh!

nunoplopes commented Feb 18, 2024

Uh oh!

alinas left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 1, 2024 •

edited

Loading

Uh oh!

ManuelJBrito commented Mar 5, 2024

Uh oh!

ManuelJBrito commented Jul 23, 2024

Uh oh!

nunoplopes left a comment

Uh oh!

Uh oh!

Uh oh!

[NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number #82110

[NewGVN] Prevent cyclic dependencies by ensuring Leader has min RPO number #82110

Uh oh!

Conversation

ManuelJBrito commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 17, 2024

Uh oh!

nunoplopes commented Feb 18, 2024

Uh oh!

Uh oh!

nunoplopes commented Feb 18, 2024

Uh oh!

alinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManuelJBrito commented Mar 5, 2024

Uh oh!

ManuelJBrito commented Jul 23, 2024

Uh oh!

nunoplopes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ManuelJBrito commented Feb 17, 2024 •

edited

Loading

github-actions bot commented Mar 1, 2024 •

edited

Loading