[PATCH] D66029: llvm-canon

[Michal Paszkowski via Phabricator via llvm-commits]

mpaszkowski marked 16 inline comments as done.
mpaszkowski added inline comments.


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:37-45
+cl::opt<bool> IRCanonicalizer::PreserveOrder(
+    "preserve-order", cl::Hidden,
+    cl::desc("Preserves original instruction order"));
+cl::opt<bool> IRCanonicalizer::RenameAll(
+    "rename-all", cl::Hidden,
+    cl::desc("Renames all instructions (including user-named)"));
+cl::opt<bool> IRCanonicalizer::FoldPreoutputs(
----------------
lebedev.ri wrote:
> Should these have defaults?
They are all false by default, this is why I haven't explicitly stated their value. I don't think this will change in the future.


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:57-78
+    nameFunctionArguments(F);
+    nameBasicBlocks(F);
+
+    SmallVector<Instruction *, 16> Outputs = collectOutputInstructions(F);
+
+    if (!PreserveOrder)
+      reorderInstructions(Outputs);
----------------
lebedev.ri wrote:
> runOnFunction() ?
Yes! I don't know why I haven't changed this any earlier.


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:166
+
+  std::string OperandList =
+      std::accumulate(Operands.begin(), Operands.end(), std::string(),
----------------
lebedev.ri wrote:
> It would be really good to predict+preallocate the size here.
Changed to standard for-loop and moved to the end of the function.


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:331-335
+  std::string OperandList =
+      std::accumulate(Operands.begin(), Operands.end(), std::string(),
+                      [](const std::string &a, const std::string &b) {
+                        return a + (a.length() > 0 ? ", " : "") + b;
+                      });
----------------
lebedev.ri wrote:
> I'm sensing a repetitive pattern. I think you want to refactor it.
The pattern only repeats for creating the operand list.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66029/new/

https://reviews.llvm.org/D66029