[PATCH] D55974: [GVN] Initialize BlockRPONumber when prior to use.

[Matt Davis via Phabricator via llvm-commits]

mattd updated this revision to Diff 180168.
mattd retitled this revision from "[GVN] Force a sequence point when comparing two DenseMap values." to "[GVN] Initialize BlockRPONumber when prior to use.".
mattd edited the summary of this revision.
mattd added a comment.

This patch aims to clean-up two items in GVN.

1 - The DenseMap `BlockRPONumber` in GVN should be populated by the time
`performScalarPRE` is called.  That map is initialized when GVN starts, and only
if the `EnablePRE` ("-enable-pre") flag is passed (defaults to true).  It seems
that no new blocks should be introduced to the function while it is being
analyzed if `EnablePRE` is set.  If so, the values in the aforementioned map
might no longer be correct.

It turns out that `performPRE` can introduce new blocks via `splitCriticalEdges`.
`performPRE` can be called multiple times (until a fixed-point is found).  That
routine introduces new blocks into the function being analyzed.  Thus the
`BlockRPONumber` map should be updated when new blocks are added, but the code
currently does not update that map.

The solution in the patch here lazily updates the map just prior to its use.

2 - The other item that I think needs to be corrected was introduced in a more
recent change.  That change introduced a code path that calls `performScalarPRE`
without checking the `EnablePRE` ("-enable-pre") flag.  `EnablePRE` is responsible
for building up the BlockRPONumber map.  If that flag is set to false when the
GVN pass starts, then the `BlockRPONumber` map will be empty.

`EnablePRE` is defaulted to true so it's unlikely someone will ever set it false,
outside of testing/debugging.  This is probably why we we never actually see
case #2 occur (no test exists which tests "-enable-pre=false").  However,
I do think it is reasonable to add an assertion and `EnablePRE` check.

At this point, I'm not convinced we need a test for #1 above, as it is NFCI,
although I have a hard time saying "NFCI," because the mechanics change, but the
intended semantics should remain.  It does make sense to add a test for #2 setting
'-enable-pre=false' (which no tests do) and just checking that nothing asserts.  I
found an existing test that does call `performScalarPRE` even with `EnablePRE` set
to false, and have added a RUN line to test the newly added assertion does not trigger.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55974/new/

https://reviews.llvm.org/D55974

Files:
  lib/Transforms/Scalar/GVN.cpp
  test/Transforms/GVN/nonescaping-malloc.ll


Index: test/Transforms/GVN/nonescaping-malloc.ll
===================================================================
--- test/Transforms/GVN/nonescaping-malloc.ll
+++ test/Transforms/GVN/nonescaping-malloc.ll
@@ -2,6 +2,10 @@
 ; RUN: opt < %s -basicaa -gvn -stats -disable-output 2>&1 | FileCheck %s
 ; rdar://7363102
 
+; Sanity check that GVN::performScalarPRE is not called with
+; enable-pre=false (asserts)
+; RUN: opt < %s -basicaa -gvn -enable-pre=false -disable-output 2>&1
+
 ; CHECK: Number of loads deleted
 
 ; GVN should be able to eliminate load %tmp22.i, because it is redundant with
Index: lib/Transforms/Scalar/GVN.cpp
===================================================================
--- lib/Transforms/Scalar/GVN.cpp
+++ lib/Transforms/Scalar/GVN.cpp
@@ -1328,12 +1328,14 @@
   }
 
   // If this load follows a GEP, see if we can PRE the indices before analyzing.
-  if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(LI->getOperand(0))) {
-    for (GetElementPtrInst::op_iterator OI = GEP->idx_begin(),
-                                        OE = GEP->idx_end();
-         OI != OE; ++OI)
-      if (Instruction *I = dyn_cast<Instruction>(OI->get()))
-        performScalarPRE(I);
+  if (EnablePRE) {
+    if (auto GEP = dyn_cast<GetElementPtrInst>(LI->getOperand(0))) {
+      for (GetElementPtrInst::op_iterator OI = GEP->idx_begin(),
+                                          OE = GEP->idx_end();
+           OI != OE; ++OI)
+        if (Instruction *I = dyn_cast<Instruction>(OI->get()))
+          performScalarPRE(I);
+    }
   }
 
   // Step 2: Analyze the availability of the load
@@ -1645,6 +1647,11 @@
 }
 
 void GVN::assignBlockRPONumber(Function &F) {
+  // Initialize BlockRPONumber if it has never been populated, or
+  // if some blocks have been split and added to F.
+  if (F.size() == BlockRPONumber.size())
+    return;
+  BlockRPONumber.clear();
   uint32_t NextBlockNumber = 1;
   ReversePostOrderTraversal<Function *> RPOT(&F);
   for (BasicBlock *BB : RPOT)
@@ -2021,7 +2028,6 @@
     // Fabricate val-num for dead-code in order to suppress assertion in
     // performPRE().
     assignValNumForDeadCode();
-    assignBlockRPONumber(F);
     bool PREChanged = true;
     while (PREChanged) {
       PREChanged = performPRE(F);
@@ -2143,6 +2149,7 @@
 }
 
 bool GVN::performScalarPRE(Instruction *CurInst) {
+  assert(EnablePRE && "GVN: Calling performScalarPRE with -enable-pre=false.");
   if (isa<AllocaInst>(CurInst) || CurInst->isTerminator() ||
       isa<PHINode>(CurInst) || CurInst->getType()->isVoidTy() ||
       CurInst->mayReadFromMemory() || CurInst->mayHaveSideEffects() ||
@@ -2184,6 +2191,9 @@
   BasicBlock *PREPred = nullptr;
   BasicBlock *CurrentBlock = CurInst->getParent();
 
+  // Update the RPO numbers for this function.
+  assignBlockRPONumber(*CurrentBlock->getParent());
+
   SmallVector<std::pair<Value *, BasicBlock *>, 8> predMap;
   for (BasicBlock *P : predecessors(CurrentBlock)) {
     // We're not interested in PRE where blocks with predecessors that are


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55974.180168.patch
Type: text/x-patch
Size: 3046 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190104/020e6581/attachment.bin>