[PATCH] D106989: [IRSim] Finding Branch Similarity

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Missing context for the patch?



================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:131
 
+  /// This structure holds the distances of how far "ahead of" or "behind" the
+  /// target blocks of a branch, or the incoming blocks of a phi nodes are.
----------------
Can you add an example of "ahead of" and "behind" in the comments here?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:145
+
+  /// The shared code needed to fill in the data stuctures for IRInstrucitonData
+  /// when it is constructed from a reference or a pointer.
----------------
typo: IRInstrucitonData


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:613
 
+  struct RelativeLocMapping {
+    /// The IRSimilarityCandidate that holds the instruction the relative
----------------
Can you add a comment explaining the role of this struct?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:621
+
+    // The corresponding value.
+    Value *OperVal;
----------------
Should this be a doxygen comment?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:651
+  /// the branches both point outside the region if they do not.
+  ///
+  /// \param A - The first IRInstructionCandidate, relative location value,
----------------
I think an example would be useful here?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:657
+  /// \returns true if the relative locations match.
+  static bool checkRelativeLocations(RelativeLocMapping A,
+                                     RelativeLocMapping B);
----------------
I think it would be good to document what a relative location is somewhere in a comment in this patch.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:187
 
+  if (isa<BranchInst>(A.Inst) && isa<BranchInst>(B.Inst) &&
+      A.RelativeBlockLocations.size() != B.RelativeBlockLocations.size())
----------------
De Morgan + return this value?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:631
+    return false;
+  if (AContained)
+    return A.RelativeLocation == B.RelativeLocation;
----------------
What does it mean if `!AContained`?

Maybe this function needs some comments?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:713
+
+    // Here we check that between two corresponding branch instructions,
+    // when the branch targets a basic block within the same region, the
----------------
run-on sentence is hard to read?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:732
+
+    if (OperValsA.size() != OperValsB.size())
+      return false;
----------------
Merge this `if` in with the previous one?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:741
+                OperValsA, OperValsB);
+    if (!all_of(ZippedRelativeLocations,
+                [&A, &B](std::tuple<int, int, Value *, Value *> R) {
----------------
use `any_of` and return `!checkRelativeLocations`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1457
+      if (IDIt->Inst != nullptr)
+        if (IDIt->Inst != NextInst)
+          return true;
----------------
- Can you pull `IDIt->Inst` into variable?
- simpler:

```
if (blah && blah != NextInst)
   return true;
```


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

https://reviews.llvm.org/D106989