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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
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
> [hidden email]
> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
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 <[hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev


> On 17 May 2018, at 23:42, Alex Denisov via llvm-dev <[hidden email]> 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
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev

+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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev


> On 17 May 2018, at 19:03, Chandler Carruth via llvm-dev <[hidden email]> 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

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
On Thu, May 17, 2018 at 6:43 AM Alex Denisov via llvm-dev <[hidden email]> 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.

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev


On Thu, May 17, 2018 at 2:03 AM, Chandler Carruth via llvm-dev <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On Thu, May 17, 2018 at 10:32 AM Xinliang David Li <[hidden email]> wrote:


On Thu, May 17, 2018 at 2:03 AM, Chandler Carruth via llvm-dev <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
On Thu, May 17, 2018 at 9:27 AM Philip Reames <[hidden email]> 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 list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
On Thu, May 17, 2018 at 6:23 AM Krzysztof Parzyszek via llvm-dev <[hidden email]> 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
> [hidden email]
> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev


On Thu, May 17, 2018 at 1:24 PM, Chandler Carruth <[hidden email]> wrote:
On Thu, May 17, 2018 at 10:32 AM Xinliang David Li <[hidden email]> wrote:


On Thu, May 17, 2018 at 2:03 AM, Chandler Carruth via llvm-dev <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev




_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On Thu, May 17, 2018 at 1:33 PM Xinliang David Li via llvm-dev <[hidden email]> wrote:


On Thu, May 17, 2018 at 1:24 PM, Chandler Carruth <[hidden email]> wrote:
On Thu, May 17, 2018 at 10:32 AM Xinliang David Li <[hidden email]> wrote:


On Thu, May 17, 2018 at 2:03 AM, Chandler Carruth via llvm-dev <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev



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
[hidden email]
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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev


> On May 17, 2018, at 2:03 AM, Chandler Carruth via llvm-dev <[hidden email]> 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

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
On Fri, May 18, 2018 at 10:26 PM Chris Lattner via llvm-dev <[hidden email]> wrote:


> On May 17, 2018, at 2:03 AM, Chandler Carruth via llvm-dev <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev


On May 18, 2018, at 10:28 PM, Chandler Carruth <[hidden email]> wrote:

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

I agree that this isn’t the right first step, just wondering about the end game on this.  It seems reasonable to go this way (depending on how “indirect branches” and up going) because various aspects of a call (including its operand count) are fixed at creation time.  “terminator or not” seems like something that would/should similarly be fixed at construction time.

-Chris


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev


On May 17, 2018, at 19:37, Hal Finkel via llvm-dev <[hidden email]> 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
[hidden email]
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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
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 <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev