[PATCH] D86970: [IRSim] Adding IRSimilarityCandidate that contains a region of IRInstructionData

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:244
+                                    const IRSimilarityCandidate &B) {
+  if (A.StartIdx <= B.getEndIdx() && A.StartIdx >= B.StartIdx)
+    return true;
----------------
Can reduce code duplication here using a lambda:

```
auto DoesOverlap = [](const IRSimilarityCandidate &X,
                      const IRSimilarityCandidate &Y) {
  // Check:
  // XXXXXX        X starts before Y ends
  //      YYYYYYY  Y starts after X starts
  return X.StartIdx <= Y.getEndIdx() && Y.StartIdx >= B.StartIdx
};
return DoesOverlap(A, B) || DoesOverlap(B, A);
```


================
Comment at: llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp:1293
+
+// Checks that regions of instructions that are identical, except for different
+// debug instructions are identified as similar since debug instructions are
----------------
This sentence is a little hard to parse. Split it into two?

E.g.

> Check that debug instructions do not impact similarity. They are marked as invisible.


================
Comment at: llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp:1306
+                             %2 = add i32 %a, %b
+                             call void @llvm.dbg.value(metadata !1)
+                             %3 = add i32 %b, %a
----------------
Can you add an extra basic block in this test which does not contain a debug value?

e.g.

```
bb3:
  %0 = add i32 %a, %b
  %1 = add i32 %b, %a
  ret i32 0
```



================
Comment at: llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp:1330
+
+// Checks that IRSimilarityCandidates that include illegal instructions, are not
+// considered to be the same set of instructions.
----------------
Can you clarify which instruction is illegal in this comment?


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

https://reviews.llvm.org/D86970