rjmccall added inline comments.

Comment at: lib/CodeGen/CGDecl.cpp:1291
+// where the pointer to the local variable is the key in the map.
+void CodeGenFunction::EmitAutoVarNoAlias(const AutoVarEmission &emission) {
+  assert(emission.Variable && "emission was not valid!");
This should really be a subroutine of EmitAutoVarAlloca; that will implicitly 
pick this logic up for a bunch of less standard places in IRGen that create 
local variables.  Please make it a static function (if possible) called 
EmitNoAliasScope and call it immediately after EmitVarAnnotations.

Comment at: lib/CodeGen/CGStmt.cpp:525
+void CodeGenFunction::LexicalNoAliasInfo::addNoAliasMD() {
+  if (MemoryInsts.empty() || NoAliasScopes.empty())
+    return;
hfinkel wrote:
> rjmccall wrote:
> > It's much more likely that NoAliasScopes will be empty than that 
> > MemoryInsts will be empty.  You should probably fast-path using that, or 
> > better yet, with the RecordMemoryInsts bit.
> I'm not sure that's true; we only record memory-accessing instructions in the 
> first place if there are relevant restrict-qualified pointers around.
Ok, that makes sense.

Comment at: lib/CodeGen/CGStmt.cpp:537
+                                      llvm::LLVMContext::MD_noalias),
+                                      NewScopeList));
rjmccall wrote:
> hfinkel wrote:
> > rjmccall wrote:
> > > This is a very strange representation.  Every memory operation in the 
> > > lexical block is annotated with a list of all of the scopes that were 
> > > entered within the block, even if they were entered after the operation.  
> > > But for some reason, not with nested scopes?
> > > 
> > > What's the right patch for me to read about this representation?
> > Perhaps unfortunately, this is an artifact of the way that restrict is 
> > defined in C. It applies to all accesses in the block in which the variable 
> > is declared, even those before the declaration of the restrict-qualified 
> > local itself.
> > 
> > It should work with nested scopes, in the sense that we add these things as 
> > we complete each scope. So we add things to the inner scope, and then when 
> > we complete the outer scope, we go back over the instructions (including 
> > those in the inner scope because the scope recording recurses up the scope 
> > hierarchy), and adds the outer scopes - it concatenates them to any added 
> > by the inner (nested) scopes.
> > 
> > The noalias intrinsic's LangRef updates are in D9375.
> Ah, I see the recursion now.  Please add a comment explaining that 
> expectation here.
I would still like this comment.  It should be enough to explain how 
MemoryInsts actually gets filled: there's a callback from inserting an 
instruction which adds all memory instructions to every active lexical scope 
that's recording them.

Comment at: lib/CodeGen/CodeGenFunction.cpp:1986
+  if (I->mayReadOrWriteMemory())
+    recordMemoryInstruction(I);
This condition will include calls, which I assume is unnecessary (except maybe 
memory intrinsics?).  More seriously, it will also include loads and stores 
into allocas, which will sweep in all sorts of bookkeeping temporaries that 
IRGen uses in more complex code-generation situations.  You might want to 
consider ways to avoid recording this kind of instruction that are very likely 
to just get immediately optimized away by e.g. mem2reg.

Please also modify LexicalScope so that it records whether there's any active 
recording scope in the CodeGenFunction.  Lexical scopes are nested, so that 
should be as easy as saving the current state when you enter a scope and 
restoring it when you leave.  That will allow you to optimize this code by 
completely bypassing the recursion and even the check for whether the 
instruction is a memory instruction in the extremely common case of a function 
with no restrict variables.

In general, a lot of things about this approach seem to have worryingly poor 
asymptotic performance in the face of large functions or (especially) many 
restrict variables.  (You could, for example, have chosen to have each memory 
instruction link to its enclosing noalias scope, which would contain a link to 
its enclosing scope and a list of the variables it directly declares.  This 
would be a more complex representation to consume, but it would not require 
inherently quadratic frontend work building all these lists.)  The only saving 
grace is that we expect very little code to using restrict, so it becomes vital 
to ensure that your code is immediately short-circuited when nothing is using 

Comment at: lib/CodeGen/CodeGenFunction.h:597
+    // those blocks) so that we can later add the appropriate metadata. Record
+    // this instruction and so the same in any parent scopes.
+    void recordMemoryInstruction(llvm::Instruction *I) {
I would suggest this wording:

  /// Record the given memory access instruction in this and all of its 
enclosing scopes.
  /// When we close this scope, we'll add the list of all the 
restrict-qualified local variables
  /// it declared to all the memory accesses which occurred within it.  
Recording is only
  /// enabled under optimization, and it's disabled in a scope unless it 
actually declares
  /// any local restrict variables.


cfe-commits mailing list

Reply via email to