[PATCH] D32667: [Polly] Introduce VirtualUse. NFC.

[Tobias Grosser via Phabricator via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

Hi Michael,

thank you for the patch. It looks very good. I only added a couple of minor comments. Feel free to commit if you agree with them.



================
Comment at: include/polly/Support/VirtualInstruction.h:25
+/// These are not always representable by llvm::Use. For instance, scalar write
+/// MemoryAccesses do use a value, but are not associated to an instruction's
+/// argument.
----------------
associated WITH


================
Comment at: include/polly/Support/VirtualInstruction.h:40
+
+    // An  llvm::BasicBlock.
+    Block,
----------------
double white space 


================
Comment at: include/polly/Support/VirtualInstruction.h:81
+  /// MemoryAccess that makes the value available in this statement. In case of
+  /// intra-statement uses, can contain an MemoryKind::Array access. In all
+  /// other cases, it is a nullptr.
----------------
"an" -> "a" MemoryAccess.


================
Comment at: include/polly/Support/VirtualInstruction.h:101
+
+  /// Get a VirtualUse for any kind use of a value within a statement.
+  ///
----------------
"any kind use": this sounds wrong. Do you just mean "for any use" or for "any kind of use within a statement"?


================
Comment at: include/polly/Support/VirtualInstruction.h:110
+  /// @param UserScope Loop scope in which the value is used. Needed to
+  ///                  determine whether the value us synthesizable.
+  /// @param Val       The value being used.
----------------
us -> is


================
Comment at: include/polly/Support/VirtualInstruction.h:113
+  /// @param Virtual   Whether to use (and prioritize over instruction location)
+  /// information about MemoryAccesses.
+  ///
----------------
Could you indent this?


================
Comment at: lib/Analysis/ScopBuilder.cpp:576
+    if (UserStmt->lookupValueReadOf(V))
+      break;
 
----------------
Can you transfer the comment from the old code:

"Do not create another MemoryAccess for reloading the value if one already exists."


================
Comment at: lib/Analysis/ScopBuilder.cpp:644
+
+static void verifyUses(Scop *S, LoopInfo &LI, DominatorTree &DT) {
+  for (auto *BB : S->getRegion().blocks()) {
----------------
What is the invariant you verify here? Can you add a comment explaining it?


================
Comment at: lib/CodeGen/BlockGenerators.cpp:136
+    // Constants should not be redefined. In this case, the GlobalMap just
+    // contains a mapping to the same constants, which us unnecessary, but
+    // harmless.
----------------
IS unnecessary


================
Comment at: lib/CodeGen/BlockGenerators.cpp:185
+    // not precomputed. These tests fail without this, but I think
+    // trySynthesizeNewValue would just re-synthesize the same instructions.
+    if ((New = BBMap.lookup(Old)))
----------------
Are they really pre-compute is it more that we already synthesized them once and consequently don't need to re-synthesize them again.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:201
+           !BBMap.count(Old) &&
+               "Globally hoisted values should not be redefined locally");
+
----------------
My guess is that we code-generate load hoisted values like all other values by copying the instruction and adding the new return value to BBMap. We likely can skip code generation for load-hoisted values. This should solve the problem. It probably does not make sense to clean this up as part of this change. So just leaving this comment and assert and fixing this later seems to be the right strategy. 


================
Comment at: lib/CodeGen/BlockGenerators.cpp:209
+    assert(!GlobalMap.count(Old) &&
+           "intra and inter-stmt values are never global");
+    New = BBMap.lookup(Old);
----------------
Inter -> start with uppercase letter.


================
Comment at: lib/Support/VirtualInstruction.cpp:79
+
+  // Uses are read-only if they have been defined before the SCoP, i.e. they
+  // cannot be changed with the SCoP. Arguments are defined before any
----------------
i.e., : Comma after "i.e."


https://reviews.llvm.org/D32667