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

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

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

Are there any instructions that aren't terminators now, but will become 
terminators with this change?  I'm wondering if this is going to affect 
reading old bitcode, and if so, how it will be handled.

-Krzysztof

On 5/17/2018 4:03 AM, Chandler Carruth via llvm-dev
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`.
> 
> 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
> 
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

Hi,

I'm curious how it would affect the getTerminator() method of a basic block?
I.e., how would one find the terminating instruction in that case? By iterating
over all of them or ...?

Cheers,
Alex.
> On 17. May 2018, at 11:03, 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`.
> 
> 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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: Message signed with OpenPGP
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180517/4cb21518/attachment.sig>

> On 17 May 2018, at 23:42, Alex Denisov via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> I'm curious how it would affect the getTerminator() method of a basic
block? I.e., how would one find the terminating instruction in that case? By
iterating over all of them or ...?
I think we already maintain an index of instructions that are “terminators”. If
we don’t do that yet, we probably should.

-- Dean

+1, sounds like a great idea

And if you're volunteering to do the work, even better! :)

Philip

p.s. Any reason we can't preserve a TerminatorInst type with an isa 
function which just returns true for all our terminators but without 
having terminators actually inherit from it?  If so, we preserve the bit 
of a "type safety" for a variable which is expected to always point to
a
terminator.


On 05/17/2018 02:03 AM, Chandler Carruth via llvm-dev
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`.
>
> 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/8d2d2a6c/attachment.html>

> On 17 May 2018, at 19:03, 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.
> 
I think I’m missing one key point here — there’s a number of types that derive
from TerminatorInst (http://llvm.org/doxygen/classllvm_1_1TerminatorInst.html)
and none of them seem like they’re related to calls. Most seem like branching
instructions and “unreachable”. Is the idea to lift the access to successors to
a separate set of functions, taking an instruction that’s a terminator somehow?
Will it be part of the Instruction base type or something else?
> 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`.
> 
> 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?
> 
IIUC, this is replacing what’s already there, which is a CallBase template
(doing CRTP I suppose) — with something that’s just an abstract base sharing
common code?  That does sound simpler, but it’d be nice to also understand what
removing the CallSite abstraction make easier (for my education).
> 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.
> 
CallPoint? CallLocation? CallBase isn’t bad but it seems like it’s already
there...
> 
> 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.
> 
+1 from me.

-- Dean

On Thu, May 17, 2018 at 6:43 AM Alex Denisov via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I'm curious how it would affect the getTerminator() method of a basic
> block? I.e., how would one find the terminating instruction in that case?
> By iterating over all of them or ...?
>
My understanding of the proposal is that all other existing terminator
invariants would be maintained: each BB still has exactly one terminator
and it must be the last instruction.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180517/bede4355/attachment.html>

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

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/72e4fe83/attachment.html>

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 9:27 AM Philip Reames <listmail at
philipreames.com>
wrote:
> +1, sounds like a great idea
>
> And if you're volunteering to do the work, even better! :)
>
> Philip
>
> p.s. Any reason we can't preserve a TerminatorInst type with an isa
> function which just returns true for all our terminators but without having
> terminators actually inherit from it?  If so, we preserve the bit of a
> "type safety" for a variable which is expected to always point to
a
> terminator.
>
Because it gives you UB instead of type safety?

>
> On 05/17/2018 02:03 AM, Chandler Carruth via llvm-dev 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`.
>
> 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 listllvm-dev at
lists.llvm.orghttp://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/f0d1a28d/attachment.html>

On Thu, May 17, 2018 at 6:23 AM Krzysztof Parzyszek via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Are there any instructions that aren't terminators now, but will become
> terminators with this change?

Nope.

>   I'm wondering if this is going to affect
> reading old bitcode, and if so, how it will be handled.
>
Shouldn't change anything. Just changes the in-memory representation.
We'll
still create the instructions the same way and so bitcode should Just Work.
(but clearly something to test and verify.)

>
> -Krzysztof
>
> On 5/17/2018 4:03 AM, Chandler Carruth via llvm-dev 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`.
> >
> > 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
> >
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> _______________________________________________
> 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/fe92424b/attachment.html>

On 05/17/2018 04:03 AM, Chandler Carruth via llvm-dev
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`.
>
> 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?
I think this is a good idea.
>
> 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.
I think that we should take whatever name we like the best. Out-of-tree
code will fail to compile after the change either way and will require
fixing. That having been said, we already have a CallBase (and a
CallBaseParent), they're just not part of the classof hierarchy, and
using CallBase seems consistent with our other class names.

 -Hal
>
>
> 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
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180517/926bbb66/attachment.html>

> On 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`.
> 
> 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”.
+1, this seems like a great direction.  Random question, which we might have
talked about but I forgot: can we just eliminate CallBase and InvokeInst
entirely?  Why not make “isTerminator()” for calls return true if the call has a
normal/unwind destination set?

-Chris

On Fri, May 18, 2018 at 10:26 PM Chris Lattner via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> > On 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`.
> >
> > 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”.
>
> +1, this seems like a great direction.  Random question, which we might
> have talked about but I forgot: can we just eliminate CallBase and
> InvokeInst entirely?  Why not make “isTerminator()” for calls return true
> if the call has a normal/unwind destination set?
>
Eventually, this might make sense, but I'm not currently certain.

I think storing stuff like successors is useful to model as a separate type
to start. Once we get there, I think it'll be easier to see if the tradeoff
isn't actually worth it and we can just make this a a property of calls.

>
> -Chris
>
> _______________________________________________
> 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/20180518/9ee85d0e/attachment-0001.html>

> On May 17, 2018, at 19:37, Hal Finkel via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> 
> On 05/17/2018 04:03 AM, Chandler Carruth via llvm-dev 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`.
>> 
>> 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?
> 
> I think this is a good idea.
+1
>> 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.
> 
> I think that we should take whatever name we like the best. Out-of-tree
code will fail to compile after the change either way and will require fixing.
That having been said, we already have a CallBase (and a CallBaseParent),
they're just not part of the classof hierarchy, and using CallBase seems
consistent with our other class names.
I agree that using CallBase makes sense here.
>> 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 <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> -- 
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> 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/20180521/747b3ec9/attachment.html>

FWIW, there seemed general happiness with this direction.

I've sent the first patch here: https://reviews.llvm.org/D47467

It also indicates my rough plan of action to churn through the remaining
cleanups to make this happen. I'm expecting the rest of the patches to be
more mechanical in nature honestly.

If you'd like to follow along, help review, or comment on the particulars
of enacting this, hop on the review thread. =D

Currently following Hal's suggestion to go after what seems like the most
obvious names here regardless of collisions. May require a little dancing
about with the incremental patches, but will keep it as minimal as possible.

-Chandler

On Thu, May 17, 2018 at 2:03 AM Chandler Carruth <chandlerc at gmail.com>
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`.
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180529/6e20124e/attachment.html>