[PATCH] D86973: [IRSim] Adding wrapper pass for IRSimilarityIdentfier

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:685
+  Optional<SimilarityGroupList> &SimilarityCandidatesOpt = IRSI.getSimilarity();
+  assert(SimilarityCandidatesOpt.hasValue() && "SimilarityCandidates is None!");
+  SimilarityGroupList &SimilarityCandidates =
----------------
AndrewLitteken wrote:
> jroelofs wrote:
> > The accessors for Optional do this check for you, so you can just "dereference" it with `*` or `->` as needed without worrying about adding your own assertion on it.
> > 
> > If it's expected that IRSI will always have this populated once it has been constructed, then maybe getSimilarity() ought to be the one to unwrap the Optional.
> Well findSimilarity needs to have been run, or the constructor where a module has been passed to the IRSI.  So, it's possible that you can construct IRSI where the SimilarityCandidates are None.
> 
> I'm not sure if that changes what you're proposing.  I think it could still make sense to put the derefence in `getSimilarity` since you shouldn't be calling it unless you've populated the IRSI with a module already.
> I think it could still make sense to put the derefence in `getSimilarity`

agreed


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:689
+
+  for (std::vector<IRSimilarityCandidate> &CandVec : SimilarityCandidates) {
+    OS << CandVec.size() << " candidates of length "
----------------
So maybe this is just:

`for (std::vector<IRSimilarityCandidate> &CandVec : IRSI.getSimilarity()) {`

or

`for (std::vector<IRSimilarityCandidate> &CandVec : *IRSI.getSimilarity()) {`

depending on which way you decide to go w/ moving that dereference.


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

https://reviews.llvm.org/D86973