[PATCH] D66029: llvm-canon

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added a comment.

I've got a few nits. Will do a second pass shortly.



================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:175
+  // Get output footprint for I.
+  SetVector<int> outputFootprint = getOutputFootprint(I, Visited);
+
----------------
get this to conform to llvm style (ie OutputFootprint)


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:178
+  // Consider output footprint in the hash.
+  for (const int &output : outputFootprint)
+    Hash = hashing::detail::hash_16_bytes(Hash, output);
----------------
Output as well. 


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:234
+  for (auto &OP : I->operands()) {
+    if (auto IOP = dyn_cast<Instruction>(OP)) {
+      // Walk down the use-def chain.
----------------
nit: auto *IOP


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:326
+    if (const Instruction *IOP = dyn_cast<Instruction>(OP)) {
+      bool hasCanonicalName = I->getName().substr(0, 2) == "op" ||
+                              I->getName().substr(0, 2) == "vl";
----------------
nit: HasCanonicalName


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:370
+    for (auto &OP : I->operands())
+      if (auto IOP = dyn_cast<Instruction>(OP))
+        reorderInstruction(IOP, I, Visited);
----------------
not: auto *IOP


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:421
+  for (auto &OP : I->operands()) {
+    if (auto VOP = dyn_cast<Value>(OP)) {
+      if (isa<Instruction>(VOP)) {
----------------
not: auto *VOP


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:439
+  // Reorder operands.
+  unsigned position = 0;
+  for (auto &OP : I->operands()) {
----------------
nit: Position


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:461
+  // Sort values according to the name of a basic block.
+  llvm::sort(Values, [](const std::pair<Value *, BasicBlock *> &lhs,
+                        const std::pair<Value *, BasicBlock *> &rhs) {
----------------
nit: LHS and RHS


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:517
+bool IRCanonicalizer::hasOnlyImmediateOperands(const Instruction *I) {
+  for (auto &OP : I->operands()) {
+    if (isa<Instruction>(OP)) {
----------------
```
  for (const auto &OP : I->operands())
    if (isa<Instruction>(OP))
      return false; // Found non-immediate operand (instruction).
```


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:548
+      // Finds and inserts the index of the output to the vector.
+      unsigned count = 0;
+      for (auto &B : *Func) {
----------------
```
      unsigned Count = 0;
      for (const auto &B : *Func) {
        for (const auto &E : B) {
          if (&E == I)
            Outputs.insert(Count);
          Count++;
        }
      }
```


================
Comment at: llvm/lib/Transforms/Utils/IRCanonicalizer.cpp:562
+
+    for (auto U : I->users()) {
+      if (auto UI = dyn_cast<Instruction>(U)) {
----------------
nit: auto *U and auto *UI


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

https://reviews.llvm.org/D66029