[PATCH] D86973: [IRSim] Adding wrapper pass for IRSimilarityIdentfier
Jon Roelofs via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 18 12:01:17 PDT 2020
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
More information about the llvm-commits
mailing list