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

[John McCall via Phabricator via cfe-commits]

rjmccall added a reviewer: Prazek.
rjmccall added a comment.

In https://reviews.llvm.org/D53295#1271590, @Anastasia wrote:

> Btw, blocks w/o captures are already optimized into regular calls?


That's a very easy optimization for the optimizer to do because the global can be marked constant.



================
Comment at: lib/CodeGen/CGBlocks.cpp:1318
+        CGM.getModule().getMDKindID("invariant.load"),
+        llvm::MDNode::get(getLLVMContext(), None));
+
----------------
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?


================
Comment at: lib/CodeGen/CGBlocks.cpp:980
+  auto storeField = [&](llvm::Value *value, unsigned index, CharUnits offset,
+                        const Twine &name, bool IsInvariant) {
+    auto *ST = Builder.CreateStore(value, projectField(index, offset, name));
----------------
Please match the capitalization of local variable names used in the surrounding code.


================
Comment at: test/CodeGenOpenCL/blocks-indirect-call.cl:11
+// NOOPT: load {{.*}}%[[INV]]{{.*}}, !invariant.group
+// ToDo: Fix LLVM optimizations to lower indirect function call
+// OPT: %[[INV:.*]] = phi {{.*}}[ @__blockInLoopCondition_block_invoke, %entry ]
----------------
I don't know what this TODO means.


https://reviews.llvm.org/D53295