[PATCH] D16783: [LICM] Store promotion when memory is thread local

[Philip Reames via llvm-commits]

reames added a comment.

In http://reviews.llvm.org/D16783#368332, @sanjoy wrote:

> In http://reviews.llvm.org/D16783#368247, @reames wrote:
>
> > In http://reviews.llvm.org/D16783#367396, @sanjoy wrote:
> >
> > > Overall the logic lgtm.  However, should this be disabled when optimizing for size?
> >
> >
> > Why would the thread local version be disabled when optimizing for size and not the existing code?  That doesn't seem to make any sense.
>
>
> Given that you're potentially duplicating a store, if this change
>  makes the promotion fire a lot more often //maybe// that's bad for
>  `-Os`.  But I have no hard numbers for this.


I'd argue that either the whole transform should be restricted in Os, or not.  My change isn't a change to any of the profitability heuristics, just a legality one.

p.s. Once you've considered my points, can you give another explicit LGTM?  I want to make sure that still stands after our follow on discussion.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1012
@@ +1011,3 @@
+        if (!GuaranteedToExecute && !CanSpeculateLoad) {
+          CanSpeculateLoad =
+            isDereferenceableAndAlignedPointer(Store->getPointerOperand(),
----------------
sanjoy wrote:
> reames wrote:
> > sanjoy wrote:
> > > So part of the confusion here for me is that `CanSpeculateLoad` looks like it is really tracking something else.  Does it make sense to rename it to `CanPromoteAliasSet`?
> > I really not getting the confusion here.  Per the comment on the variable you're asking me to rename, it really is answering only the speculation legality question.  In particular, that by itself is not enough to establish promotion is legal.  That's the whole point.
> > I really not getting the confusion here.
> 
> That just proves you're smarter than I am. :)
> 
> My confusion initially was that you're checking if the store location
> is dereferenceable, but yet use that to decide if the load can be
> speculated.  But now I get that this is really about proving that
> //any// location in the alias set can be speculatively loaded from,
> since they're all MustAlias.
> 
> Just as a understanding check -- if you remove this clause completely,
> that will just be a quality of implementation regression, not a
> correctness issue, right?
Correct.  Using the store to establish dereferenceability lets the transform kick in more often, but is not required for correctness.  

I'll add a clarify comment about the mustalias bit.  That's not obvious from the code and might help to prevent future confusion.  


http://reviews.llvm.org/D16783