llvm dev - May 2018 - RFC: Removing TerminatorInst, simplifying calls

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.

>
> 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
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180517/cfccc154/attachment.html>

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>

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
>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180517/de8f7c03/attachment-0001.html>

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>