[PATCH] D9403: llvm.noalias - Clang CodeGen for local restrict-qualified pointers

[John McCall]

rjmccall added inline comments.

================
Comment at: lib/CodeGen/CGExpr.cpp:1286
@@ -1283,1 +1285,3 @@
+    Value = Builder.CreateNoAlias(Value, NAI->second);
+
   if (Ty->isAtomicType() ||
----------------
hfinkel wrote:
> rjmccall wrote:
> > This is very strange.  Why do the aliasing properties of an address cause metadata to be attached to the value stored into it?  Shouldn't the metadata be on the address (in which case, perhaps it should have been attached to the load from the restrict pointer?) or the store?
> > 
> > Also, please find some way to avoid doing a DenseMap lookup on every single store.  I would guess that the address only has an entry in that table if it was restrict-qualified; you can probably find a way to pass that information down.
> > This is very strange. Why do the aliasing properties of an address cause metadata to be attached to the value stored into it? Shouldn't the metadata be on the address (in which case, perhaps it should have been attached to the load from the restrict pointer?) or the store?
> 
> 
> To be clear, this is not the metadata, this is the intrinsic; the metadata needs to be attached to all access in the scope. The intrinsic wraps the 'noalias' address. And this is "wrapping" the restrict-qualified address with the llvm.noalias intrinsic before it is stored into the stack space allocated for the local variable.
> 
> > Also, please find some way to avoid doing a DenseMap lookup on every single store. I would guess that the address only has an entry in that table if it was restrict-qualified; you can probably find a way to pass that information down.
> 
> You are right: some (indirect) caller of this function should see a restrict-qualified variable. Maybe this information should end up in the LValue structure?
> 
> Maybe it is worth now thinking about that should be the follow-on patch, because that also might influence the design. This patch handles direct restrict-qualified local variables, but does not handle the case where the variables are inside a structure. Specifically, I mean this:
> 
>   struct V {
>     double * restrict x;
>     double * restrict y;
>     double * restrict z;
>   };
> 
>   void foo() {
>     V v = { getX(), getY(), getZ() };
>     // The stores that compose this initializer need to be wrapped in @llvm.noalias also.
>   }
> 
> So I'd like to end up with a design for this patch that is easy to extend to handle the restrict-qualified-local-structure-member case as well.
"Value" is the value being stored to the address.

If I've got code like this:
  double * restrict addr = get_addr();
  double value = get_value();
  *addr = value;

you are producing IR like
  %addr = call double* @get_addr()
  %value = call double get_value()
  %value2 = call @llvm.noalias(double %value)
  store %value2, %addr

This is what I'm saying does not make any sense, because there's nothing special about %value; the restrictions are on %addr.  I think you probably just have a bug in your patch and meant to use Addr.

> You are right: some (indirect) caller of this function should see a restrict-qualified variable. Maybe this information should end up in the LValue structure?

Yes, we already store when l-values are volatile, so storing that they're restrict should be easy.


http://reviews.llvm.org/D9403