[PATCH] D58406: Fix IR/Analysis layering issue in OptBisect

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D58406#1404198 <https://reviews.llvm.org/D58406#1404198>, @fedor.sergeev wrote:

> In D58406#1404179 <https://reviews.llvm.org/D58406#1404179>, @dblaikie wrote:
>
> > Alternatively - how would you feel about callers passing in the strings directly?
> >
> >   return !BisectEnabled || checkPass(P->getPassName(), P->getDescription(U));
> >   
> >
> > The Pass and Unit are only used to retrieve the Pass Name and Description (passing the Unit straight back to the Pass that just passed it in) - so why not pass in those (Pass Name and Description) instead?
>
>
> You mean e.g. changing
>
>   !F.getContext().getOptPassGate().shouldRunPass(this, &F))
>
> into
>
>   !F.getContext().getOptPassGate().shouldRunPass(this->getName(), F.getDescription()))
>   
>
> Yep, that would resolve the layering issue w/o requiring opaque pointers.
>
> Btw, in new-pm's pass instrumentation we do pass Pass Name instead of a pointer to pass exactly because of this layering issue.
>  However, note, that there we still pass IR unit itself (wrapped into llvm::Any) instead of "description"
>  because instrumentations framework is generic and instrumentation functionality is not predefined.
>  And yet, specific instrumentations like PrintIR do need IRUnit's description and right now every instrumentation does it on its own.
>
> If we could implement a generic way of querying IRUnit description (w/o introducing a layering issue), it would
>  be beneficial both for legacy and new pm implementations.


OK - so sounds like you have a slight preference for keeping the description callback (though that requires also passing in the pass, not just the pass name) & using llvm::Any?

Or keeping the description API (maybe removing its virtuality until that's needed in the new PM version?) but having passes call it locally themselves, rather than being called back from OptBisect?


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

https://reviews.llvm.org/D58406