[PATCH] D32990: [NewGVN] Take in account incoming edges computing congruent PhiExpression(s)

[Davide Italiano via Phabricator via llvm-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL302552: [NewGVN] Fix a consistent order for phi nodes operands. (authored by davide).

Changed prior to commit:
  https://reviews.llvm.org/D32990?vs=98312&id=98317#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32990

Files:
  llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
  llvm/trunk/test/Transforms/NewGVN/pr32952.ll


Index: llvm/trunk/test/Transforms/NewGVN/pr32952.ll
===================================================================
--- llvm/trunk/test/Transforms/NewGVN/pr32952.ll
+++ llvm/trunk/test/Transforms/NewGVN/pr32952.ll
@@ -0,0 +1,42 @@
+; PR32952: Don't erroneously consider congruent two phi nodes which
+; have the same arguments but different incoming edges.
+; RUN: opt -newgvn -S %s | FileCheck %s
+
+ at a = common global i16 0, align 2
+ at .str = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
+
+define i32 @tinkywinky() {
+entry:
+  %0 = load i16, i16* @a, align 2
+  %conv = sext i16 %0 to i32
+  %neg = xor i32 %conv, -1
+  %conv1 = trunc i32 %neg to i16
+  %conv3 = zext i16 %conv1 to i32
+  %cmp = icmp slt i32 %conv, %conv3
+  br i1 %cmp, label %tinky, label %winky
+
+tinky:
+  store i16 2, i16* @a, align 2
+  br label %patatino
+
+winky:
+  br label %patatino
+
+patatino:
+; CHECK: %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ]
+; CHECK: %banana = phi i16 [ %0, %tinky ], [ %conv1, %winky ]
+  %meh = phi i16 [ %0, %winky ], [ %conv1, %tinky ]
+  %banana = phi i16 [ %0, %tinky ], [ %conv1, %winky ]
+  br label %end
+
+end:
+; CHECK: %promoted = zext i16 %banana to i32
+; CHECK: %other = zext i16 %meh to i32
+  %promoted = zext i16 %banana to i32
+  %other = zext i16 %meh to i32
+  %first = tail call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i32 %promoted)
+  %second = tail call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i32 %other)
+  ret i32 0
+}
+
+declare i32 @printf(i8*, ...)
Index: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
===================================================================
--- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
+++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
@@ -730,23 +730,35 @@
 
   unsigned PHIRPO = RPOOrdering.lookup(DT->getNode(PHIBlock));
 
+  // NewGVN assumes the operands of a PHI node are in a consistent order across
+  // PHIs. LLVM doesn't seem to always guarantee this. While we need to fix
+  // this in LLVM at some point we don't want GVN to find wrong congruences.
+  // Therefore, here we sort uses in predecessor order.
+  SmallVector<const Use *, 4> PHIOperands;
+  for (const Use &U : PN->operands())
+    PHIOperands.push_back(&U);
+  std::sort(PHIOperands.begin(), PHIOperands.end(),
+            [&](const Use *U1, const Use *U2) {
+              return PN->getIncomingBlock(*U1) < PN->getIncomingBlock(*U2);
+            });
+
   // Filter out unreachable phi operands.
-  auto Filtered = make_filter_range(PN->operands(), [&](const Use &U) {
-    return ReachableEdges.count({PN->getIncomingBlock(U), PHIBlock});
+  auto Filtered = make_filter_range(PHIOperands, [&](const Use *U) {
+    return ReachableEdges.count({PN->getIncomingBlock(*U), PHIBlock});
   });
 
   std::transform(Filtered.begin(), Filtered.end(), op_inserter(E),
-                 [&](const Use &U) -> Value * {
-                   auto *BB = PN->getIncomingBlock(U);
+                 [&](const Use *U) -> Value * {
+                   auto *BB = PN->getIncomingBlock(*U);
                    auto *DTN = DT->getNode(BB);
                    if (RPOOrdering.lookup(DTN) >= PHIRPO)
                      HasBackedge = true;
-                   AllConstant &= isa<UndefValue>(U) || isa<Constant>(U);
+                   AllConstant &= isa<UndefValue>(*U) || isa<Constant>(*U);
 
                    // Don't try to transform self-defined phis.
-                   if (U == PN)
+                   if (*U == PN)
                      return PN;
-                   return lookupOperandLeader(U);
+                   return lookupOperandLeader(*U);
                  });
   return E;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D32990.98317.patch
Type: text/x-patch
Size: 3757 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170509/99aa9485/attachment.bin>