[PATCH] D45743: [Polly] Print executed statement instances at runtime.

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

thanks for the patch. Looks great!

I have one interface comment, which you might to have a look at.

Otherwise, just minor comments / documentation things. While the original code is not as extensively documented, if it is low overhead you might think of completing some more.



================
Comment at: include/polly/CodeGen/BlockGenerators.h:339
+  ///
+  /// If printing of scalar is enable, it also appends the value of each scalar
+  /// to the line:
----------------
scalarS


================
Comment at: include/polly/CodeGen/RuntimeDebugBuilder.h:35
+  static llvm::Value *getPrintableString(PollyIRBuilder &Builder,
+                                         llvm::StringRef Str) {
+    return Builder.CreateGlobalStringPtr(Str, "", 4);
----------------
Should we document the arguments?


================
Comment at: include/polly/CodeGen/RuntimeDebugBuilder.h:39
+
+  static bool isPrintable(llvm::Type *Ty);
+
----------------
Maybe add a comment?


================
Comment at: include/polly/CodeGen/RuntimeDebugBuilder.h:76
+  static void createPrinter(PollyIRBuilder &Builder, bool UseGPU,
+                            llvm::ArrayRef<llvm::Value *> Values);
+
----------------
Should we document the arguments?

IMPORTANT: I don't think we should expose this function. This is the internal interface, see below.


================
Comment at: include/polly/CodeGen/RuntimeDebugBuilder.h:93
                             llvm::StringRef String, Args... args) {
-    Values.push_back(Builder.CreateGlobalStringPtr(String, "", 4));
+    Values.push_back(getPrintableString(Builder, String));
     createPrinter(Builder, UseGPU, Values, args...);
----------------
Nice!


================
Comment at: include/polly/CodeGen/RuntimeDebugBuilder.h:110
       createPrinter(Builder, UseGPU, Values, args...);
   }
 
----------------
IMPORTANT: I feel this function needs fixing instead of exposing the function pointed out above. It seems this function does not seem to work and apparently is not even tested, which is why you maybe exposed the other function. 

An easy fix could be to change this function to.

```
Values.insert(Values.begin(), Array.begin(), Array.end());
createPrinter(Builder, UseGPU, Values, args...);
```

This changes the semantics of this function, in that the original idea was to add whitespaces between elements passed in a vector and the new variant would not do this any more.  We could probably add/fix white space insertions if this is needed/useful. I don't have an opinion here, but we should likely document this slightly more explicit.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:61
+    "polly-codegen-trace-scalars",
+    cl::desc("Add printf calls that prints the values of all scalar values "
+             "used in a statement. Requires -polly-codegen-trace-stmts."),
----------------
that _print_


================
Comment at: lib/CodeGen/BlockGenerators.cpp:682
+  isl::map Schedule = isl::map::from_union_map(USchedule);
+  assert(Schedule.is_empty().is_false());
+
----------------
Add a notice to the assert?


================
Comment at: lib/CodeGen/BlockGenerators.cpp:692
+  // Print the name of the statement.
+  // TODO: Indent by the depth of the statement instance in the schedule tree.
+  Values.push_back(RuntimeDebugBuilder::getPrintableString(Builder, BaseName));
----------------
I assume you intended to leave this in. That's fine with me, just pointing it out.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:727
+        for (Value *Op : Inst->operand_values()) {
+          // Do not print values that do cannot change during the execution of
+          // the SCoP.
----------------
do cannot


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1494
+  generateBeginStmtTrace(Stmt, LTS, EntryBBMap);
+#endif
+
----------------
Remove #if 1?


Repository:
  rPLO Polly

https://reviews.llvm.org/D45743