fixing return value of performScalarPRE() ignored issue

[vincentqiuuu]

Summary

The return value of performScalarPRE() at its call site in function bool GVN::processNonLocalLoad(LoadInst *LI) is not taken into account. This patch fixes the issue.

[mehdi_amini]

Thanks for the patch, there is a broken test in the validation with this patch: test/Transforms/GVN/pre-gep-load.ll

Can you have a look?

[mehdi_amini]

Note: It is very likely that the test just need to be updated, but you may want to be sure why.

[vincentqiuuu]

Okay I'll take a look.

Le vendredi 11 décembre 2015, Mehdi AMINI <[e-mail]> a écrit :

joker.eph added a comment.

Note: It is very likely that the test just need to be updated, but you may
want to be sure why.

http://reviews.llvm.org/D14899

[vincentqiuuu]

After studying the test case that failed and GVN.cpp, I found that int the function processNonLocalLoad(LoadInst *LI), not taking the return value of performScalarPRE(I) into account is actually correct. It's not a bug because performScalarPRE(I) is just a step of analyzing when the first operand of the LoadInst is a GEP, which means the LoadInst follows a GEP. performScalarPRE(I) can return true (the GEP is thus eliminated) whereas the LoadInst is not eliminated and thus processNonLocalLoad(LoadInst *LI) should return false.

[• dberlin]

I was about to tell you what you just discovered :)

THe real issue here is that we are inconsistent in what the return values
of these functions mean.
processNonLocalLoad is not returning whether it changed, but whether it
deleted the load.
processLoad in turn is also returning whether it deleted the load.

However, processInstruction then uses this to determine if the function has
changed ...

If you want to make these functions return whether anything changed, i'm
not opposed, but you have to carefully go through the logic separate the
two things it uses the return values for ;)

lib/Transforms/Scalar/GVN.cpp

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.
+  bool Changed = false;
   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);
+        Changed |= 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.
   if (ValuesPerBlock.empty())
-    return false;
+    return Changed;
 
   // Step 3: Eliminate fully redundancy.
   //
   // If all of the instructions we depend on produce a known value for this
   // load, then it is fully redundant and we can use PHI insertion to compute
   // its value.  Insert PHIs and remove the fully redundant value now.
   if (UnavailableBlocks.empty()) {
     DEBUG(dbgs() << "GVN REMOVING NONLOCAL LOAD: " << *LI << '\n');
if (V->getType()->getScalarType()->isPointerTy())
       MD->invalidateCachedPointerInfo(V);
     markInstructionForDeletion(LI);
     ++NumGVNLoad;
     return true;
   }
 
   // Step 4: Eliminate partial redundancy.
   if (!EnablePRE || !EnableLoadPRE)
-    return false;
+    return Changed;
 
-  return PerformLoadPRE(LI, ValuesPerBlock, UnavailableBlocks);
+  Changed |= PerformLoadPRE(LI, ValuesPerBlock, UnavailableBlocks);
+  return Changed;
 }
 
 bool GVN::processAssumeIntrinsic(IntrinsicInst *IntrinsicI) {
   assert(IntrinsicI->getIntrinsicID() == Intrinsic::assume &&
          "This function can only be called with llvm.assume intrinsic");
   Value *V = IntrinsicI->getArgOperand(0);
 
   if (ConstantInt *Cond = dyn_cast<ConstantInt>(V)) {