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

[hfinkel at anl.gov]

hfinkel added inline comments.

================
Comment at: lib/CodeGen/CGExpr.cpp:1286
@@ -1283,1 +1285,3 @@
+    Value = Builder.CreateNoAlias(Value, NAI->second);
+
   if (Ty->isAtomicType() ||
----------------
rjmccall wrote:
> 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.
No, this is not correct. If I have code like this:

  double * restrict addr = get_addr();
  double value = get_value();
  *addr = value;

then we don't get code like this:

  %addr = call double* @get_addr()
  %value = call double get_value()
  %value2 = call @llvm.noalias(double %value)
  store %value2, %addr

The code that Clang generates does not look like that: it does not generate an SSA variable for %addr, it generates a local stack location for it, and then loads/stores from that location when accessed. The only address that appear in NoAliasAddrMap are the address of the local allocas for the restrict-qualified variables themselves. It is only the pointer values being stored into the local restrict-qualified variables themselves that get wrapped.

If you look at the regression test, I think it is clear that the code does the right thing. (If that's not clear, I should improve the test.)

================
Comment at: lib/CodeGen/CGExpr.cpp:1286
@@ -1283,1 +1285,3 @@
+    Value = Builder.CreateNoAlias(Value, NAI->second);
+
   if (Ty->isAtomicType() ||
----------------
hfinkel wrote:
> rjmccall wrote:
> > 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.
> No, this is not correct. If I have code like this:
> 
>   double * restrict addr = get_addr();
>   double value = get_value();
>   *addr = value;
> 
> then we don't get code like this:
> 
>   %addr = call double* @get_addr()
>   %value = call double get_value()
>   %value2 = call @llvm.noalias(double %value)
>   store %value2, %addr
> 
> The code that Clang generates does not look like that: it does not generate an SSA variable for %addr, it generates a local stack location for it, and then loads/stores from that location when accessed. The only address that appear in NoAliasAddrMap are the address of the local allocas for the restrict-qualified variables themselves. It is only the pointer values being stored into the local restrict-qualified variables themselves that get wrapped.
> 
> If you look at the regression test, I think it is clear that the code does the right thing. (If that's not clear, I should improve the test.)
> Yes, we already store when l-values are volatile, so storing that they're restrict should be easy.

FWIW, LValue already keeps track of whether or not the value is restrict qualified (along with whether or not it is volatile). I'll see if I can predicate the map lookup on that, which may be the best solution (does not increase the size of LValue for an uncommon case, but also does not perform a map lookup for every store).



http://reviews.llvm.org/D9403