[PATCH] [Polly] Allow to annotate alias scopes in the new SCoP.

Tobias Grosser tobias at grosser.es
Wed Oct 1 23:50:23 PDT 2014


Nice!

Some minor comments inlined.

================
Comment at: include/polly/CodeGen/CodeGeneration.h:41
@@ -39,1 +40,3 @@
+extern bool PollyAnnotateAliasScopes;
+
 static inline int getNumberOfIterations(__isl_take isl_set *Domain) {
----------------
I don't think we need a command line option for this. That seems pure goodness, so why don't we enable it by default?

================
Comment at: include/polly/CodeGen/IRBuilder.h:32
@@ +31,3 @@
+///   1) Loops are stored in a stack-like structure will keep track of all
+///      of them. Contained memory instructions and loop headers are annotated
+///      according to all parallel surrounding loops.
----------------
The first sentence talks more about the way the LoopAnnotator works and also seems somehow incomplete.

================
Comment at: lib/CodeGen/IRBuilder.cpp:34
@@ +33,3 @@
+static MDNode *getID(LLVMContext &Ctx, Value *arg0 = nullptr,
+                     Value *arg1 = nullptr) {
+  MDNode *ID;
----------------
What are arg0 and arg0? Are you saying this metadata node can (optionally) reference two other metadata nodes? Maybe add this to the example in the documentation.

================
Comment at: lib/CodeGen/IRBuilder.cpp:47
@@ +46,3 @@
+    ID = MDNode::get(Ctx, Args);
+  }
+  ID->replaceOperandWith(0, ID);
----------------
What about writing this like:
```
std::vector<Value *> Args = {0}

if (arg0)
  Args.push_back(arg0);

if (arg1)
  Args.push_back(arg1);

ID = MDNode::get(Ctx, Args);
```

  

================
Comment at: lib/CodeGen/IRBuilder.cpp:63
@@ +62,3 @@
+
+  SmallPtrSet<Value *, 8> BasePtrs;
+  for (ScopStmt *Stmt : S)
----------------
Please use a data structure which guarantees the order in which we iterate over (SetVector?)

================
Comment at: lib/CodeGen/IRBuilder.cpp:135
@@ +134,3 @@
+        BasePtr = SU->getValue();
+    }
+
----------------
Would it be better to use the new to (later) use the newly introduced ScopArrayInfo class to get the base pointer?

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:610
@@ +609,3 @@
+    if (PollyAnnotateAliasScopes)
+      Annotator.buildAliasScopes(S);
+
----------------
I would enable this by default.

http://reviews.llvm.org/D5563






More information about the llvm-commits mailing list