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

[Andrew Litteken via Phabricator via llvm-commits]

AndrewLitteken added inline comments.


================
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);
----------------
jroelofs wrote:
> 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.
I think that would definitely make it easier to work with from the outside.  I'll change it to this style.


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

https://reviews.llvm.org/D86971