[PATCH] D53295: Mark store and load of block invoke function as invariant.group

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1318
+        CGM.getModule().getMDKindID("invariant.load"),
+        llvm::MDNode::get(getLLVMContext(), None));
+
----------------
rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > OpenCL blocks are still potentially function-local, right?  I don't think you're allowed to put `invariant.load` on something that's visibly initialized, even if it's visibly initialized to the same thing every time.  The problem is that `invariant.load` could allow the load to be hoisted above the initialization.
> > > 
> > > If you can solve that problem, you can make this non-OpenCL-specific.
> > It seems that invariant.load implies the pointer is invariant in the whole module, disregarding the store to it, which is not suitable for this case.
> > 
> > In this case what I need is that after the first store to the pointer, every load does not change.
> > 
> > It seems invariant.group can be used for this.
> Hmm.  I guess `invariant.group` does what you need because LLVM will probably unify the GEPs to extract the function-pointer field if they appear in the same function, and there are no launderings of that pointer.
> 
> CC'ing Piotr to get his opinion about whether this is a correct use of `invariant.group`.  Piotr, we have a struct-typed alloca that we initialize in a probably-dominating position.  A pointer to the struct can be passed off to other contexts which we can't necessarily analyze, blocking normal optimization; however, we know that this struct can (mostly) not be legally modified after initialization.  We're marking one of the initializing stores as well as the load of the corresponding field when it's used.  These two places are completely separate and will perform their own GEPs, but because we don't emit any `launder`s on the struct, LLVM will likely unify the GEPs if they appear in the same function, allowing `invariant.group`-based optimizations to apply.  Does this seem reasonable?
Yep, seems totally reasonable. Sorry for such a late response, I just have open reviews and saw that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53295/new/

https://reviews.llvm.org/D53295