[PATCH] D66029: llvm-canon

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some initial comments.
In general:

- Don't spell out type if you just used `*cast<???>`
- Don't drop `*`/`&` after `auto`
- Do end files with newline
- Consider small-size optimization. Please try to see if some of these `std::string` can be replaced with reasonably-sized `llvm::SmallString<?>`
- Please consider preallocating some strings
- This needs a bit more refactoring i think



================
Comment at: docs/Passes.rst:693
+``-ir-canonicalizer``: Transforms IR into canonical form
+--------------------------------------------------------------------
+
----------------
Too many `-`


================
Comment at: include/llvm/Transforms/Utils/IRCanonicalizer.h:96
+#endif // LLVM_TRANSFORMS_UTILS_IRCANONICALIZER_H
\ No newline at end of file

----------------
Please make sure that files end with newlines


================
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(
----------------
Should these have defaults?


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:57-78
+    nameFunctionArguments(F);
+    nameBasicBlocks(F);
+
+    SmallVector<Instruction *, 16> Outputs = collectOutputInstructions(F);
+
+    if (!PreserveOrder)
+      reorderInstructions(Outputs);
----------------
runOnFunction() ?


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:73
+
+        if (PHINode *PN = dyn_cast<PHINode>(&I))
+          reorderPHIIncomingValues(PN);
----------------
`if(auto *PN = dyn_cast<PHINode>(&I))`


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:150-170
+  // Instruction operands for further sorting.
+  SmallVector<std::string, 4> Operands;
+
+  // Collect operands.
+  for (auto &OP : I->operands()) {
+    if (!isa<Function>(OP)) {
+      std::string TextRepresentation;
----------------
This block will result in most of memory nagging in this pass.



================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:151
+  // Instruction operands for further sorting.
+  SmallVector<std::string, 4> Operands;
+
----------------
Can you make any reasonable guess as to what would be 90'th percentile of Operand string length?
Maybe try using `SmallString<64>`.


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:166
+
+  std::string OperandList =
+      std::accumulate(Operands.begin(), Operands.end(), std::string(),
----------------
It would be really good to predict+preallocate the size here.


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:183
+  // Consider output footprint in the hash.
+  for (auto output : outputFootprint)
+    Hash = hashing::detail::hash_16_bytes(Hash, output);
----------------
`const int& output = ` ?
The type is not clear to me here.


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:190
+  // In case of CallInst, consider callee in the instruction name.
+  if (auto CI = dyn_cast<CallInst>(I)) {
+    Function *F = CI->getCalledFunction();
----------------
`auto* CI`


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:219
+/// \param I Instruction to be renamed.
+void IRCanonicalizer::nameAsRegularInstruction(Instruction *I) {
+  // Instruction operands for further sorting.
----------------
Same comments as for the previous function


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:263
+  for (auto &OP : I->operands())
+    if (auto IOP = dyn_cast<Instruction>(OP))
+      OperandsOpcodes.push_back(IOP->getOpcode());
----------------
`auto* IOP`


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:270
+  // Consider operand opcodes in the hash.
+  for (auto Code : OperandsOpcodes)
+    Hash = hashing::detail::hash_16_bytes(Hash, Code);
----------------
`const int Code = `?


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:277
+  // In case of CallInst, consider callee in the instruction name.
+  if (const CallInst *CI = dyn_cast<CallInst>(I))
+    if (const Function *F = CI->getCalledFunction())
----------------
`const auto *CI`


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:305
+    // Don't fold if one of the users is an output instruction.
+    for (auto U : I->users())
+      if (auto IU = dyn_cast<Instruction>(U))
----------------
`auto*`


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:306
+    for (auto U : I->users())
+      if (auto IU = dyn_cast<Instruction>(U))
+        if (isOutput(IU))
----------------
`auto*`


================
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;
+                      });
----------------
I'm sensing a repetitive pattern. I think you want to refactor it.


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:388
+    for (auto &OP : Used->operands()) {
+      if (auto IOP = dyn_cast<Instruction>(OP)) {
+        // Walk up the def-use tree.
----------------
`auto*`


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:427-430
+             [](const std::pair<std::string, Value *> &lhs,
+                const std::pair<std::string, Value *> &rhs) {
+               return lhs.first < rhs.first;
+             });
----------------
`llvm::less_first`


================
Comment at: lib/Transforms/Utils/IRCanonicalizer.cpp:504
+  // other instructions, yet they only depend on immediate values.
+  return std::distance(I->user_begin(), I->user_end()) != 0 &&
+         hasOnlyImmediateOperands(I);
----------------
`!I->user_empty()` ?


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

https://reviews.llvm.org/D66029