[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:
> 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