[PATCH] D124775: [IRSim] Removing check that caused early matching of commutative instructions

[Andrew Litteken via Phabricator via llvm-commits]

AndrewLitteken created this revision.
AndrewLitteken added a reviewer: paquette.
Herald added a subscriber: hiraditya.
Herald added a project: All.
AndrewLitteken requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

When the first commutative instruction in a region using the same value in both positions was compared to a corresponding instruction with two different values, there was an early check that determined that since the values were new, it was true that these values acted in the same way structurally.  If this was not contradicted later in the program, the regions were marked as similar.  This removes that check, so that it is clear that the same value cannot be mapped to two different values.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124775

Files:
  llvm/lib/Analysis/IRSimilarityIdentifier.cpp
  llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp


Index: llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp
===================================================================
--- llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp
+++ llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp
@@ -2619,6 +2619,28 @@
   }
 }
 
+// This test ensures that when the first instruction in a sequence is
+// a commutative instruction with the same value, but the corresponding
+// instruction is not, we mark the regions and not similar.
+TEST(IRSimilarityIdentifier, CommutativeSameValueFirstMisMatch) {
+  StringRef ModuleString = R"(
+                          define void @v_1_0(i64 %v_33) {
+                            entry:
+                              %mul44 = mul i64 undef, undef
+                              %add = add i64 %mul44, %v_33
+                              %mul.il = mul i64 0, undef
+                              %mul.i = add i64 %mul.il, %mul.il
+                              unreachable
+                            })";
+  LLVMContext Context;
+  std::unique_ptr<Module> M = makeLLVMModule(Context, ModuleString);
+
+  std::vector<std::vector<IRSimilarityCandidate>> SimilarityCandidates;
+  getSimilarities(*M, SimilarityCandidates);
+
+  ASSERT_TRUE(SimilarityCandidates.size() == 0);
+}
+
 // This test makes sure that intrinsic functions that are marked commutative
 // are still treated as non-commutative since they are function calls.
 TEST(IRSimilarityIdentifier, InstrinsicCommutative) {
Index: llvm/lib/Analysis/IRSimilarityIdentifier.cpp
===================================================================
--- llvm/lib/Analysis/IRSimilarityIdentifier.cpp
+++ llvm/lib/Analysis/IRSimilarityIdentifier.cpp
@@ -526,19 +526,13 @@
   for (Value *V : SourceOperands) {
     ArgVal = SourceValueToNumberMapping.find(V)->second;
 
+    // Instead of finding a current mapping, we attempt to insert a set.
     std::tie(ValueMappingIt, WasInserted) = CurrentSrcTgtNumberMapping.insert(
         std::make_pair(ArgVal, TargetValueNumbers));
 
-    // Instead of finding a current mapping, we inserted a set.  This means a
-    // mapping did not exist for the source Instruction operand, it has no
-    // current constraints we need to check.
-    if (WasInserted)
-      continue;
-
-    // If a mapping already exists for the source operand to the values in the
-    // other IRSimilarityCandidate we need to iterate over the items in other
-    // IRSimilarityCandidate's Instruction to determine whether there is a valid
-    // mapping of Value to Value.
+    // We need to iterate over the items in other IRSimilarityCandidate's
+    // Instruction to determine whether there is a valid mapping of
+    // Value to Value.
     DenseSet<unsigned> NewSet;
     for (unsigned &Curr : ValueMappingIt->second)
       // If we can find the value in the mapping, we add it to the new set.
@@ -558,7 +552,6 @@
     if (ValueMappingIt->second.size() != 1)
       continue;
 
-
     unsigned ValToRemove = *ValueMappingIt->second.begin();
     // When there is only one item left in the mapping for and operand, remove
     // the value from the other operands.  If it results in there being no


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D124775.426428.patch
Type: text/x-patch
Size: 3174 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220502/1ae1ef94/attachment.bin>