[NewGVN] Make sure we do not add a user to itself.

[fhahn]

Summary

If we simplify an instruction to itself, we do not need to add a user to
itself. For congruence classes with a defining expression, we already
use a similar logic.

Fixes PR38259.

[fhahn]

ping

[fhahn]

ping

[fhahn]

ping

[fhahn]

ping

[fhahn]

ping

[efriedma]

I don't really know the relevant code well enough, and I don't think I'll have time to study it in the near future.

You might want to ask on llvmdev to see if there's anyone interested in reviewing NewGVN patches.

[davide]

I'll take a look early next week. Sorry, I don't work on scalar optimizations anymore on llvm these days, so I need to page this code in again.

[fhahn]

In D51168#1283117, @efriedma wrote:

I don't really know the relevant code well enough, and I don't think I'll have time to study it in the near future.

You might want to ask on llvmdev to see if there's anyone interested in reviewing NewGVN patches.

@efriedma thanks for all the reviews you did already for NewGVN related changes, I really appreciate it! Thanks for the suggestion, I guess I could prepare a few additional patches and send some kind of status update with them to llvmdev.

In D51168#1283253, @davide wrote:

I'll take a look early next week. Sorry, I don't work on scalar optimizations anymore on llvm these days, so I need to page this code in again.

That would be great!

[davide]

LGTM.

[fhahn]

Thank you very much Davide :) In case you are interested in reviewing another small NewGVN patch, there would be D51595.

lib/Transforms/Scalar/NewGVN.cpp

if (I)
                         << " variable " << *V << "\n");
     deleteExpression(E);
     return createVariableExpression(V);
   }
 
   CongruenceClass *CC = ValueToClass.lookup(V);
   if (CC) {
     if (CC->getLeader() && CC->getLeader() != I) {
+      // If we simplified to something else, we need to communicate
+      // that we're users of the value we simplified to.
+      if (I != V) {
         // Don't add temporary instructions to the user lists.
         if (!AllTempInstructions.count(I))
           addAdditionalUsers(V, I);
+      }
       return createVariableOrConstant(CC->getLeader());
     }
     if (CC->getDefiningExpr()) {
       // If we simplified to something else, we need to communicate
       // that we're users of the value we simplified to.
       if (I != V) {
         // Don't add temporary instructions to the user lists.
         if (!AllTempInstructions.count(I))

test/Transforms/NewGVN/simp-to-self.ll

+; RUN: opt -S < %s -newgvn | FileCheck %s
+
+; CHECK-LABEL: for.cond:
+; CHECK-NEXT:    %lv = load i32, i32* bitcast (i64* @a to i32*)
+; 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) {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond1.1, %entry
+  %tmp = phi i1 [ undef, %entry ], [ 1, %for.cond ]
+  %conv = zext i1 %tmp to i32
+  %lv = load i32, i32* bitcast (i64* @a to i32*)
+  %bf.clear = and i32 %lv, -131072
+  %bf.set = or i32 %conv, %bf.clear
+  %bf.clear.1 = and i32 %bf.set, -131072
+  %bf.set.1 = or i32 1, %bf.clear.1
+  br i1 %bc, label %for.cond, label %exit
+
+exit:                              ; preds = %for.cond1
+  store i32 %bf.set.1, i32* bitcast (i64* @a to i32*)
+  ret void
+}