[PATCH] D73804: [GVN] Add GVNOption to control load-pre more fine-grained.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked an inline comment as done.
asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1406
     return false;
+  if (!isLoadInLoopPREEnabled() && this->LI &&
+      this->LI->getLoopFor(LI->getParent()))
----------------
fhahn wrote:
> hgreving wrote:
> > fhahn wrote:
> > > nit: drop unnecessary this-> ?
> > This is testing the LoopInfo, not the LoadInst. Having said that, I considered changing the this->LI name into sth else since LI shadows LI in GVN::runImpl as well, but thought this is an unrelated change. Happy to submit this as well.
> Ah right, I didn't look carefully enough. It seems a very unfortunate name clash, but I guess this->LI is fine.
I thought to suggest renaming of either the LoopInfo or LoadInst instances in a separate cleanup patch, but thought that's more than you intended to do. 

Happy to look over the patch, if you do send it out. Sadly, LoopInfo has only a couple uses here, but it's traditionally named LI (I found a single declaration called LF), and LoadInst has a very large number of uses here, and can be named L, I, LI, LoadI etc. 
So, *shrug* use your best judgment if either of renamings is worth doing.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D73804