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:
> 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.
================
Comment at: lib/CodeGen/CGStmt.cpp:267
@@ +266,3 @@
+/// restrict-qualified variables declared within it.
+struct RestrictFinder : RecursiveASTVisitor<RestrictFinder> {
+ bool FoundRestrictDecl;
----------------
rjmccall wrote:
> RecursiveASTVisitors are actually really big; the last time I checked, a
> RecursiveASTVisitor compiled to several hundred kilobytes of code, no matter
> how few traversals you actually have in it.
>
> Also, doing a recursive walk every time you enter a scope is inherently
> quadratic with deeply-nested scopes.
>
> You don't really need a fully recursive walk, though. The language
> specification for 'restrict' talks about the scope B that declares the
> restrict-qualified pointer; you just need to scan the current compound
> statement for variable declarations.
> RecursiveASTVisitors are actually really big; the last time I checked, a
> RecursiveASTVisitor compiled to several hundred kilobytes of code, no matter
> how few traversals you actually have in it.
Hrmm.
> Also, doing a recursive walk every time you enter a scope is inherently
> quadratic with deeply-nested scopes.
Good point.
> You don't really need a fully recursive walk, though. The language
> specification for 'restrict' talks about the scope B that declares the
> restrict-qualified pointer; you just need to scan the current compound
> statement for variable declarations.
You're right, I'll write a more-targeted function.
http://reviews.llvm.org/D9403
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
