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

Mehdi Amini via llvm-commits llvm-commits at lists.llvm.org
Mon May 2 13:08:05 PDT 2016


> On May 2, 2016, at 12:55 PM, Justin Bogner <mail at justinbogner.com> 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...

That's just confirming that the interface does not just "happen to be..."

> 
>> 
>>> 
>>> 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.

It is exactly the same argument that applied when writing pass in the past, with great success as we saw.






More information about the llvm-commits mailing list