[PATCH] D86971: [IRSim] Adding structural comparison to IRSimilarityCandidate.

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs accepted this revision.
jroelofs added a comment.
This revision is now accepted and ready to land.

LGTM, with some optional suggestions.



================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:407
+/// We cannot create the same mapping since the use of c4 is not used in the
+/// same was as %b or c2.
 class IRSimilarityCandidate {
----------------



================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:466-470
+  static bool compareOperandMapping(
+      const IRSimilarityCandidate &A, const IRSimilarityCandidate &B,
+      ArrayRef<Value *> &OperValsA, ArrayRef<Value *> &OperValsB,
+      DenseMap<unsigned, DenseSet<unsigned>> &ValueNumberMappingA,
+      DenseMap<unsigned, DenseSet<unsigned>> &ValueNumberMappingB);
----------------
Functions with a ton of parameters like this can sometimes get a bit unwieldy, but this one has an obvious structure that could be taken advantage of. What do you think about making the signature be:

```
struct OperandMapping {
  const IRSimilarityCandidate &IRSC;
  ArrayRef<Value *> &OperVals;
  DenseMap<unsigned, DenseSet<unsigned>> &ValueNumberMapping;
};

static bool compareOperandMapping(OperandMapping A, OperandMapping B);
```

Then the call site is just:

```
compareOperandMapping({ A, OperVals, ValueNumberMapping },
                      { B, OtherOperVals, OtherValueNumberMapping });
```

and everything gets grouped succinctly.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:333
+
+  iterator It = A.begin();
+  iterator OIt = B.begin();
----------------
In here, all the names are Thing/OtherThing-ish, whereas in compareOperandMapping they're AThing/BThing-ish. I think it'd be helpful to keep the latter style to have them consistent.


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

https://reviews.llvm.org/D86971