[PATCH] D59401: Fix non-determinism in Reassociate caused by address coincidences

[Daniel Sanders via Phabricator via llvm-commits]

dsanders created this revision.
dsanders added reviewers: rtereshin, bogner.
Herald added a subscriber: mgrang.
Herald added a project: LLVM.

Between building the pair map and querying it there are a few places that
erase and create Values. It's rare but the address of these newly created
Values is occasionally the same as a just-erased Value that we already
have in the pair map. These coincidences should be accounted for to avoid
non-determinism.


Repository:
  rL LLVM

https://reviews.llvm.org/D59401

Files:
  include/llvm/Transforms/Scalar/Reassociate.h
  lib/Transforms/Scalar/Reassociate.cpp


Index: lib/Transforms/Scalar/Reassociate.cpp
===================================================================
--- lib/Transforms/Scalar/Reassociate.cpp
+++ lib/Transforms/Scalar/Reassociate.cpp
@@ -2217,8 +2217,15 @@
         if (std::less<Value *>()(Op1, Op0))
           std::swap(Op0, Op1);
         auto it = PairMap[Idx].find({Op0, Op1});
-        if (it != PairMap[Idx].end())
-          Score += it->second;
+        if (it != PairMap[Idx].end()) {
+          // Functions like BreakUpSubtract() can erase the Values we're using
+          // as keys and create new Values after we built the PairMap. There's a
+          // small chance that the new nodes can have the same address as
+          // something already in the table. We shouldn't accumulate the stored
+          // score in that case as it refers to the wrong Value.
+          if (it->second.isValid())
+            Score += it->second.Score;
+        }
 
         unsigned MaxRank = std::max(Ops[i].Rank, Ops[j].Rank);
         if (Score > Max || (Score == Max && MaxRank < BestRank)) {
@@ -2287,9 +2294,15 @@
             std::swap(Op0, Op1);
           if (!Visited.insert({Op0, Op1}).second)
             continue;
-          auto res = PairMap[BinaryIdx].insert({{Op0, Op1}, 1});
-          if (!res.second)
-            ++res.first->second;
+          auto res = PairMap[BinaryIdx].insert({{Op0, Op1}, {Op0, Op1, 1}});
+          if (!res.second) {
+            // If either key value has been erased then we've got the same
+            // address by coincidence. That can't happen here because nothing is
+            // erasing values but it can happen by the time we're querying the
+            // map.
+            assert(res.first->second.isValid() && "WeakVH invalidated");
+            ++res.first->second.Score;
+          }
         }
       }
     }
Index: include/llvm/Transforms/Scalar/Reassociate.h
===================================================================
--- include/llvm/Transforms/Scalar/Reassociate.h
+++ include/llvm/Transforms/Scalar/Reassociate.h
@@ -82,7 +82,14 @@
   static const unsigned GlobalReassociateLimit = 10;
   static const unsigned NumBinaryOps =
       Instruction::BinaryOpsEnd - Instruction::BinaryOpsBegin;
-  DenseMap<std::pair<Value *, Value *>, unsigned> PairMap[NumBinaryOps];
+
+  struct PairMapValue {
+    WeakVH Value1;
+    WeakVH Value2;
+    unsigned Score;
+    bool isValid() const { return Value1 && Value2; }
+  };
+  DenseMap<std::pair<Value *, Value *>, PairMapValue> PairMap[NumBinaryOps];
 
   bool MadeChange;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59401.190766.patch
Type: text/x-patch
Size: 2565 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190315/35b87347/attachment.bin>