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
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
