[PATCH] D66029: llvm-canon

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added inline comments.


================
Comment at: tools/llvm-canon/canonicalizer.cpp:88
+        // Initialize to a random constant, so the state isn't zero.
+        uint64_t Hash = 0x6acaa36bef8325c5ULL;
+
----------------
What is this magic number? Could this be generated with something like srand at the begining of runOnFunction? 


================
Comment at: tools/llvm-canon/canonicalizer.cpp:111
+/// ReturnInst).
+void Canonicalizer::nameInstructions(SmallVector<Instruction *, 16> &Outputs) {
+
----------------
This function is a little short and only used in one place, could it be dropped and just have

```
    for (auto &I : Outputs)
        nameInstruction(I);
```

put in its place at the call site?


================
Comment at: tools/llvm-canon/canonicalizer.cpp:129
+        nameAsInitialInstruction(I);
+    }
+    else {
----------------
clang-format should clean up the formatting here.


================
Comment at: tools/llvm-canon/canonicalizer.cpp:154
+    for (auto &OP : I->operands()) {
+        if (isa<Value>(OP) && !isa<Function>(OP)) {
+            std::string TextRepresentation;
----------------
Remove isa<Value>(OP). Seems redundant. As far as I know, every LLVM object is a Value. 


================
Comment at: tools/llvm-canon/canonicalizer.cpp:169
+    (Operands.begin(), Operands.end(), std::string(),
+        [](const std::string& a, const std::string& b) -> std::string {
+        return a + (a.length() > 0 ? ", " : "") + b;
----------------
don't think you need -> std::string here. 


================
Comment at: tools/llvm-canon/canonicalizer.cpp:175
+    // Initialize to a random constant, so the state isn't zero.
+    uint64_t Hash = 0x6acaa36bef8325c5ULL;
+
----------------
Again, consider dropping magic numbers. Come up with something else, like setting based on srand()


================
Comment at: tools/llvm-canon/canonicalizer.cpp:185
+    // Consider output footprint in the hash.
+    for (auto output : outputFootprint) {
+        Hash = hashing::detail::hash_16_bytes(Hash, output);
----------------
drop braces


================
Comment at: tools/llvm-canon/canonicalizer.cpp:267
+    // Initialize to a random constant, so the state isn't zero.
+    uint64_t Hash = 0x6acaa36bef8325c5ULL;
+
----------------
Drop magic number.


================
Comment at: tools/llvm-canon/canonicalizer.cpp:277
+    // Collect operand opcodes for hashing.
+    for (auto &OP : I->operands()) {
+        if (auto IOP = dyn_cast<Instruction>(OP)) {
----------------
Drop the braces here.


================
Comment at: tools/llvm-canon/canonicalizer.cpp:297
+
+    // In case of CallInst, consider callee in the instruction name.
+    if (auto CI = dyn_cast<CallInst>(I)) {
----------------
Drop braces:

```
    // In case of CallInst, consider callee in the instruction name.
    if (const CallInst *CI = dyn_cast<CallInst>(I))
        if (const Function *F = CI->getCalledFunction())
            Name.append(F->getName());

```


================
Comment at: tools/llvm-canon/canonicalizer.cpp:307
+    Name.append("(" + OperandList + ")");
+
+    if ((I->getName().empty() || renameAll) && !I->getType()->isVoidTy()) {
----------------
Drop braces.


================
Comment at: tools/llvm-canon/canonicalizer.cpp:334
+        // Don't fold if one of the users is an output instruction.
+        for (auto U : I->users()) {
+            if (auto IU = dyn_cast<Instruction>(U)) {
----------------
Drop braces


================
Comment at: tools/llvm-canon/canonicalizer.cpp:351
+
+    for (auto &OP : I->operands()) {
+        if (auto IOP = dyn_cast<Instruction>(OP)) {
----------------
Drop braces and use ternary:

```
    for (auto &OP : I->operands())
        if (const Instruction *IOP = dyn_cast<Instruction>(OP))
                Operands.push_back(
                      (I->getName().substr(0, 2) == "op" || I->getName().substr(0, 2) == "vl") ?
                      // Regular/initial instruction with canonicalized name.
                      IOP->getName().substr(0, 7)) :
                      // User-named instruction, don't substring.
                      Operands.push_back(IOP->getName());
```


================
Comment at: tools/llvm-canon/canonicalizer.cpp:395
+    // Walk up the tree.
+    for (auto &I : Outputs) {
+        for (auto &OP : I->operands()) {
----------------
Drop braces


================
Comment at: tools/llvm-canon/canonicalizer.h:1
+//===-- canonicalizer.h - Canonicalizer class -------------------*- C++ -*-===//
+//
----------------
Move to somewhere in llvm/lib/Transforms. 

Also, run clang-format on this entire file.


================
Comment at: tools/llvm-canon/canonicalizer.h:31
+/// Canonicalizer aims to transform LLVM IR into canonical form.
+class Canonicalizer : public ModulePass {
+public:
----------------
Rename to IRCanonicalizerPass


================
Comment at: tools/llvm-canon/canonicalizer.h:42
+    /// \see foldPreoutputs
+    Canonicalizer()
+        : ModulePass(ID),
----------------
Do you really need both constructors? Why not

```
    Canonicalizer() = delete;
    Canonicalizer(bool renameAll = false, bool foldPreoutputs = false)
        : ModulePass(ID),
        renameAll(renameAll),
        foldPreoutputs(foldPreoutputs) {}

```


================
Comment at: tools/llvm-canon/canonicalizer.h:53
+    /// \param fold When true, doesn't fold pre-output instructions.
+    Canonicalizer(bool rename, bool fold)
+        : ModulePass(ID),
----------------
change rename and fold to RenameAll and FoldPreoutputs. They can be named the same for initializers.


================
Comment at: tools/llvm-canon/canonicalizer.h:65
+    /// Renames all instructions including named ones.
+    bool renameAll;
+    /// Folds all regular instructions (including pre-outputs).
----------------
Rename to start with upper case. I believe that is still currently the coding standard.

https://llvm.org/docs/CodingStandards.html#the-low-level-issues


================
Comment at: tools/llvm-canon/canonicalizer.h:65
+    /// Renames all instructions including named ones.
+    bool renameAll;
+    /// Folds all regular instructions (including pre-outputs).
----------------
plotfi wrote:
> Rename to start with upper case. I believe that is still currently the coding standard.
> 
> https://llvm.org/docs/CodingStandards.html#the-low-level-issues
Change to RenameAll


================
Comment at: tools/llvm-canon/canonicalizer.h:67
+    /// Folds all regular instructions (including pre-outputs).
+    bool foldPreoutputs;
+    /// @}
----------------
change to FoldPreoutputs


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66029