[PATCH] D31539: Hoisting invariant.group in LICM

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:571
 
+    if (isLoadInvariantGroupInLoop(LI, DT, CurLoop))
+      return true;
----------------
sanjoy wrote:
> I'm not sure that that the langref lets you do this.  What it allows for is
> 
> ```
> %a = *ptr, !invariant.group
> %b = *ptr, !invariant.group
> ```
> 
> to
> 
> ```
> %a = *ptr, !invariant.group
> %b = %a
> ```
> 
> which is slightly different from what you're doing here.
> 
> So I'd recommend changing the langref wording to be more like what we have for `!invariant.load`.
I am not sure how I can make it more clear in LangRef, but I am happy to change that if you have any ideas.

Right now it says " The existence of the invariant.group metadata on the instruction tells the optimizer that every load and store to the same pointer operand within the same invariant group can be assumed to load or store the same value"

so if my pointer operand doesn't change in any loop step, then I guess it works.
The other interpretation is that unrolling the loop one time and then optimizing load in the loop based on invariant.group is exactly the same.

But I agree that the docs shoud probably mention that it has to be executed etc.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:877
       // drop.  It is a compile time optimization, not required for correctness.
       !isGuaranteedToExecute(I, DT, CurLoop, SafetyInfo))
+    I.dropUnknownNonDebugMetadata({LLVMContext::MD_invariant_group,
----------------
sanjoy wrote:
> This bit does not look correct.  Why can't these attributes be control dependent?
Good catch, I didn't think about it because this works for devirtualization. 
This means that we need a way to say that given property holds globally. See mailing list


https://reviews.llvm.org/D31539