[llvm-dev] Opt Bisect layering

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
So I'm poking around trying to improve the layering of LLVM (using an internal strict modular build as a motivation/tool to identify issues) & came across a layering violation in the OptBisect implementation.

This feature is implemented in lib/IR, but includes several headers from include/llvm/Analysis - which would create a circular dependency if not for the fact that all the headers it includes are header-only (templates).

Any ideas about how this could/should be fixed? Perhaps these analyses should be moved into somewhere more common - but I don't know there's anywhere more (or less) common than lib/IR itself (since I assume they depend on IR & that's where OptBisect is, so they can't be lower/higher than that).

Does that sound OK? Anyone have other/better ideas?

- Dave

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev

It looks like the dependency is really minimal, just an informational query about a Loop/Region/CallGraphSCC and not on any actual analysis.  Seems like there should be a pattern that would make that reasonable while maintaining the layering.  Maybe a minimalist IRAggregate class that supports what OptBisect needs and becomes a base for the others?  (Don't you love people who do drive-by hand-waving?)

--paulr

 

From: llvm-dev [mailto:[hidden email]] On Behalf Of David Blaikie via llvm-dev
Sent: Wednesday, March 21, 2018 12:09 PM
To: llvm-dev; Kaylor, Andrew; Friedman, Eli
Subject: [llvm-dev] Opt Bisect layering

 

So I'm poking around trying to improve the layering of LLVM (using an internal strict modular build as a motivation/tool to identify issues) & came across a layering violation in the OptBisect implementation.

This feature is implemented in lib/IR, but includes several headers from include/llvm/Analysis - which would create a circular dependency if not for the fact that all the headers it includes are header-only (templates).

Any ideas about how this could/should be fixed? Perhaps these analyses should be moved into somewhere more common - but I don't know there's anywhere more (or less) common than lib/IR itself (since I assume they depend on IR & that's where OptBisect is, so they can't be lower/higher than that).

Does that sound OK? Anyone have other/better ideas?

- Dave


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
In reply to this post by Dean Michael Berris via llvm-dev

There is a patch under review right now from someone who wants to provide a mechanism to replace OptBisect as the pass gate keeping mechanism.

 

https://reviews.llvm.org/D44464

 

Possibly we could take this opportunity to move OptBisect to a different layer, though I don’t know where else it would belong.

 

The other possibility is that we could have the caller pass in a description instead of a pointer to the pass and the IR unit. OptBisect isn’t doing anything with them other than building a string for output.

 

-Andy

 

From: David Blaikie [mailto:[hidden email]]
Sent: Wednesday, March 21, 2018 12:09 PM
To: llvm-dev <[hidden email]>; Kaylor, Andrew <[hidden email]>; Friedman, Eli <[hidden email]>
Subject: Opt Bisect layering

 

So I'm poking around trying to improve the layering of LLVM (using an internal strict modular build as a motivation/tool to identify issues) & came across a layering violation in the OptBisect implementation.

This feature is implemented in lib/IR, but includes several headers from include/llvm/Analysis - which would create a circular dependency if not for the fact that all the headers it includes are header-only (templates).

Any ideas about how this could/should be fixed? Perhaps these analyses should be moved into somewhere more common - but I don't know there's anywhere more (or less) common than lib/IR itself (since I assume they depend on IR & that's where OptBisect is, so they can't be lower/higher than that).

Does that sound OK? Anyone have other/better ideas?

- Dave


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
In reply to this post by Dean Michael Berris via llvm-dev
Andrew,

I would not make the caller pass the description of the IR unit. That is because it would result in the description generated every time even if OptBisect is disabled. Description generation is not very chip.
Thinking on the OptBisect extension, I believe passing the units are the right choice because OptPassGates may use them to make pass skipping decisions.

-Yevgeny Rouban
-----------------------------------------------------------

From: llvm-dev [[hidden email]] On Behalf Of Kaylor, Andrew via llvm-dev
Sent: Thursday, March 22, 2018 3:52 AM
To: David Blaikie <[hidden email]>; llvm-dev <[hidden email]>; Friedman, Eli <[hidden email]>
Subject: Re: [llvm-dev] Opt Bisect layering

 

There is a patch under review right now from someone who wants to provide a mechanism to replace OptBisect as the pass gate keeping mechanism.

 

https://reviews.llvm.org/D44464

 

Possibly we could take this opportunity to move OptBisect to a different layer, though I don’t know where else it would belong.

 

The other possibility is that we could have the caller pass in a description instead of a pointer to the pass and the IR unit. OptBisect isn’t doing anything with them other than building a string for output.

 

-Andy



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
So... looking at OptBisect, I have a few thoughts:

1) what's the purpose of the virtual interface/OptPassGate? I'm guessing maybe that worked around the circular referencing in these APIs? hmm, no, I suppose that wouldn't work/be relevant here.

2) Why is OptBisector a ManagedStatic? That seems pretty antithetical to the role of LLVMContext. When/why would a user be bisecting over multiple LLVMContexts? & even then, maybe it'd be more suitable for that grouping (the scope for the bisection) to be API driven - passing the bisector into the LLVMContext ctor to define the set of contexts that share a bisector?

On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev <[hidden email]> wrote:
Andrew,

I would not make the caller pass the description of the IR unit. That is because it would result in the description generated every time even if OptBisect is disabled. Description generation is not very chip.
Thinking on the OptBisect extension, I believe passing the units are the right choice because OptPassGates may use them to make pass skipping decisions.

-Yevgeny Rouban
-----------------------------------------------------------

From: llvm-dev [[hidden email]] On Behalf Of Kaylor, Andrew via llvm-dev
Sent: Thursday, March 22, 2018 3:52 AM
To: David Blaikie <[hidden email]>; llvm-dev <[hidden email]>; Friedman, Eli <[hidden email]>
Subject: Re: [llvm-dev] Opt Bisect layering

 

There is a patch under review right now from someone who wants to provide a mechanism to replace OptBisect as the pass gate keeping mechanism.

 

https://reviews.llvm.org/D44464

 

Possibly we could take this opportunity to move OptBisect to a different layer, though I don’t know where else it would belong.

 

The other possibility is that we could have the caller pass in a description instead of a pointer to the pass and the IR unit. OptBisect isn’t doing anything with them other than building a string for output.

 

-Andy


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
& now looking back at the patch-in-progress, I see it allows setting the OptBisector/OptPassGate as suggested in (2).

If that becomes the /only/ option (ie: LLVMContext has no default OptPassGate) then the virtual interface could be kept down in IR (though it's still a bit questionable to have those Analysis types (Loop, Region, CallGraphSCC) even declared in IR). Then the implementation of OptBisector could be moved into Analysis - freely able to depend on the concrete Analysis types.

- Dave

On Thu, Mar 29, 2018 at 2:01 PM David Blaikie <[hidden email]> wrote:
So... looking at OptBisect, I have a few thoughts:

1) what's the purpose of the virtual interface/OptPassGate? I'm guessing maybe that worked around the circular referencing in these APIs? hmm, no, I suppose that wouldn't work/be relevant here.

2) Why is OptBisector a ManagedStatic? That seems pretty antithetical to the role of LLVMContext. When/why would a user be bisecting over multiple LLVMContexts? & even then, maybe it'd be more suitable for that grouping (the scope for the bisection) to be API driven - passing the bisector into the LLVMContext ctor to define the set of contexts that share a bisector?

On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev <[hidden email]> wrote:
Andrew,

I would not make the caller pass the description of the IR unit. That is because it would result in the description generated every time even if OptBisect is disabled. Description generation is not very chip.
Thinking on the OptBisect extension, I believe passing the units are the right choice because OptPassGates may use them to make pass skipping decisions.

-Yevgeny Rouban
-----------------------------------------------------------

From: llvm-dev [[hidden email]] On Behalf Of Kaylor, Andrew via llvm-dev
Sent: Thursday, March 22, 2018 3:52 AM
To: David Blaikie <[hidden email]>; llvm-dev <[hidden email]>; Friedman, Eli <[hidden email]>
Subject: Re: [llvm-dev] Opt Bisect layering

 

There is a patch under review right now from someone who wants to provide a mechanism to replace OptBisect as the pass gate keeping mechanism.

 

https://reviews.llvm.org/D44464

 

Possibly we could take this opportunity to move OptBisect to a different layer, though I don’t know where else it would belong.

 

The other possibility is that we could have the caller pass in a description instead of a pointer to the pass and the IR unit. OptBisect isn’t doing anything with them other than building a string for output.

 

-Andy


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
On 03/30/2018 12:05 AM, David Blaikie via llvm-dev wrote:
 > & now looking back at the patch-in-progress, I see it allows setting
the OptBisector/OptPassGate as suggested in (2).
Well, the patch currently discussed does not attempt to solve the
passgate object management issue.
It is left for the discretion of passgate object provider.

 >
 > If that becomes the /only/ option (ie: LLVMContext has no default
OptPassGate) then the virtual interface could be kept down in IR (though
it's still a bit questionable to have those Analysis types (Loop,
Region, CallGraphSCC) even declared in IR). Then the implementation of
OptBisector could be moved into Analysis - freely able to depend on the
concrete Analysis types.

To me this is a "Pass Manager catch" - entity that attempts to control
all the passes needs to be part of (or tightly cooperate with) pass manager.
Pass manager is currently in IR, and perhaps rightfully so.
Yet passes that it controls work on "IR units" which are either IR or
Analysis, thus Analysis leaks into the interfaces inevitably.
Kinda logical conflict it is...

regards,
   Fedor.

 >
 > - Dave
 >
 > On Thu, Mar 29, 2018 at 2:01 PM David Blaikie <[hidden email]> wrote:
 >
 >     So... looking at OptBisect, I have a few thoughts:
 >
 >     1) what's the purpose of the virtual interface/OptPassGate? I'm
guessing maybe that worked around the circular referencing in these
APIs? hmm, no, I suppose that wouldn't work/be relevant here.
 >
 >     2) Why is OptBisector a ManagedStatic? That seems pretty
antithetical to the role of LLVMContext. When/why would a user be
bisecting over multiple LLVMContexts? & even then, maybe it'd be more
suitable for that grouping (the scope for the bisection) to be API
driven - passing the bisector into the LLVMContext ctor to define the
set of contexts that share a bisector?
 >
 >     On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev
<[hidden email]> wrote:
 >
 >         Andrew,
 >
 >         I would not make the caller pass the description of the IR
unit. That is because it would result in the description generated every
time even if OptBisect is disabled. Description generation is not very chip.
 >         Thinking on the OptBisect extension, I believe passing the
units are the right choice because OptPassGates may use them to make
pass skipping decisions.
 >
 >         -Yevgeny Rouban
 > -----------------------------------------------------------
 >
 >         From: llvm-dev [mailto:[hidden email]] On
Behalf Of Kaylor, Andrew via llvm-dev
 >         Sent: Thursday, March 22, 2018 3:52 AM
 >         To: David Blaikie <[hidden email]>; llvm-dev
<[hidden email]>; Friedman, Eli <[hidden email]>
 >         Subject: Re: [llvm-dev] Opt Bisect layering
 >
 >
 >
 >         There is a patch under review right now from someone who
wants to provide a mechanism to replace OptBisect as the pass gate
keeping mechanism.
 >
 >
 >
 >         https://reviews.llvm.org/D44464
 >
 >
 >
 >         Possibly we could take this opportunity to move OptBisect to
a different layer, though I don’t know where else it would belong.
 >
 >
 >
 >         The other possibility is that we could have the caller pass
in a description instead of a pointer to the pass and the IR unit.
OptBisect isn’t doing anything with them other than building a string
for output.
 >
 >
 >
 >         -Andy
 >
 >         _______________________________________________
 >         LLVM Developers mailing list
 >         [hidden email]
 >         http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >
 >
 >
 > _______________________________________________
 > LLVM Developers mailing list
 > [hidden email]
 > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev


On Mon, Apr 2, 2018 at 11:32 PM Fedor Sergeev via llvm-dev <[hidden email]> wrote:
On 03/30/2018 12:05 AM, David Blaikie via llvm-dev wrote:
 > & now looking back at the patch-in-progress, I see it allows setting
the OptBisector/OptPassGate as suggested in (2).
Well, the patch currently discussed does not attempt to solve the
passgate object management issue.
It is left for the discretion of passgate object provider.

 >
 > If that becomes the /only/ option (ie: LLVMContext has no default
OptPassGate) then the virtual interface could be kept down in IR (though
it's still a bit questionable to have those Analysis types (Loop,
Region, CallGraphSCC) even declared in IR). Then the implementation of
OptBisector could be moved into Analysis - freely able to depend on the
concrete Analysis types.

To me this is a "Pass Manager catch" - entity that attempts to control
all the passes needs to be part of (or tightly cooperate with) pass manager.
Pass manager is currently in IR, and perhaps rightfully so.
Yet passes that it controls work on "IR units" which are either IR or
Analysis, thus Analysis leaks into the interfaces inevitably.
Kinda logical conflict it is...

This is in response to my "it's still a bit questionable" comment? That's not too important - I'm not pushing to change that if we can get the mechanical layering functional regardless, by only having forward declarations of those different Analysis entities in llvm/IR, but not need their definitions except in the implementation of this virtual interface which could live in llvm/Analysis.

But to discuss it anyway: It seems a bit different that the "Pass Manager catch" depends on the concrete types but the Pass Manager (PassManager.h) itself, does not - it's only templates, none of it depends on Region, Loop, etc. If the catch could be implemented similarly to the manager itself, then it'd have the same layering requirements & no problem. But I haven't looked closely enough at the APIs to figure out if/how that might be done - the current implementation/mechanisms are at odds because of the incompatibility of templates and virtual dispatch (can't have a virtual function template - it'd have an unbounded/unknowable number of vtable entries, etc). Some sort of visitor-y thing might be needed/useful, I'm not sure. But again, not sure this is necessary to address/fix for the issues I'm seeing/pushing to deal with - but I'm happy to discuss/help design this area as well if you'd like :)

- Dave
 

regards,
   Fedor.

 >
 > - Dave
 >
 > On Thu, Mar 29, 2018 at 2:01 PM David Blaikie <[hidden email]> wrote:
 >
 >     So... looking at OptBisect, I have a few thoughts:
 >
 >     1) what's the purpose of the virtual interface/OptPassGate? I'm
guessing maybe that worked around the circular referencing in these
APIs? hmm, no, I suppose that wouldn't work/be relevant here.
 >
 >     2) Why is OptBisector a ManagedStatic? That seems pretty
antithetical to the role of LLVMContext. When/why would a user be
bisecting over multiple LLVMContexts? & even then, maybe it'd be more
suitable for that grouping (the scope for the bisection) to be API
driven - passing the bisector into the LLVMContext ctor to define the
set of contexts that share a bisector?
 >
 >     On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev
<[hidden email]> wrote:
 >
 >         Andrew,
 >
 >         I would not make the caller pass the description of the IR
unit. That is because it would result in the description generated every
time even if OptBisect is disabled. Description generation is not very chip.
 >         Thinking on the OptBisect extension, I believe passing the
units are the right choice because OptPassGates may use them to make
pass skipping decisions.
 >
 >         -Yevgeny Rouban
 > -----------------------------------------------------------
 >
 >         From: llvm-dev [mailto:[hidden email]] On
Behalf Of Kaylor, Andrew via llvm-dev
 >         Sent: Thursday, March 22, 2018 3:52 AM
 >         To: David Blaikie <[hidden email]>; llvm-dev
<[hidden email]>; Friedman, Eli <[hidden email]>
 >         Subject: Re: [llvm-dev] Opt Bisect layering
 >
 >
 >
 >         There is a patch under review right now from someone who
wants to provide a mechanism to replace OptBisect as the pass gate
keeping mechanism.
 >
 >
 >
 >         https://reviews.llvm.org/D44464
 >
 >
 >
 >         Possibly we could take this opportunity to move OptBisect to
a different layer, though I don’t know where else it would belong.
 >
 >
 >
 >         The other possibility is that we could have the caller pass
in a description instead of a pointer to the pass and the IR unit.
OptBisect isn’t doing anything with them other than building a string
for output.
 >
 >
 >
 >         -Andy
 >
 >         _______________________________________________
 >         LLVM Developers mailing list
 >         [hidden email]
 >         http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >
 >
 >
 > _______________________________________________
 > LLVM Developers mailing list
 > [hidden email]
 > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
 > Pass Manager (PassManager.h) itself, does not - it's only templates,
none of it depends on Region, Loop, etc.
Well, true but the problem happens when you try to instantiate the thing.
And for generic features like opt-bisection, ir-print-after-all etc that
want:
   - to have a say before/on/after every execution of every pass
   - have a shared implementation of the main logic

you have to do most of the following:
   1. instantiate your interfaces for all the IRUnits
   2. have pass manager doing the job for you directly
   3. extend pass interface with specific helpers for your job
(skipFunction)

neither of those helps perfect layering...
And with new pass manager having no common Pass hierarchy this gets even
more clumsy.

 > I'm happy to discuss/help design this area as well if you'd like :)
Yeah, I'm interested to continue this design discussion, although my
interests
are primarily in the area of new pass manager currently.
I'm going to post a separate RFC on that topic.

regards,
   Fedor.

On 04/03/2018 06:16 PM, David Blaikie wrote:
 >
 >
 > On Mon, Apr 2, 2018 at 11:32 PM Fedor Sergeev via llvm-dev
<[hidden email]> wrote:
 >
 >     On 03/30/2018 12:05 AM, David Blaikie via llvm-dev wrote:
 >      > & now looking back at the patch-in-progress, I see it allows
setting
 >     the OptBisector/OptPassGate as suggested in (2).
 >     Well, the patch currently discussed does not attempt to solve the
 >     passgate object management issue.
 >     It is left for the discretion of passgate object provider.
 >
 >      >
 >      > If that becomes the /only/ option (ie: LLVMContext has no default
 >     OptPassGate) then the virtual interface could be kept down in IR
(though
 >     it's still a bit questionable to have those Analysis types (Loop,
 >     Region, CallGraphSCC) even declared in IR). Then the
implementation of
 >     OptBisector could be moved into Analysis - freely able to depend
on the
 >     concrete Analysis types.
 >
 >     To me this is a "Pass Manager catch" - entity that attempts to
control
 >     all the passes needs to be part of (or tightly cooperate with)
pass manager.
 >     Pass manager is currently in IR, and perhaps rightfully so.
 >     Yet passes that it controls work on "IR units" which are either IR or
 >     Analysis, thus Analysis leaks into the interfaces inevitably.
 >     Kinda logical conflict it is...
 >
 >
 > This is in response to my "it's still a bit questionable" comment?
That's not too important - I'm not pushing to change that if we can get
the mechanical layering functional regardless, by only having forward
declarations of those different Analysis entities in llvm/IR, but not
need their definitions except in the implementation of this virtual
interface which could live in llvm/Analysis.
 >
 > But to discuss it anyway: It seems a bit different that the "Pass
Manager catch" depends on the concrete types but the Pass Manager
(PassManager.h) itself, does not - it's only templates, none of it
depends on Region, Loop, etc. If the catch could be implemented
similarly to the manager itself, then it'd have the same layering
requirements & no problem. But I haven't looked closely enough at the
APIs to figure out if/how that might be done - the current
implementation/mechanisms are at odds because of the incompatibility of
templates and virtual dispatch (can't have a virtual function template -
it'd have an unbounded/unknowable number of vtable entries, etc). Some
sort of visitor-y thing might be needed/useful, I'm not sure. But again,
not sure this is necessary to address/fix for the issues I'm
seeing/pushing to deal with - but I'm happy to discuss/help design this
area as well if you'd like :)
 >
 > - Dave
 >
 >
 >
 >     regards,
 >        Fedor.
 >
 >      >
 >      > - Dave
 >      >
 >      > On Thu, Mar 29, 2018 at 2:01 PM David Blaikie
<[hidden email]> wrote:
 >      >
 >      >     So... looking at OptBisect, I have a few thoughts:
 >      >
 >      >     1) what's the purpose of the virtual
interface/OptPassGate? I'm
 >     guessing maybe that worked around the circular referencing in these
 >     APIs? hmm, no, I suppose that wouldn't work/be relevant here.
 >      >
 >      >     2) Why is OptBisector a ManagedStatic? That seems pretty
 >     antithetical to the role of LLVMContext. When/why would a user be
 >     bisecting over multiple LLVMContexts? & even then, maybe it'd be more
 >     suitable for that grouping (the scope for the bisection) to be API
 >     driven - passing the bisector into the LLVMContext ctor to define the
 >     set of contexts that share a bisector?
 >      >
 >      >     On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev
 >     <[hidden email]> wrote:
 >      >
 >      >         Andrew,
 >      >
 >      >         I would not make the caller pass the description of the IR
 >     unit. That is because it would result in the description
generated every
 >     time even if OptBisect is disabled. Description generation is not
very chip.
 >      >         Thinking on the OptBisect extension, I believe passing the
 >     units are the right choice because OptPassGates may use them to make
 >     pass skipping decisions.
 >      >
 >      >         -Yevgeny Rouban
 >      > -----------------------------------------------------------
 >      >
 >      >         From: llvm-dev [mailto:[hidden email]] On
 >     Behalf Of Kaylor, Andrew via llvm-dev
 >      >         Sent: Thursday, March 22, 2018 3:52 AM
 >      >         To: David Blaikie <[hidden email]>; llvm-dev
 >     <[hidden email]>; Friedman, Eli <[hidden email]>
 >      >         Subject: Re: [llvm-dev] Opt Bisect layering
 >      >
 >      >
 >      >
 >      >         There is a patch under review right now from someone who
 >     wants to provide a mechanism to replace OptBisect as the pass gate
 >     keeping mechanism.
 >      >
 >      >
 >      >
 >      >         https://reviews.llvm.org/D44464
 >      >
 >      >
 >      >
 >      >         Possibly we could take this opportunity to move
OptBisect to
 >     a different layer, though I don’t know where else it would belong.
 >      >
 >      >
 >      >
 >      >         The other possibility is that we could have the caller
pass
 >     in a description instead of a pointer to the pass and the IR unit.
 >     OptBisect isn’t doing anything with them other than building a string
 >     for output.
 >      >
 >      >
 >      >
 >      >         -Andy
 >      >
 >      > _______________________________________________
 >      >         LLVM Developers mailing list
 >      >         [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >      >
 >      >
 >      >
 >      > _______________________________________________
 >      > LLVM Developers mailing list
 >      > [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >
 >
 >     _______________________________________________
 >     LLVM Developers mailing list
 >     [hidden email]
 >     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev


On Tue, Apr 3, 2018 at 8:50 AM Fedor Sergeev <[hidden email]> wrote:
 > Pass Manager (PassManager.h) itself, does not - it's only templates,
none of it depends on Region, Loop, etc.
Well, true but the problem happens when you try to instantiate the thing.
And for generic features like opt-bisection, ir-print-after-all etc that
want:
   - to have a say before/on/after every execution of every pass
   - have a shared implementation of the main logic

you have to do most of the following:
   1. instantiate your interfaces for all the IRUnits
   2. have pass manager doing the job for you directly
   3. extend pass interface with specific helpers for your job
(skipFunction)

neither of those helps perfect layering...
And with new pass manager having no common Pass hierarchy this gets even
more clumsy.

Not sure I follow, sorry - when you go to instantiate the pass manager & the catch system - at that point there's a concrete set of passes (you have a dependence on Analysis and Transforms) and entities (regions, loops, etc) so the dependencies seem like they make sense.
 

 > I'm happy to discuss/help design this area as well if you'd like :)
Yeah, I'm interested to continue this design discussion, although my
interests
are primarily in the area of new pass manager currently.
I'm going to post a separate RFC on that topic.

Sounds good.

Looping back for this thing - would it be reasonable to remove the default OptBisect from IR, move it into Transform or some other leafier dependency? Leaving only the basic interface (that can get away with forward declarations of Region, Loop, etc) in IR? Is that likely to be done in the patch under review, or shortly after it?
 

regards,
   Fedor.

On 04/03/2018 06:16 PM, David Blaikie wrote:
 >
 >
 > On Mon, Apr 2, 2018 at 11:32 PM Fedor Sergeev via llvm-dev
<[hidden email]> wrote:
 >
 >     On 03/30/2018 12:05 AM, David Blaikie via llvm-dev wrote:
 >      > & now looking back at the patch-in-progress, I see it allows
setting
 >     the OptBisector/OptPassGate as suggested in (2).
 >     Well, the patch currently discussed does not attempt to solve the
 >     passgate object management issue.
 >     It is left for the discretion of passgate object provider.
 >
 >      >
 >      > If that becomes the /only/ option (ie: LLVMContext has no default
 >     OptPassGate) then the virtual interface could be kept down in IR
(though
 >     it's still a bit questionable to have those Analysis types (Loop,
 >     Region, CallGraphSCC) even declared in IR). Then the
implementation of
 >     OptBisector could be moved into Analysis - freely able to depend
on the
 >     concrete Analysis types.
 >
 >     To me this is a "Pass Manager catch" - entity that attempts to
control
 >     all the passes needs to be part of (or tightly cooperate with)
pass manager.
 >     Pass manager is currently in IR, and perhaps rightfully so.
 >     Yet passes that it controls work on "IR units" which are either IR or
 >     Analysis, thus Analysis leaks into the interfaces inevitably.
 >     Kinda logical conflict it is...
 >
 >
 > This is in response to my "it's still a bit questionable" comment?
That's not too important - I'm not pushing to change that if we can get
the mechanical layering functional regardless, by only having forward
declarations of those different Analysis entities in llvm/IR, but not
need their definitions except in the implementation of this virtual
interface which could live in llvm/Analysis.
 >
 > But to discuss it anyway: It seems a bit different that the "Pass
Manager catch" depends on the concrete types but the Pass Manager
(PassManager.h) itself, does not - it's only templates, none of it
depends on Region, Loop, etc. If the catch could be implemented
similarly to the manager itself, then it'd have the same layering
requirements & no problem. But I haven't looked closely enough at the
APIs to figure out if/how that might be done - the current
implementation/mechanisms are at odds because of the incompatibility of
templates and virtual dispatch (can't have a virtual function template -
it'd have an unbounded/unknowable number of vtable entries, etc). Some
sort of visitor-y thing might be needed/useful, I'm not sure. But again,
not sure this is necessary to address/fix for the issues I'm
seeing/pushing to deal with - but I'm happy to discuss/help design this
area as well if you'd like :)
 >
 > - Dave
 >
 >
 >
 >     regards,
 >        Fedor.
 >
 >      >
 >      > - Dave
 >      >
 >      > On Thu, Mar 29, 2018 at 2:01 PM David Blaikie
<[hidden email]> wrote:
 >      >
 >      >     So... looking at OptBisect, I have a few thoughts:
 >      >
 >      >     1) what's the purpose of the virtual
interface/OptPassGate? I'm
 >     guessing maybe that worked around the circular referencing in these
 >     APIs? hmm, no, I suppose that wouldn't work/be relevant here.
 >      >
 >      >     2) Why is OptBisector a ManagedStatic? That seems pretty
 >     antithetical to the role of LLVMContext. When/why would a user be
 >     bisecting over multiple LLVMContexts? & even then, maybe it'd be more
 >     suitable for that grouping (the scope for the bisection) to be API
 >     driven - passing the bisector into the LLVMContext ctor to define the
 >     set of contexts that share a bisector?
 >      >
 >      >     On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev
 >     <[hidden email]> wrote:
 >      >
 >      >         Andrew,
 >      >
 >      >         I would not make the caller pass the description of the IR
 >     unit. That is because it would result in the description
generated every
 >     time even if OptBisect is disabled. Description generation is not
very chip.
 >      >         Thinking on the OptBisect extension, I believe passing the
 >     units are the right choice because OptPassGates may use them to make
 >     pass skipping decisions.
 >      >
 >      >         -Yevgeny Rouban
 >      > -----------------------------------------------------------
 >      >
 >      >         From: llvm-dev [mailto:[hidden email]] On
 >     Behalf Of Kaylor, Andrew via llvm-dev
 >      >         Sent: Thursday, March 22, 2018 3:52 AM
 >      >         To: David Blaikie <[hidden email]>; llvm-dev
 >     <[hidden email]>; Friedman, Eli <[hidden email]>
 >      >         Subject: Re: [llvm-dev] Opt Bisect layering
 >      >
 >      >
 >      >
 >      >         There is a patch under review right now from someone who
 >     wants to provide a mechanism to replace OptBisect as the pass gate
 >     keeping mechanism.
 >      >
 >      >
 >      >
 >      >         https://reviews.llvm.org/D44464
 >      >
 >      >
 >      >
 >      >         Possibly we could take this opportunity to move
OptBisect to
 >     a different layer, though I don’t know where else it would belong.
 >      >
 >      >
 >      >
 >      >         The other possibility is that we could have the caller
pass
 >     in a description instead of a pointer to the pass and the IR unit.
 >     OptBisect isn’t doing anything with them other than building a string
 >     for output.
 >      >
 >      >
 >      >
 >      >         -Andy
 >      >
 >      > _______________________________________________
 >      >         LLVM Developers mailing list
 >      >         [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >      >
 >      >
 >      >
 >      > _______________________________________________
 >      > LLVM Developers mailing list
 >      > [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >
 >
 >     _______________________________________________
 >     LLVM Developers mailing list
 >     [hidden email]
 >     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
Ping on this - any chance we can look at fixing the OptBisect layering here/now?

Could we move the implementation into Analysis & require users to set it, rather than having it as a default value in IR?

On Tue, Apr 3, 2018 at 9:25 AM David Blaikie <[hidden email]> wrote:
On Tue, Apr 3, 2018 at 8:50 AM Fedor Sergeev <[hidden email]> wrote:
 > Pass Manager (PassManager.h) itself, does not - it's only templates,
none of it depends on Region, Loop, etc.
Well, true but the problem happens when you try to instantiate the thing.
And for generic features like opt-bisection, ir-print-after-all etc that
want:
   - to have a say before/on/after every execution of every pass
   - have a shared implementation of the main logic

you have to do most of the following:
   1. instantiate your interfaces for all the IRUnits
   2. have pass manager doing the job for you directly
   3. extend pass interface with specific helpers for your job
(skipFunction)

neither of those helps perfect layering...
And with new pass manager having no common Pass hierarchy this gets even
more clumsy.

Not sure I follow, sorry - when you go to instantiate the pass manager & the catch system - at that point there's a concrete set of passes (you have a dependence on Analysis and Transforms) and entities (regions, loops, etc) so the dependencies seem like they make sense.
 

 > I'm happy to discuss/help design this area as well if you'd like :)
Yeah, I'm interested to continue this design discussion, although my
interests
are primarily in the area of new pass manager currently.
I'm going to post a separate RFC on that topic.

Sounds good.

Looping back for this thing - would it be reasonable to remove the default OptBisect from IR, move it into Transform or some other leafier dependency? Leaving only the basic interface (that can get away with forward declarations of Region, Loop, etc) in IR? Is that likely to be done in the patch under review, or shortly after it?
 

regards,
   Fedor.

On 04/03/2018 06:16 PM, David Blaikie wrote:
 >
 >
 > On Mon, Apr 2, 2018 at 11:32 PM Fedor Sergeev via llvm-dev
<[hidden email]> wrote:
 >
 >     On 03/30/2018 12:05 AM, David Blaikie via llvm-dev wrote:
 >      > & now looking back at the patch-in-progress, I see it allows
setting
 >     the OptBisector/OptPassGate as suggested in (2).
 >     Well, the patch currently discussed does not attempt to solve the
 >     passgate object management issue.
 >     It is left for the discretion of passgate object provider.
 >
 >      >
 >      > If that becomes the /only/ option (ie: LLVMContext has no default
 >     OptPassGate) then the virtual interface could be kept down in IR
(though
 >     it's still a bit questionable to have those Analysis types (Loop,
 >     Region, CallGraphSCC) even declared in IR). Then the
implementation of
 >     OptBisector could be moved into Analysis - freely able to depend
on the
 >     concrete Analysis types.
 >
 >     To me this is a "Pass Manager catch" - entity that attempts to
control
 >     all the passes needs to be part of (or tightly cooperate with)
pass manager.
 >     Pass manager is currently in IR, and perhaps rightfully so.
 >     Yet passes that it controls work on "IR units" which are either IR or
 >     Analysis, thus Analysis leaks into the interfaces inevitably.
 >     Kinda logical conflict it is...
 >
 >
 > This is in response to my "it's still a bit questionable" comment?
That's not too important - I'm not pushing to change that if we can get
the mechanical layering functional regardless, by only having forward
declarations of those different Analysis entities in llvm/IR, but not
need their definitions except in the implementation of this virtual
interface which could live in llvm/Analysis.
 >
 > But to discuss it anyway: It seems a bit different that the "Pass
Manager catch" depends on the concrete types but the Pass Manager
(PassManager.h) itself, does not - it's only templates, none of it
depends on Region, Loop, etc. If the catch could be implemented
similarly to the manager itself, then it'd have the same layering
requirements & no problem. But I haven't looked closely enough at the
APIs to figure out if/how that might be done - the current
implementation/mechanisms are at odds because of the incompatibility of
templates and virtual dispatch (can't have a virtual function template -
it'd have an unbounded/unknowable number of vtable entries, etc). Some
sort of visitor-y thing might be needed/useful, I'm not sure. But again,
not sure this is necessary to address/fix for the issues I'm
seeing/pushing to deal with - but I'm happy to discuss/help design this
area as well if you'd like :)
 >
 > - Dave
 >
 >
 >
 >     regards,
 >        Fedor.
 >
 >      >
 >      > - Dave
 >      >
 >      > On Thu, Mar 29, 2018 at 2:01 PM David Blaikie
<[hidden email]> wrote:
 >      >
 >      >     So... looking at OptBisect, I have a few thoughts:
 >      >
 >      >     1) what's the purpose of the virtual
interface/OptPassGate? I'm
 >     guessing maybe that worked around the circular referencing in these
 >     APIs? hmm, no, I suppose that wouldn't work/be relevant here.
 >      >
 >      >     2) Why is OptBisector a ManagedStatic? That seems pretty
 >     antithetical to the role of LLVMContext. When/why would a user be
 >     bisecting over multiple LLVMContexts? & even then, maybe it'd be more
 >     suitable for that grouping (the scope for the bisection) to be API
 >     driven - passing the bisector into the LLVMContext ctor to define the
 >     set of contexts that share a bisector?
 >      >
 >      >     On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev
 >     <[hidden email]> wrote:
 >      >
 >      >         Andrew,
 >      >
 >      >         I would not make the caller pass the description of the IR
 >     unit. That is because it would result in the description
generated every
 >     time even if OptBisect is disabled. Description generation is not
very chip.
 >      >         Thinking on the OptBisect extension, I believe passing the
 >     units are the right choice because OptPassGates may use them to make
 >     pass skipping decisions.
 >      >
 >      >         -Yevgeny Rouban
 >      > -----------------------------------------------------------
 >      >
 >      >         From: llvm-dev [mailto:[hidden email]] On
 >     Behalf Of Kaylor, Andrew via llvm-dev
 >      >         Sent: Thursday, March 22, 2018 3:52 AM
 >      >         To: David Blaikie <[hidden email]>; llvm-dev
 >     <[hidden email]>; Friedman, Eli <[hidden email]>
 >      >         Subject: Re: [llvm-dev] Opt Bisect layering
 >      >
 >      >
 >      >
 >      >         There is a patch under review right now from someone who
 >     wants to provide a mechanism to replace OptBisect as the pass gate
 >     keeping mechanism.
 >      >
 >      >
 >      >
 >      >         https://reviews.llvm.org/D44464
 >      >
 >      >
 >      >
 >      >         Possibly we could take this opportunity to move
OptBisect to
 >     a different layer, though I don’t know where else it would belong.
 >      >
 >      >
 >      >
 >      >         The other possibility is that we could have the caller
pass
 >     in a description instead of a pointer to the pass and the IR unit.
 >     OptBisect isn’t doing anything with them other than building a string
 >     for output.
 >      >
 >      >
 >      >
 >      >         -Andy
 >      >
 >      > _______________________________________________
 >      >         LLVM Developers mailing list
 >      >         [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >      >
 >      >
 >      >
 >      > _______________________________________________
 >      > LLVM Developers mailing list
 >      > [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >
 >
 >     _______________________________________________
 >     LLVM Developers mailing list
 >     [hidden email]
 >     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
David,

we definitely need to address this issue and I did not forget about it.

I'm still fleshing out a proposal on a generic solution for "pass execution control points",
which was inspired by the new pass manager needs but then it appears
we can largerly reuse the implementation for both managers.

Since the implementation should be based on a special Analysis,
it definitely will be able to address this layering issue (as a side-effect :).

I very much hope to be able send something real to the list in less than a week.

regards,
  Fedor.

On 04/24/2018 01:42 AM, David Blaikie wrote:
Ping on this - any chance we can look at fixing the OptBisect layering here/now?

Could we move the implementation into Analysis & require users to set it, rather than having it as a default value in IR?

On Tue, Apr 3, 2018 at 9:25 AM David Blaikie <[hidden email]> wrote:
On Tue, Apr 3, 2018 at 8:50 AM Fedor Sergeev <[hidden email]> wrote:
 > Pass Manager (PassManager.h) itself, does not - it's only templates,
none of it depends on Region, Loop, etc.
Well, true but the problem happens when you try to instantiate the thing.
And for generic features like opt-bisection, ir-print-after-all etc that
want:
   - to have a say before/on/after every execution of every pass
   - have a shared implementation of the main logic

you have to do most of the following:
   1. instantiate your interfaces for all the IRUnits
   2. have pass manager doing the job for you directly
   3. extend pass interface with specific helpers for your job
(skipFunction)

neither of those helps perfect layering...
And with new pass manager having no common Pass hierarchy this gets even
more clumsy.

Not sure I follow, sorry - when you go to instantiate the pass manager & the catch system - at that point there's a concrete set of passes (you have a dependence on Analysis and Transforms) and entities (regions, loops, etc) so the dependencies seem like they make sense.
 

 > I'm happy to discuss/help design this area as well if you'd like :)
Yeah, I'm interested to continue this design discussion, although my
interests
are primarily in the area of new pass manager currently.
I'm going to post a separate RFC on that topic.

Sounds good.

Looping back for this thing - would it be reasonable to remove the default OptBisect from IR, move it into Transform or some other leafier dependency? Leaving only the basic interface (that can get away with forward declarations of Region, Loop, etc) in IR? Is that likely to be done in the patch under review, or shortly after it?
 

regards,
   Fedor.

On 04/03/2018 06:16 PM, David Blaikie wrote:
 >
 >
 > On Mon, Apr 2, 2018 at 11:32 PM Fedor Sergeev via llvm-dev
<[hidden email]> wrote:
 >
 >     On 03/30/2018 12:05 AM, David Blaikie via llvm-dev wrote:
 >      > & now looking back at the patch-in-progress, I see it allows
setting
 >     the OptBisector/OptPassGate as suggested in (2).
 >     Well, the patch currently discussed does not attempt to solve the
 >     passgate object management issue.
 >     It is left for the discretion of passgate object provider.
 >
 >      >
 >      > If that becomes the /only/ option (ie: LLVMContext has no default
 >     OptPassGate) then the virtual interface could be kept down in IR
(though
 >     it's still a bit questionable to have those Analysis types (Loop,
 >     Region, CallGraphSCC) even declared in IR). Then the
implementation of
 >     OptBisector could be moved into Analysis - freely able to depend
on the
 >     concrete Analysis types.
 >
 >     To me this is a "Pass Manager catch" - entity that attempts to
control
 >     all the passes needs to be part of (or tightly cooperate with)
pass manager.
 >     Pass manager is currently in IR, and perhaps rightfully so.
 >     Yet passes that it controls work on "IR units" which are either IR or
 >     Analysis, thus Analysis leaks into the interfaces inevitably.
 >     Kinda logical conflict it is...
 >
 >
 > This is in response to my "it's still a bit questionable" comment?
That's not too important - I'm not pushing to change that if we can get
the mechanical layering functional regardless, by only having forward
declarations of those different Analysis entities in llvm/IR, but not
need their definitions except in the implementation of this virtual
interface which could live in llvm/Analysis.
 >
 > But to discuss it anyway: It seems a bit different that the "Pass
Manager catch" depends on the concrete types but the Pass Manager
(PassManager.h) itself, does not - it's only templates, none of it
depends on Region, Loop, etc. If the catch could be implemented
similarly to the manager itself, then it'd have the same layering
requirements & no problem. But I haven't looked closely enough at the
APIs to figure out if/how that might be done - the current
implementation/mechanisms are at odds because of the incompatibility of
templates and virtual dispatch (can't have a virtual function template -
it'd have an unbounded/unknowable number of vtable entries, etc). Some
sort of visitor-y thing might be needed/useful, I'm not sure. But again,
not sure this is necessary to address/fix for the issues I'm
seeing/pushing to deal with - but I'm happy to discuss/help design this
area as well if you'd like :)
 >
 > - Dave
 >
 >
 >
 >     regards,
 >        Fedor.
 >
 >      >
 >      > - Dave
 >      >
 >      > On Thu, Mar 29, 2018 at 2:01 PM David Blaikie
<[hidden email]> wrote:
 >      >
 >      >     So... looking at OptBisect, I have a few thoughts:
 >      >
 >      >     1) what's the purpose of the virtual
interface/OptPassGate? I'm
 >     guessing maybe that worked around the circular referencing in these
 >     APIs? hmm, no, I suppose that wouldn't work/be relevant here.
 >      >
 >      >     2) Why is OptBisector a ManagedStatic? That seems pretty
 >     antithetical to the role of LLVMContext. When/why would a user be
 >     bisecting over multiple LLVMContexts? & even then, maybe it'd be more
 >     suitable for that grouping (the scope for the bisection) to be API
 >     driven - passing the bisector into the LLVMContext ctor to define the
 >     set of contexts that share a bisector?
 >      >
 >      >     On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev
 >     <[hidden email]> wrote:
 >      >
 >      >         Andrew,
 >      >
 >      >         I would not make the caller pass the description of the IR
 >     unit. That is because it would result in the description
generated every
 >     time even if OptBisect is disabled. Description generation is not
very chip.
 >      >         Thinking on the OptBisect extension, I believe passing the
 >     units are the right choice because OptPassGates may use them to make
 >     pass skipping decisions.
 >      >
 >      >         -Yevgeny Rouban
 >      > -----------------------------------------------------------
 >      >
 >      >         From: llvm-dev [mailto:[hidden email]] On
 >     Behalf Of Kaylor, Andrew via llvm-dev
 >      >         Sent: Thursday, March 22, 2018 3:52 AM
 >      >         To: David Blaikie <[hidden email]>; llvm-dev
 >     <[hidden email]>; Friedman, Eli <[hidden email]>
 >      >         Subject: Re: [llvm-dev] Opt Bisect layering
 >      >
 >      >
 >      >
 >      >         There is a patch under review right now from someone who
 >     wants to provide a mechanism to replace OptBisect as the pass gate
 >     keeping mechanism.
 >      >
 >      >
 >      >
 >      >         https://reviews.llvm.org/D44464
 >      >
 >      >
 >      >
 >      >         Possibly we could take this opportunity to move
OptBisect to
 >     a different layer, though I don’t know where else it would belong.
 >      >
 >      >
 >      >
 >      >         The other possibility is that we could have the caller
pass
 >     in a description instead of a pointer to the pass and the IR unit.
 >     OptBisect isn’t doing anything with them other than building a string
 >     for output.
 >      >
 >      >
 >      >
 >      >         -Andy
 >      >
 >      > _______________________________________________
 >      >         LLVM Developers mailing list
 >      >         [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >      >
 >      >
 >      >
 >      > _______________________________________________
 >      > LLVM Developers mailing list
 >      > [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >
 >
 >     _______________________________________________
 >     LLVM Developers mailing list
 >     [hidden email]
 >     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Opt Bisect layering

Dean Michael Berris via llvm-dev
Ping - did this end up progressing? (I might've missed or forgotten about anything coming out on the list)

On Thu, May 3, 2018 at 7:28 AM Fedor Sergeev <[hidden email]> wrote:
David,

we definitely need to address this issue and I did not forget about it.

I'm still fleshing out a proposal on a generic solution for "pass execution control points",
which was inspired by the new pass manager needs but then it appears
we can largerly reuse the implementation for both managers.

Since the implementation should be based on a special Analysis,
it definitely will be able to address this layering issue (as a side-effect :).

I very much hope to be able send something real to the list in less than a week.

regards,
  Fedor.


On 04/24/2018 01:42 AM, David Blaikie wrote:
Ping on this - any chance we can look at fixing the OptBisect layering here/now?

Could we move the implementation into Analysis & require users to set it, rather than having it as a default value in IR?

On Tue, Apr 3, 2018 at 9:25 AM David Blaikie <[hidden email]> wrote:
On Tue, Apr 3, 2018 at 8:50 AM Fedor Sergeev <[hidden email]> wrote:
 > Pass Manager (PassManager.h) itself, does not - it's only templates,
none of it depends on Region, Loop, etc.
Well, true but the problem happens when you try to instantiate the thing.
And for generic features like opt-bisection, ir-print-after-all etc that
want:
   - to have a say before/on/after every execution of every pass
   - have a shared implementation of the main logic

you have to do most of the following:
   1. instantiate your interfaces for all the IRUnits
   2. have pass manager doing the job for you directly
   3. extend pass interface with specific helpers for your job
(skipFunction)

neither of those helps perfect layering...
And with new pass manager having no common Pass hierarchy this gets even
more clumsy.

Not sure I follow, sorry - when you go to instantiate the pass manager & the catch system - at that point there's a concrete set of passes (you have a dependence on Analysis and Transforms) and entities (regions, loops, etc) so the dependencies seem like they make sense.
 

 > I'm happy to discuss/help design this area as well if you'd like :)
Yeah, I'm interested to continue this design discussion, although my
interests
are primarily in the area of new pass manager currently.
I'm going to post a separate RFC on that topic.

Sounds good.

Looping back for this thing - would it be reasonable to remove the default OptBisect from IR, move it into Transform or some other leafier dependency? Leaving only the basic interface (that can get away with forward declarations of Region, Loop, etc) in IR? Is that likely to be done in the patch under review, or shortly after it?
 

regards,
   Fedor.

On 04/03/2018 06:16 PM, David Blaikie wrote:
 >
 >
 > On Mon, Apr 2, 2018 at 11:32 PM Fedor Sergeev via llvm-dev
<[hidden email]> wrote:
 >
 >     On 03/30/2018 12:05 AM, David Blaikie via llvm-dev wrote:
 >      > & now looking back at the patch-in-progress, I see it allows
setting
 >     the OptBisector/OptPassGate as suggested in (2).
 >     Well, the patch currently discussed does not attempt to solve the
 >     passgate object management issue.
 >     It is left for the discretion of passgate object provider.
 >
 >      >
 >      > If that becomes the /only/ option (ie: LLVMContext has no default
 >     OptPassGate) then the virtual interface could be kept down in IR
(though
 >     it's still a bit questionable to have those Analysis types (Loop,
 >     Region, CallGraphSCC) even declared in IR). Then the
implementation of
 >     OptBisector could be moved into Analysis - freely able to depend
on the
 >     concrete Analysis types.
 >
 >     To me this is a "Pass Manager catch" - entity that attempts to
control
 >     all the passes needs to be part of (or tightly cooperate with)
pass manager.
 >     Pass manager is currently in IR, and perhaps rightfully so.
 >     Yet passes that it controls work on "IR units" which are either IR or
 >     Analysis, thus Analysis leaks into the interfaces inevitably.
 >     Kinda logical conflict it is...
 >
 >
 > This is in response to my "it's still a bit questionable" comment?
That's not too important - I'm not pushing to change that if we can get
the mechanical layering functional regardless, by only having forward
declarations of those different Analysis entities in llvm/IR, but not
need their definitions except in the implementation of this virtual
interface which could live in llvm/Analysis.
 >
 > But to discuss it anyway: It seems a bit different that the "Pass
Manager catch" depends on the concrete types but the Pass Manager
(PassManager.h) itself, does not - it's only templates, none of it
depends on Region, Loop, etc. If the catch could be implemented
similarly to the manager itself, then it'd have the same layering
requirements & no problem. But I haven't looked closely enough at the
APIs to figure out if/how that might be done - the current
implementation/mechanisms are at odds because of the incompatibility of
templates and virtual dispatch (can't have a virtual function template -
it'd have an unbounded/unknowable number of vtable entries, etc). Some
sort of visitor-y thing might be needed/useful, I'm not sure. But again,
not sure this is necessary to address/fix for the issues I'm
seeing/pushing to deal with - but I'm happy to discuss/help design this
area as well if you'd like :)
 >
 > - Dave
 >
 >
 >
 >     regards,
 >        Fedor.
 >
 >      >
 >      > - Dave
 >      >
 >      > On Thu, Mar 29, 2018 at 2:01 PM David Blaikie
<[hidden email]> wrote:
 >      >
 >      >     So... looking at OptBisect, I have a few thoughts:
 >      >
 >      >     1) what's the purpose of the virtual
interface/OptPassGate? I'm
 >     guessing maybe that worked around the circular referencing in these
 >     APIs? hmm, no, I suppose that wouldn't work/be relevant here.
 >      >
 >      >     2) Why is OptBisector a ManagedStatic? That seems pretty
 >     antithetical to the role of LLVMContext. When/why would a user be
 >     bisecting over multiple LLVMContexts? & even then, maybe it'd be more
 >     suitable for that grouping (the scope for the bisection) to be API
 >     driven - passing the bisector into the LLVMContext ctor to define the
 >     set of contexts that share a bisector?
 >      >
 >      >     On Wed, Mar 21, 2018 at 10:20 PM Yevgeny Rouban via llvm-dev
 >     <[hidden email]> wrote:
 >      >
 >      >         Andrew,
 >      >
 >      >         I would not make the caller pass the description of the IR
 >     unit. That is because it would result in the description
generated every
 >     time even if OptBisect is disabled. Description generation is not
very chip.
 >      >         Thinking on the OptBisect extension, I believe passing the
 >     units are the right choice because OptPassGates may use them to make
 >     pass skipping decisions.
 >      >
 >      >         -Yevgeny Rouban
 >      > -----------------------------------------------------------
 >      >
 >      >         From: llvm-dev [mailto:[hidden email]] On
 >     Behalf Of Kaylor, Andrew via llvm-dev
 >      >         Sent: Thursday, March 22, 2018 3:52 AM
 >      >         To: David Blaikie <[hidden email]>; llvm-dev
 >     <[hidden email]>; Friedman, Eli <[hidden email]>
 >      >         Subject: Re: [llvm-dev] Opt Bisect layering
 >      >
 >      >
 >      >
 >      >         There is a patch under review right now from someone who
 >     wants to provide a mechanism to replace OptBisect as the pass gate
 >     keeping mechanism.
 >      >
 >      >
 >      >
 >      >         https://reviews.llvm.org/D44464
 >      >
 >      >
 >      >
 >      >         Possibly we could take this opportunity to move
OptBisect to
 >     a different layer, though I don’t know where else it would belong.
 >      >
 >      >
 >      >
 >      >         The other possibility is that we could have the caller
pass
 >     in a description instead of a pointer to the pass and the IR unit.
 >     OptBisect isn’t doing anything with them other than building a string
 >     for output.
 >      >
 >      >
 >      >
 >      >         -Andy
 >      >
 >      > _______________________________________________
 >      >         LLVM Developers mailing list
 >      >         [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >      >
 >      >
 >      >
 >      > _______________________________________________
 >      > LLVM Developers mailing list
 >      > [hidden email]
 >      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >
 >
 >     _______________________________________________
 >     LLVM Developers mailing list
 >     [hidden email]
 >     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
 >



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev