[PATCH] D56515: [GVN] Check the EnablePRE flag prior to calling performScalarPRE

[Matt Davis via Phabricator via llvm-commits]

mattd created this revision.
mattd added reviewers: efriedma, t.p.northover.

The `EnablePRE` flag is responsible for controlling access to both the `BlockRPONumber` map and to the routine that uses the map (`performScalarPRE`).  While investigating an issue, it became apparent that there is a path to `performScalarPRE` that does not check the `EnablePRE` flag, and in that case we cannot guarantee `BlockRPONumber` is populated correctly.  This patch introduces a guard along the path that accesses `performScalarPRE`.

This patch is based on a discussion at:  https://reviews.llvm.org/D55974


https://reviews.llvm.org/D56515

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
@@ -2143,6 +2145,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() ||


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D56515.180937.patch
Type: text/x-patch
Size: 1980 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190109/b268f142/attachment-0001.bin>