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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

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.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1012
@@ +1011,3 @@
+        if (!GuaranteedToExecute && !CanSpeculateLoad) {
+          CanSpeculateLoad =
+            isDereferenceableAndAlignedPointer(Store->getPointerOperand(),
----------------
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?


http://reviews.llvm.org/D16783