[PATCH] D27939: [LICM] Report failing to hoist a load with an invariant address

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:477
+        pointerInvalidatedByLoop(LI->getOperand(0), Size, AAInfo, CurAST);
+    if (ORE && Invalidated && CurLoop->isLoopInvariant(LI->getPointerOperand()))
+      ORE->emit(OptimizationRemarkMissed(
----------------
anemet wrote:
> hfinkel wrote:
> > anemet wrote:
> > > anemet wrote:
> > > > hfinkel wrote:
> > > > > Can we hoist a load without a loop-invariant address? It seems like we shouldn't be making aliasing queries for loads we can't possibly hoist, and so I don't understand why we specifically need the `CurLoop->isLoopInvariant` check here.
> > > > No, you're right.  The call to this is guarded with hasLoopInvariantOperands.
> > > Actually, I take this back.  We need the check.  See the new comment in the new version of the patch and also the new testcase for an example.
> > Okay, but I wonder if this points to a larger problem. Maybe we just need a FIXME on this comment for now, but I'm thinking that this invariance check is cheap and the other checks (i.e. aliasing queries, dereferenceablility checks, etc.) are, at least in some cases, expensive. Thus, this guard really belongs elsewhere. We can sink something with a non-loop-invariant store either, can we?
> Yes but we *can* sink a load with a loop-variant address whose result is only used outside the loop (see testcase).  
> 
> Loop-invariance is not tested in this case (because it shouldn't) however it is tested on other paths to this code (as it should) so I don't think there is a problem of wasting work.
Ah, okay. That makes sense, thanks! LGTM.


https://reviews.llvm.org/D27939