Calling-convention lowering proposal

Project Infrastructure LLVM Dev List Archives
1

Hello,

Attached is a patch which significantly reworks how calls, incoming
arguments, and outgoing return values are lowered. It's a major change,
affecting all targets, so I'm looking for feedback on the approach.

The goal of the patch is to eliminate a bunch of awkward code,
eliminate some unnecessary differences between targets, and to
facilitate future refactoring and feature work.

This patch gets rid of ISD::CALL, ISD::FORMAL_ARGUMENTS, ISD::RET, and
ISD::ISD::ARG_FLAGS, as well as the old LowerArguments and LowerCallTo
hooks. To replace them, it adds three new TargetLowering hooks:
LowerCall, LowerFormalArguments, and LowerReturn. These hooks provide
targets with the same information as the special nodes, except in an
immediately usable form instead of awkwardly encoded as SDNode
operands. The patch also re-works a substantial portion of the
target-independent tail-call code. The patch also includes changes
for all in-tree targets to use the new hooks.

Beyond dejagnu and some manual assembly output inspection,
so far I've only tested this on x86 targets. I'm not in a hurry
with this; I'm just looking for input at this point.

Dan

calling-conv-lowering.patch (289 KB)

2

Dan Gohman wrote:

Hello,

Attached is a patch which significantly reworks how calls, incoming
arguments, and outgoing return values are lowered. It's a major change,
affecting all targets, so I'm looking for feedback on the approach.

The goal of the patch is to eliminate a bunch of awkward code,
eliminate some unnecessary differences between targets, and to
facilitate future refactoring and feature work.

This patch gets rid of ISD::CALL, ISD::FORMAL_ARGUMENTS, ISD::RET, and
ISD::ISD::ARG_FLAGS, as well as the old LowerArguments and LowerCallTo
hooks. To replace them, it adds three new TargetLowering hooks:
LowerCall, LowerFormalArguments, and LowerReturn. These hooks provide
targets with the same information as the special nodes, except in an
immediately usable form instead of awkwardly encoded as SDNode
operands. The patch also re-works a substantial portion of the
target-independent tail-call code. The patch also includes changes
for all in-tree targets to use the new hooks.

Beyond dejagnu and some manual assembly output inspection,
so far I've only tested this on x86 targets. I'm not in a hurry
with this; I'm just looking for input at this point.

Dan

Hi Dan,
I quickly ran my tests with your changes and a few of them breaks llc while a few other have incorrect assembly. I am attaching the .bc files here. The llc options I use --pre-RA-sched=list-burr and -regalloc=pbqp

fun_in_expr3.bc (llc breaks)
struct_args_5.bc (incorrect assemly, execution fails)
char_char.bc (llc breaks)

I haven't really examined what is the reason of failures. Will do so as soon as I get some time off from rest of the stuff. Let me know if you need any specific inputs on pic16 port.

- Sanjiv

fun_in_expr3.bc (604 Bytes)

struct_args_5.bc (1.14 KB)

char_char.bc (620 Bytes)

3

Hi, Dan

The goal of the patch is to eliminate a bunch of awkward code,
eliminate some unnecessary differences between targets, and to
facilitate future refactoring and feature work.

I quickly looked over the patch and it seems to be a significant
cleanup of all really ugly lowering code!

Maybe it will be possible to provide some dummy implementation of
LowerFormalArguments / LowerReturn (pass everything on stack, etc), so
one willing to add new backend won't need to implement such complex
piece of code as a first step?

4

Attached is a patch which significantly reworks how calls, incoming
arguments, and outgoing return values are lowered. It's a major change,
affecting all targets, so I'm looking for feedback on the approach.

The goal of the patch is to eliminate a bunch of awkward code,
eliminate some unnecessary differences between targets, and to
facilitate future refactoring and feature work.

Nice!

This patch gets rid of ISD::CALL, ISD::FORMAL_ARGUMENTS, ISD::RET, and
ISD::ISD::ARG_FLAGS, as well as the old LowerArguments and LowerCallTo
hooks. To replace them, it adds three new TargetLowering hooks:
LowerCall, LowerFormalArguments, and LowerReturn. These hooks provide
targets with the same information as the special nodes, except in an
immediately usable form instead of awkwardly encoded as SDNode
operands. The patch also re-works a substantial portion of the
target-independent tail-call code. The patch also includes changes
for all in-tree targets to use the new hooks.

This is a great improvement! Most of my comments are general design thoughts, not specific to this one patch. I understand that this is one step in the right direction.

Do clients end up lowering the return information before they lower the argument information? If they do, it should be relatively straight-forward to support "spilling to the stack" return values that can't fit in physregs. It would be nice to eventually support arbitrary return values in the code generator. Doing this would basically just require return lowering to push an extra iPtr argument.

Looking at the patch, it seems odd that ISD::LIBCALL still exists. Is the "CSE of libcalls" optimization really important in practice? If so, perhaps this should be done as an explicit dag combine optimization: if two clumps of flagged together nodes are identical and have the same inputs, replace one clump with the other. This would be a nice general solution instead of something specific to calls.

Unrelated, but it would also be nice to make INLINEASM nodes have their own SDNode subclass someday instead of a bunch of magically indexed operands. Similar to Nate's recent shuffle change.

InputArg/OutputArg should probably contain an ArgFlagsTy instead of inherit from it.

I like the various -'s of random call lowering stuff from the .td files, this is great!

-Chris

5

Thanks for the testing and the testcases. PIC16 is certainly the most
complicated of the bunch :-}. I have fixes for each of these, and with
them I've verified that a patched llc is emitting the same output as an
unpatched llc, with one exception which I believe is ok. I'll post
details and an updated patch soon.

Dan

6

My goal is for the CallingConvLowering code to eventually take over
most, if not all, of the work for these hooks. Then, someone writing
a new target could just add a simple CallingConv that just does
CCAssignToStack for everything as a first step.

I don't know when that might happen, but with this patch it's a little
easier to think about.

Dan

7

Do clients end up lowering the return information before they lower
the argument information? If they do, it should be relatively
straight-forward to support "spilling to the stack" return values that
can't fit in physregs. It would be nice to eventually support
arbitrary return values in the code generator. Doing this would
basically just require return lowering to push an extra iPtr argument.

There are details, but yes, PR2660 will be easier to approach
with this patch :-).

Looking at the patch, it seems odd that ISD::LIBCALL still exists. Is
the "CSE of libcalls" optimization really important in practice? If
so, perhaps this should be done as an explicit dag combine
optimization: if two clumps of flagged together nodes are identical
and have the same inputs, replace one clump with the other. This
would be a nice general solution instead of something specific to calls.

It comes up in test/CodeGen/ARM/cse-libcalls.ll, at least. However,
that may be due to the particular way SELECT_CC is legalized. I'm
going to re-evaluate this.

Unrelated, but it would also be nice to make INLINEASM nodes have
their own SDNode subclass someday instead of a bunch of magically
indexed operands. Similar to Nate's recent shuffle change.

Patches welcome :-).

InputArg/OutputArg should probably contain an ArgFlagsTy instead of
inherit from it.

I think you're right. I'll make this change.

Thanks,

Dan

8

I don't have specific feedback on this patch but I do have feedback about how
we go about making these kinds of changing.

This patch changes the LLVM API. We should have a process for deprecating
obsolete interfaces before removing them entirely. It's a significant
maintenance headache to pull down a new release and fix all of the API
issues along with tracking down new bugs introduced.

I completely agree with the goals of this patch. However, I would ask that we
deprecate the existing interfaces for 2.6 and then remove them in 2.7. We
should add the new interfaces under a different name until 2.7 at which point
they will become default.

LLVM is getting significant 3rd-party use and we should figure out how to act
in a new way to better support our users.

                                                    -Dave

9

Attached is a patch which significantly reworks how calls, incoming
arguments, and outgoing return values are lowered. It's a major change,
affecting all targets, so I'm looking for feedback on the approach.

I don't have specific feedback on this patch but I do have feedback about how
we go about making these kinds of changing.

This patch changes the LLVM API. We should have a process for deprecating
obsolete interfaces before removing them entirely. It's a significant
maintenance headache to pull down a new release and fix all of the API
issues along with tracking down new bugs introduced.

I completely agree with the goals of this patch. However, I would ask that we
deprecate the existing interfaces for 2.6 and then remove them in 2.7. We
should add the new interfaces under a different name until 2.7 at which point
they will become default.

I don't really see how this is any different. Instead of changing the API in 2.6, it just gets postponed to 2.7. So with 2.7 you have to upgrade your API and track down new bugs introduced by 2.7 (hopefully not very many).

The majority of the people are not going to make the API changes until they are forced to. Its just low on the priority list. We've seen this from many people who don't upgrade because they don't want to make the API changes.

-Tanya

10

No, we make no attempt at being API compatible across revs of LLVM. This is a great way to encourage people to contribute their code to the project. If they want to live with their code out of tree, then they have to deal with API breakage.

-Chris

11

> I completely agree with the goals of this patch. However, I would ask
> that we deprecate the existing interfaces for 2.6 and then remove them in
> 2.7. We should add the new interfaces under a different name until 2.7
> at which point they will become default.

I don't really see how this is any different. Instead of changing the API
in 2.6, it just gets postponed to 2.7. So with 2.7 you have to upgrade
your API and track down new bugs introduced by 2.7 (hopefully not very
many).

The difference is that it isolates the API change from the other changes in
LLVM.

When we do an upgrade to a new LLVM release, we spend a significant amount
of time resolving conflicts and fixing new bugs. It's one less thing to have
to worry about if APIs remain stable (through deprecation).

What we would do is fix all of the bugs, etc. then afterward move our code
over to the new APIs. Then for the next release it's simply a matter of
renaming the API function we call.

The majority of the people are not going to make the API changes until
they are forced to. Its just low on the priority list. We've seen this
from many people who don't upgrade because they don't want to make the API
changes.

But you have to give people warning. That's what deprecation does. It means
people can upgrade and fix all of the resulting problems without having to
worry about changing APIs at the same time. They can then migrate to the new
API before upgrading to the next version that removes the deprecated API.

Deprecation doesn't solve the "people not wanting to upgrade" problem. That's
not the point. There's very little we can do to satisfy those people. What
deprecation does is help the people that DO want to upgrade to the new
API but would prefer an incremental way of doing it.

We talk about making incremental changes to the LLVM repository all the time.
Why would we not apply the same philosophy to releases?

Every open source project with a significant client base does things this way.
I believe LLVM has reached this point as well.

                                                 -Dave

12

> This patch changes the LLVM API. We should have a process for
> deprecating
> obsolete interfaces before removing them entirely. It's a significant
> maintenance headache to pull down a new release and fix all of the API
> issues along with tracking down new bugs introduced.

No, we make no attempt at being API compatible across revs of LLVM.

That's a huge mistake going forward if we want to grow the community.

This is a great way to encourage people to contribute their code to
the project. If they want to live with their code out of tree, then
they have to deal with API breakage.

It's not about people not contributing code. It's about 3rd party modules
that have to interface to LLVM. The LLVM community does not care about
the Cray optimizer code. But the Cray optimizer code must use LLVM APIs
to translate the Cray IR to LLVM IR.

If we can distinguish between "internal" and "external" LLVM APIs that's fine
but I suspect each person's definition of "internal" and "external" will vary
quite a bit because people interface to LLVM at very different points.

                                                   -Dave

13

This patch changes the LLVM API. We should have a process for
deprecating
obsolete interfaces before removing them entirely. It's a significant
maintenance headache to pull down a new release and fix all of the API
issues along with tracking down new bugs introduced.

No, we make no attempt at being API compatible across revs of LLVM.

That's a huge mistake going forward if we want to grow the community.

Perhaps. However, I greatly prefer to make life easier for people who contribute code (by preventing them from having to worry about deprecation etc) than for those who don't. Reducing the barrier to contributing code is a good way (IMO) to encourage contributions and new developers.

This is a great way to encourage people to contribute their code to
the project. If they want to live with their code out of tree, then
they have to deal with API breakage.

It's not about people not contributing code. It's about 3rd party modules
that have to interface to LLVM. The LLVM community does not care about
the Cray optimizer code. But the Cray optimizer code must use LLVM APIs
to translate the Cray IR to LLVM IR.

Understood, Apple also has external code that interfaces to LLVM as well, and it has to be updated as well. I still maintain that we should optimize for encouraging contributions. If you have out of tree code that depends on unstable APIs, you should expect to have to change it to work with new version of LLVM.

The only stable API we vend is the C interface bindings.

-Chris

14

Chris Lattner wrote:

15

Chris Lattner wrote:

This patch changes the LLVM API. We should have a process for
deprecating
obsolete interfaces before removing them entirely. It's a
significant
maintenance headache to pull down a new release and fix all of the
API
issues along with tracking down new bugs introduced.

No, we make no attempt at being API compatible across revs of LLVM.

That's a huge mistake going forward if we want to grow the community.

Perhaps. However, I greatly prefer to make life easier for people who
contribute code (by preventing them from having to worry about
deprecation etc) than for those who don't. Reducing the barrier to
contributing code is a good way (IMO) to encourage contributions and
new developers.

Perhaps, but it's a barrier for those who actually want to use LLVM's JIT.
We'd love to keep up with the latest versions of LLVM, but stuff just keeps
breaking and sometimes features are just deleted. For example, it used
to be possible easily to turn on and off asm dumping, but it isn't any more.

That's not true. It's just the option name was changed. The option was added initially to facilitate debugging, but people somehow treats it as a feature. That's fine. That just encourage users to get involved with the community.

Evan

16

Evan Cheng wrote:

Chris Lattner wrote:

This patch changes the LLVM API. We should have a process for
deprecating
obsolete interfaces before removing them entirely. It's a
significant
maintenance headache to pull down a new release and fix all of the
API
issues along with tracking down new bugs introduced.

No, we make no attempt at being API compatible across revs of LLVM.

That's a huge mistake going forward if we want to grow the
community.

Perhaps. However, I greatly prefer to make life easier for people
who
contribute code (by preventing them from having to worry about
deprecation etc) than for those who don't. Reducing the barrier to
contributing code is a good way (IMO) to encourage contributions and
new developers.

Perhaps, but it's a barrier for those who actually want to use
LLVM's JIT.

We'd love to keep up with the latest versions of LLVM, but stuff just keeps
breaking and sometimes features are just deleted. For example, it used
to be possible easily to turn on and off asm dumping, but it isn't
any more.

That's not true. It's just the option name was changed.

You can turn it on once, but you can never turn it off again.
That is, unless there is some way to do it other than by passing
command line options to cl::ParseCommandLineOptions. The last
time I looked the problem was that DebugOnly was defined as
cl::ValueRequired rather than cl::OneOrMore, so the CLI would
reject any subsequent setting.

If there is some way around this, I'd be delighted to know.

Andrew.

17

Evan Cheng wrote:

Chris Lattner wrote:

This patch changes the LLVM API. We should have a process for
deprecating
obsolete interfaces before removing them entirely. It's a
significant
maintenance headache to pull down a new release and fix all of the
API
issues along with tracking down new bugs introduced.

No, we make no attempt at being API compatible across revs of LLVM.

That's a huge mistake going forward if we want to grow the
community.

Perhaps. However, I greatly prefer to make life easier for people
who
contribute code (by preventing them from having to worry about
deprecation etc) than for those who don't. Reducing the barrier to
contributing code is a good way (IMO) to encourage contributions and
new developers.

Perhaps, but it's a barrier for those who actually want to use
LLVM's JIT.

We'd love to keep up with the latest versions of LLVM, but stuff just keeps
breaking and sometimes features are just deleted. For example, it used
to be possible easily to turn on and off asm dumping, but it isn't
any more.

That's not true. It's just the option name was changed.

You can turn it on once, but you can never turn it off again.

That's not different from before.

That is, unless there is some way to do it other than by passing
command line options to cl::ParseCommandLineOptions. The last
time I looked the problem was that DebugOnly was defined as
cl::ValueRequired rather than cl::OneOrMore, so the CLI would
reject any subsequent setting.

If there is some way around this, I'd be delighted to know.

This will require modifying code. Patches welcome.

Evan

18

Evan Cheng wrote:

Evan Cheng wrote:

We'd love to keep up with the latest versions of LLVM, but stuff
just keeps
breaking and sometimes features are just deleted. For example, it
used
to be possible easily to turn on and off asm dumping, but it isn't
any more.

That's not true. It's just the option name was changed.

You can turn it on once, but you can never turn it off again.

That's not different from before.

It certainly was. See
http://lists.cs.uiuc.edu/pipermail/llvmdev/2009-January/019557.html

That is, unless there is some way to do it other than by passing
command line options to cl::ParseCommandLineOptions. The last
time I looked the problem was that DebugOnly was defined as
cl::ValueRequired rather than cl::OneOrMore, so the CLI would
reject any subsequent setting.

If there is some way around this, I'd be delighted to know.

This will require modifying code. Patches welcome.

Oh sorry, I thought you'd already seen it. It's at

http://lists.cs.uiuc.edu/pipermail/llvmdev/2009-January/019661.html

Andrew.