[PATCH] D122207: [IROutliner][IRSim] Add all outlined region basic blocks to canonical numbering to add basic blocks to generated PHINode numbering generation.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:1025
+  getBasicBlocks(BBSet);
+  // Find canonical numbers for the BasicBlocks in the current candididate.
+  // This is done by finding the corresponding value for the first instruction
----------------



================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:1032
+  for (BasicBlock *BB : BBSet) {
+    unsigned ThisBBGVN = ValueToNumber.find(BB)->second;
+
----------------
I think we can have more descriptive names for the variables here?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:1034
+
+    // We can skip the BasicBlock if the caonical numbering has already been
+    // found in a separate instruction.
----------------



================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:1039
+
+    Value *V;
+    // If the basic block is the starting block, then the shared instruction may
----------------
Can we use a more descriptive name than `V`?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:1046
+    else
+      V = &BB->front();
+    unsigned ThisGVN = *getGVN(V);
----------------
Nit: We can use the ternary operator for the assignment here.

```
Value *V = BB == getStartBB() ? frontInstruction() : &BB->front();
```


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:1047
+      V = &BB->front();
+    unsigned ThisGVN = *getGVN(V);
+    unsigned ThisCanonNum = *getCanonicalNum(ThisGVN);
----------------
I think we can be more descriptive than `This<Thing>` for the names here. Is there a way to link the name back to what you described in the comment above?



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1192
+    GVN = OGVN.getValue();
+    OGVN = Cand.getCanonicalNum(GVN);
+    assert(OGVN.hasValue() && "No GVN found for incoming block?");
----------------
nit: Maybe in a follow-up, it'd be good to rename `OGVN` to something like `CanonicalNum`, just to be clearer as to what it really is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122207