[GVN] Check the EnablePRE flag prior to calling performScalarPRE

[mattd]

Summary

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

lib/Transforms/Scalar/GVN.cpp

bool GVN::processNonLocalLoad(LoadInst *LI) {
   if (NumDeps == 1 &&
       !Deps[0].getResult().isDef() && !Deps[0].getResult().isClobber()) {
     LLVM_DEBUG(dbgs() << "GVN: non-local load "; LI->printAsOperand(dbgs());
                dbgs() << " has unknown dependencies\n";);
     return false;
   }
 
   // If this load follows a GEP, see if we can PRE the indices before analyzing.
-  if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(LI->getOperand(0))) {
+  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
   AvailValInBlkVect ValuesPerBlock;
   UnavailBlkVect UnavailableBlocks;
   AnalyzeLoadAvailability(LI, Deps, ValuesPerBlock, UnavailableBlocks);
 
   // If we have no predecessors that produce a known value for this load, exit
   // early.
bool GVN::performScalarPREInsertion(Instruction *Instr, BasicBlock *Pred,
   VN.add(Instr, Num);
 
   // Update the availability map to include the new instruction.
   addToLeaderTable(Num, Instr, Pred);
   return true;
 }
 
 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() ||
       isa<DbgInfoIntrinsic>(CurInst))
     return false;
 
   // Don't do PRE on compares. The PHI would prevent CodeGenPrepare from
   // sinking the compare again, and it would force the code generator to

test/Transforms/GVN/nonescaping-malloc.ll

 ; REQUIRES: asserts
 ; 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
 ; load %tmp8.i. This requires being able to prove that %tmp7.i doesn't
 ; alias the malloc'd value %tmp.i20.i.i, which it can do since %tmp7.i
 ; is derived from %tmp5.i which is computed from a load, and %tmp.i20.i.i
 ; is never stored and does not escape.