[PATCH] D9403: llvm.noalias - Clang CodeGen for local restrict-qualified pointers

John McCall via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 16 15:11:24 PDT 2016


rjmccall added inline comments.

================
Comment at: lib/CodeGen/CGStmt.cpp:525
@@ +524,3 @@
+void CodeGenFunction::LexicalNoAliasInfo::addNoAliasMD() {
+  if (MemoryInsts.empty() || NoAliasScopes.empty())
+    return;
----------------
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.

================
Comment at: lib/CodeGen/CGStmt.cpp:537
@@ +536,3 @@
+                                      llvm::LLVMContext::MD_noalias),
+                                      NewScopeList));
+
----------------
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.

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1900
@@ +1899,3 @@
+
+  if (I->mayReadOrWriteMemory())
+    recordMemoryInstruction(I);
----------------
Is it intentional that this includes calls and invokes?  If so, please leave a comment describing which instructions we want to apply this to and why.

In general, this entire patch is really light on comments.

================
Comment at: lib/CodeGen/CodeGenFunction.h:541
@@ +540,3 @@
+
+    LexicalNoAliasInfo(bool RMI = false) : RecordMemoryInsts(RMI) {}
+
----------------
Why this is defaultable?


https://reviews.llvm.org/D9403





More information about the cfe-commits mailing list