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