[PATCH] D19782: [IPO/GlobalDCE] Convert the pass to use static functions.

[Chandler Carruth via llvm-commits]

On Mon, May 2, 2016 at 12:55 PM Justin Bogner via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Mehdi Amini <mehdi.amini at apple.com> writes:
> >> On May 2, 2016, at 11:20 AM, Justin Bogner <mail at justinbogner.com>
> wrote:
> >>
> >> Mehdi AMINI <mehdi.amini at apple.com> writes:
> >>> Davide Italiano <dccitaliano at gmail.com> writes:
> >>>> Mehdi AMINI <mehdi.amini at apple.com> writes:
> >>>>> ================
> >>>>> Comment at: include/llvm/Transforms/IPO/GlobalDCE.h:44
> >>>>> @@ +43,3 @@
> >>>>> +  bool RemoveUnusedGlobalValue(GlobalValue &GV);
> >>>>> +};
> >>>>> +
> >>>>> ----------------
> >>>>> Can we split what is the "pass" and what is the actual utility?
> >>>>
> >>>> I think it's possible, but maybe overkill? Probably people with more
> >>>> experience porting passes can tell what they think.
> >>>
> >>> I think this should be the standard layout, it should be the exception
> >>> to merge together the utility logic and the pass logic.
> >>
> >> Why? One of the nice things about the new pass manager design is that
> >> the pass *is* a utility. They're just objects that can do the transforms
> >> you care about and that happen to have a passmanager-compatible run
> >> method.
> >
> > It does not "happen to have", it *forces you* to have.  If it just
> > "happened to have" the right interface, there shouldn't be an include
> > for the PassManager.
>
> Well, the interface uses the PreservedAnalyses and AnalysisManager
> types, that are defined in the PassManager include, so it's pretty hard
> to have the right interface without it...
>
> >
> >>
> >> I don't see how adding an extra layer of indirection would make things
> >> better, in the general case.
> >
> > single responsibility principle, in the general case...
>
> I'm not convinced having a single utility that is a pass violates SRP.
> The class does one thing - the transformation. There may be multiple
> interfaces in terms of how it's called, but that's a different thing.
>
> OTOH, adding a layer of abstraction for no reason falls afoul of the
> "You aren't gonna need it" adage. It's building something we don't have
> a use for.
>
> In some cases breaking these up into multiple pieces will certainly make
> sense and make for a better design, but for simple passes like this it's
> a waste of time.
>

FWIW, I tend to agree with Justin here.

I think the feeling I have about when this tips in favor of factoring
things out is when there are multiple different usage scenarios, especially
if that motivates us to have a completely non-pass utility API in a header
like Transforms/Utils does.

-Chandler


> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160502/0f308bc4/attachment.html>