[llvm-dev] RFC: Removing TerminatorInst, simplifying calls

[Chandler Carruth via llvm-dev]

On Thu, May 17, 2018 at 1:33 PM Xinliang David Li via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

>
>
> On Thu, May 17, 2018 at 1:24 PM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>> On Thu, May 17, 2018 at 10:32 AM Xinliang David Li <xinliangli at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Thu, May 17, 2018 at 2:03 AM, Chandler Carruth via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> Going to keep this RFC short and to the point:
>>>>
>>>> TerminatorInst doesn't pull its weight in the type system. There is
>>>> essentially a single relevant API -- iterating successors. There is no
>>>> other interesting aspect shared -- the interface itself just dispatches to
>>>> specific instructions to be implemented.
>>>>
>>>> On the flip side, CallInst and InvokeInst have *massive* amounts of
>>>> code shared and struggle to be effective due to being unable to share a
>>>> base class in the type system. We have CallSite and a host of other
>>>> complexity trying to cope with this, and honestly, it isn't doing such a
>>>> great job.
>>>>
>>>> I propose we make "terminator-ness" a *property* of an instruction and
>>>> take it out of the type system. We can build a handful of APIs to dispatch
>>>> between instructions with this property and expose successors. This should
>>>> be really comparable to the existing code and have nearly no down sides.
>>>>
>>>> Then I propose we restructure the type system to allow CallInst and
>>>> InvokeInst to easily share logic:
>>>> - Create `CallBase` which is an *abstract* class derived from
>>>> Instruction that provides all of the common call logic
>>>> - Make `CallInst` derive from this
>>>> - Make `InvokeInst` derive from this, extend it for EH aspects and
>>>> successors
>>>> - Remove `CallSite` and all accompanying code, rewriting it to use
>>>> `CallBase`.
>>>>
>>>>
>>> Agree that this is area that needs cleanup.
>>>
>>> How about simplifying the type system even more -- Make CallInst the
>>> base class, and let InvokeInst Derive from it (i.e, no need for CallBase).
>>>
>>
>> It is important to be able to easily identify a non-invoke call. Writing:
>> "isa<CallInst>(...) && !isa<InvokeInst>(...)" would be really painful and
>> is a sign that this isn't the right model IMO.
>>
>>
>
>
> You have a point here, the question is how often this kind of query really
> happens?  Conceptually, it is more often to see these two patterns: 1)
> common logic which only checks isa<CallInst> and 2) Invoke Specific logic
> which checks isa<InvokeInst>
>

But all three will be available:

isa<CallBase>
isa<CallInst>
isa<InvokeInst>

Generally, splitting these out this way is pretty standard OO principle.
I'd be very reluctant to not follow it.


>
> David
>
>
>>
>>> David
>>>
>>>
>>>
>>>> The end result will, IMO, be a much simpler IR type system and
>>>> implementation. The code interacting with instructions should also be much
>>>> more consistent and clear w/o the awkward CallSite "abstraction".
>>>>
>>>> Thoughts? Seem OK at a high level?
>>>>
>>>> Happy to bikeshed the name `CallBase`, but I've discussed this with
>>>> several folks, including Reid and Chris and nothing better came up really.
>>>> `CallSite` might be nicer, but the confusion with the *existing* type seems
>>>> much more problematic.
>>>>
>>>>
>>>> Assuming folks are happy with this direction, are there any incremental
>>>> patches that folks would like to see in pre-commit review? I've only done
>>>> some initial investigation of what it takes to cut this through. Provided
>>>> folks are positive about the direction, I'll work on what this would
>>>> actually look like in practice.
>>>>
>>>> -Chandler
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>>
>>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180517/143b16b8/attachment.html>