[PATCH] D13338: [Polly] Consolidate invariant loads

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

the patch looks conceptually good and it has a very useful commit message. I do not have any code changes, but would suggest a couple of additional comments. Some of the information that I miss as source code comments can probably just been taken from your commit message.

Some minor comments:

- There is an incomplete sentence in part 3) of the commit message

> Cool! Looks less impactful now!


@Michael: Thanks for reviewing! One point: in this message it seems you finished your review, but it is unclear if the patch is good to go, if you would prefer Johannes to still address some of your open comments or if you prefer me to have another look. It would help if you could state this explicitly.

Best,
Tobias


================
Comment at: include/polly/ScopInfo.h:702
@@ -701,1 +701,3 @@
 
+/// @brief Type for an order list of invariant accesses.
+using InvariantAccessListTy = std::forward_list<InvariantAccessTy>;
----------------
ordered

================
Comment at: include/polly/ScopInfo.h:939
@@ -932,3 +938,3 @@
   void hoistMemoryAccesses(MemoryAccessList &InvMAs,
-                           InvariantAccessesTy &TargetList);
+                           InvariantAccessesTy &InvariantAccesses);
 
----------------
As Michael mentioned, the rename seems unrelated. If it is I would prefer to commit it separately before this commit to reduce the actual diff.

================
Comment at: lib/Analysis/ScopInfo.cpp:1442
@@ +1441,3 @@
+      IAClassDomainCtx =
+          isl_set_union(IAClassDomainCtx, isl_set_copy(DomainCtx));
+      ClassList.push_front(std::make_pair(MA, IAClassDomainCtx));
----------------
As Michael mentioned, adding the information of part 2) of your commit message as a comment here would make the code more understandable. IAs this function is getting large, it might indeed make sense to split it into two subfunctions, each with its own comment.

================
Comment at: lib/Analysis/ScopInfo.cpp:1477
@@ +1476,3 @@
+  // Try to find an equivalence class for the load, if found return
+  // the SCEV for the representing element, otherwise return S.
+  const SCEV *PointerSCEV = SE->getSCEV(LInst->getPointerOperand());
----------------
You mention the term equivalence classes here and in one header, but do not explain which equivalence classes exist. It would be helpful to explicitly state at one of these locations what are the elements that are sorted into equivalence classes, how do they differ and which properties are used to sort them into equivalence classes.

================
Comment at: lib/Analysis/ScopInfo.cpp:1506
@@ -1446,1 +1505,3 @@
 __isl_give isl_id *Scop::getIdForParam(const SCEV *Parameter) const {
+  // Normalize invariant loads first to get the correct parameter SCEV.
+  Parameter = normalizeInvariantLoadSCEV(Parameter);
----------------
Michael commented: "Why is one parameter correct but not the other?"

This does not yet seem to be addressed and is not clear to me either,

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:914-915
@@ -912,4 +913,4 @@
 
-  const auto &InvAccList = S.getInvariantAccesses();
-  if (InvAccList.empty())
+  const auto &InvariantAccesses = S.getInvariantAccesses();
+  if (InvariantAccesses.empty())
     return;
----------------
As Michael commented, this rename seems unrelated. (I like the rename, but please just commit it separately ahead of time)

@Michael: auto can derive 'const' and '*' but in some cases we still add them to make clear that something is a ptr or a const. Not sure if this information adds additional value here though.



================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:936
@@ -933,2 +935,3 @@
     Instruction *AccInst = MA->getAccessInstruction();
     Value *PreloadVal = preloadInvariantLoad(*MA, Domain, Build);
+    for (const InvariantAccessTy &IA : IAClass.second)
----------------
As Michael mentioned, this seems to be 3) from your commit message. Adding the information from your commit message in the source code would be useful.


http://reviews.llvm.org/D13338