[PATCH] D106989: [IRSim] Finding Branch Similarity

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:151
 
+  /// A function that for an IRInstructionData containing a branch, finds the
+  /// relative distances from the source basic block to the target by taking
----------------
remove fluff


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:305
 
+  /// A mapping for a basic block in a module to it's assigned number/location
+  /// in the module.
----------------



================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:130
 
+  SmallVector<int, 4> RelativeBlockLocations;
+
----------------
Why int?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:359
+  /// Assigns values to all the basic blocks in Module \p M.
+  /// \param M - The modules containing the basic blocks to assign numbers to.
+  void initializeForBBs(Module &M) {
----------------



================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:461
+
+    bool EnableBranches = false;
   };
----------------
comment?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:461
+
+    bool EnableBranches = false;
   };
----------------
paquette wrote:
> comment?
comment on this variable?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:860
 
+  bool EnableBranches = true;
+
----------------
doxygen comment on this variable?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:26
 
+cl::opt<bool> DoNotEnableBranches(
+    "no-ir-sim-branch-matching", cl::init(false), cl::ReallyHidden,
----------------
Mention this is for debugging?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:175
+  if (isa<BranchInst>(A.Inst) && isa<BranchInst>(B.Inst)) {
+    if (A.RelativeBlockLocations.size() != B.RelativeBlockLocations.size())
+      return false;
----------------
pull `if` into the `if` above?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:687
+    // analagous location in the region we are comparing to.
+    if ((isa<BranchInst>(ItA->Inst) && isa<BranchInst>(ItB->Inst)) ||
+        (isa<PHINode>(ItA->Inst) && isa<PHINode>(ItB->Inst))) {
----------------
Cache `ItA->Inst` and `ItB->Inst` in variables?

also can you early return true or false using this condition? This `if` is pretty big.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:702
+                  OperValsA, OperValsB);
+      if (!all_of(ZippedRelativeLocations,
+                  [&A, &B](std::tuple<int, int, Value *, Value *> R) {
----------------
this is a pretty dense statement. can you simplify this somehow? maybe it would be better as a helper function or something?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:712
+                    B.getBasicBlocks(BasicBlockB);
+                    if (BasicBlockA.find(ABB) != BasicBlockA.end())
+                      AContained = true;
----------------
this could assign to AContained right?

```
bool AContained = BasicBloc A.find(...) != ...;
```


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:805
+  
+  if (StringLen == 2) {
+    const unsigned StartIdx = *RS.StartIndices.begin();
----------------
why is there a special case for 2? comment?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:174
 
+  if (isa<BranchInst>(A.Inst) && isa<BranchInst>(B.Inst)) {
+    if (A.RelativeBlockLocations.size() != B.RelativeBlockLocations.size())
----------------
pull second `if` into this one?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:688
+    // analagous location in the region we are comparing to.
+    if ((isa<BranchInst>(ItA->Inst) && isa<BranchInst>(ItB->Inst)) ||
+        (isa<PHINode>(ItA->Inst) && isa<PHINode>(ItB->Inst))) {
----------------
de morgan this and early return true to reduce indentation?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1392
+
+    if (!IDIt.getNodePtr()->isKnownSentinel())
+      if (IDIt->Inst != NextInst)
----------------
I think that you should check for the end of the list in a different way. This seems like a workaround for a different bug? Nowhere else in the codebase uses `isKnownSentinel` directly.


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

https://reviews.llvm.org/D106989