[PATCH] D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops

[Philip Reames via Phabricator via llvm-commits]

reames marked an inline comment as done.
reames added a comment.

In https://reviews.llvm.org/D50925#1211015, @anna wrote:

> There are 3 kinds of tests worth adding:
>
> 1. predicated invariant stores, i.e. the block containing the store itself is predicated and not guaranteed to execute (cannot be handled by LICM)


Covered by existing early exit test.

> 2. invariant store value is a phi containing invariant incoming values and the phi result depends on an invariant condition (can be handled by LICM. This patch handles?)

Unclear what you mean here.

> 3. invariant store value is a phi containing  invariant incoming values and the phi result depends on a variant condition (cannot be handled by LICM safely)

Again, I don't know what you mean by this.  If the value is a phi in the loop, it's by definition not invariant.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:748
+    // load store promotion instead.
+    auto &AS = CurAST->getAliasSetFor(MemoryLocation::get(SI));
+
----------------
anna wrote:
> I might be missing something. Here we're only checking for the address we're storing into is invariant. Where's the check for the stored value is invariant? That decides if we should hoist or sink the store right?
> 
> Also, could you pls add test cases where the stored value is variant while the store address is invariant.
This is handled in the caller.  See conditional logic before call to hoist.  We check that *all* operands are loop invariant.  

As for the test, see the existing neg_lv_value.  


https://reviews.llvm.org/D50925