[PATCH] D39781: [GVNHoist] Fix: PR35222 gvn-hoist incorrectly erases load

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added inline comments.


================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:801
+          const BasicBlock *B = V[i]->getParent();
+          if (DT->dominates(IDFB, V[i]->getParent()) && // Ignore spurious PDFs.
+              LInfo->getLoopFor(IDFB) == LInfo->getLoopFor(B)) {
----------------
dberlin wrote:
> I don't think this check is overall safe.
> 
> The definition of loop here is not going to be sufficient either.
> 
> I think if you want to avoid spurious CHI's, you need to do the equivalent of what you see in "A Practical Improvement to the Partial Redundancy Elimination in SSA Form" (https://sites.google.com/site/jongsoopark/home/ssapre.pdf)
> 
> which precisely defines which will be unnecessary and the safety conditions.
> 
> 
> 
> 
Thanks for the reference, I'll study the paper and find out if they have some extra safety checks.


https://reviews.llvm.org/D39781