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

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added a comment.

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.

> I guess the way it is now, those two strings are not computed if Bisect is disabled, so there's some efficiency gain - any sense whether that's significant/important?

I dont believe once-per-pass-invocation name query is anything essential we need to worry about.


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

https://reviews.llvm.org/D58406