[PATCH] D66029: llvm-canon

[Felipe de Azevedo Piovezan via Phabricator via llvm-commits]

fdeazeve added inline comments.


================
Comment at: tools/llvm-canon/canonicalizer.cpp:53
+        for (auto &B : F) {
+            for (auto &I : B) {
+                if (I.isCommutative()) {
----------------
you can `include "llvm/IR/InstIterator.h"`

and use `for (auto &I : instructions(F))`


================
Comment at: tools/llvm-canon/canonicalizer.cpp:74
+        if (renameAll || A.getName().empty()) {
+            // Argument has no name or renameAll flag is used.
+            A.setName("a" + Twine(ArgumentCounter));
----------------
Is this comment really necessary? It feels like it is repeating the `if` statement in the opposite order.


================
Comment at: tools/llvm-canon/canonicalizer.cpp:88
+        // Initialize to a random constant, so the state isn't zero.
+        uint64_t Hash = 0x6acaa36bef8325c5ULL;
+
----------------
plotfi wrote:
> What is this magic number? Could this be generated with something like srand at the begining of runOnFunction? 
I might be missing something, but don't we want this algorithm to produce the same result if it's run on the "same" (two identical functions, modulo renaming, etc) function twice in a row?


================
Comment at: tools/llvm-canon/canonicalizer.cpp:203
+
+    if ((I->getName().empty() || renameAll) && !I->getType()->isVoidTy()) {
+        // Instruction is unnamed or renameAll flag is raised.
----------------
Make this an early return at the very start of the function? This entire method has no effects when this if statement is false, correct?


================
Comment at: tools/llvm-canon/canonicalizer.cpp:235
+    // Collect operands.
+    for (auto &OP : I->operands()) {
+        if (auto IOP = dyn_cast<Instruction>(OP)) {
----------------
Isn't this a reference to a pointer? I think you meant `auto *OP`. The reason I'm saying this is because you use `dyn_cast` below, and yet `dyn_cast` doesn't work with references.


================
Comment at: tools/llvm-canon/canonicalizer.cpp:237
+        if (auto IOP = dyn_cast<Instruction>(OP)) {
+            // This is an an instruction.
+
----------------
Please remove unnecessary comment


================
Comment at: tools/llvm-canon/canonicalizer.cpp:298
+    // In case of CallInst, consider callee in the instruction name.
+    if (auto CI = dyn_cast<CallInst>(I)) {
+        Function *F = CI->getCalledFunction();
----------------
Guidelines suggest using `auto *` when copying pointers. https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto


================
Comment at: tools/llvm-canon/canonicalizer.cpp:329
+
+    // If this flas is raised, fold all regular
+    // instructions (including pre-outputs).
----------------
typo: flag


================
Comment at: tools/llvm-canon/canonicalizer.h:94
+    bool isInitialInstruction(const Instruction *I);
+    bool hasOnlyImmediateOperands(const Instruction *I);
+    SetVector<int> getOutputFootprint(Instruction *I, 
----------------
High level comment about the header:

A lot of functions take function pointers instead of references, and yet they are never check for nullptr. This makes me believe you really wanted them to be references. In fact, if you check how those functions are called, the Instructions were originally references, and before calling the helper functions you are always doing `&I`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66029