Page MenuHomePhabricator

Swift Calling Convention: add swifterror attribute
ClosedPublic

Authored by manmanren on Mar 11 2016, 11:14 AM.

Details

Summary

This attribute can be applied to a single parameter of a function or to an AllocInst.
This patch can be applied on top of the patch in reviews.llvm.org/D17866

Diff Detail

Repository
rL LLVM

Event Timeline

manmanren updated this revision to Diff 50451.Mar 11 2016, 11:14 AM
manmanren retitled this revision from to Swift Calling Convention: add swifterror attribute.
manmanren updated this object.
manmanren changed the visibility from "Public (No Login Required)" to "manmanren (Manman Ren)".
manmanren updated this object.Mar 11 2016, 11:17 AM
manmanren added reviewers: lhames, rnk, rengolin.
manmanren changed the visibility from "manmanren (Manman Ren)" to "Public (No Login Required)".
manmanren added subscribers: rjmccall, llvm-commits.
rengolin edited edge metadata.Mar 16 2016, 4:28 AM
rengolin added a subscriber: rengolin.

I was expecting a bit more tests for this, including adding the
attribute to multiple positions in the argument list, or to multiple
types, and make sure only the valid types / positions are accepted.

I was expecting a bit more tests for this, including adding the
attribute to multiple positions in the argument list, or to multiple
types, and make sure only the valid types / positions are accepted.

Yes, I will add more tests.

Cheers,
Manman

manmanren updated this revision to Diff 51492.Mar 23 2016, 4:39 PM
manmanren edited edge metadata.

Addressing review comments by adding more tests.

Cheers,
Manman

mehdi_amini added inline comments.Mar 29 2016, 10:44 AM
include/llvm/Target/TargetCallingConv.h
92 ↗(On Diff #51492)

Flags |= ...

test/Bitcode/swifterror.ll
8 ↗(On Diff #51492)

I think we could have a test for the alloca at the call site, and a function that takes a SwiftError in another position than the first parameter.

manmanren updated this revision to Diff 51959.Mar 29 2016, 11:34 AM

Addressing review comments from Mehdi.

Something missing in this patch is a "motivation". It is not clear to me *why* we want this attribute. I'm assuming it will be used to do something smart later?

The convention allows a pointer-sized variable to be passed by reference more efficiently than simply passing a pointer to memory, as long as the variable doesn't actually need to be addressable.

OK, but to be sure I understand correctly: the "optimization" is not part of this patch right? I only see plumbing the attribute to the codegen but no use of it.
Might worth mentioning your one sentence description in the attribute description?
Also, described as you just did, I wonder if the name couldn't have been more generic than "swifterror" (not that I have a great suggestion to express what you just described though...).

In D18092#386031, @joker.eph wrote:

OK, but to be sure I understand correctly: the "optimization" is not part of this patch right? I only see plumbing the attribute to the codegen but no use of it.
Might worth mentioning your one sentence description in the attribute description?
Also, described as you just did, I wonder if the name couldn't have been more generic than "swifterror" (not that I have a great suggestion to express what you just described though...).

Patches for the swift error attribute are separated into 3 smaller patches. This is the first patch in the series: the second patch is in review as well: http://reviews.llvm.org/D18108. I am going to upload the third patch soon. The second patch handles the target-independent portion of the optimization, and the third patch will update the targets (AArch64, X86 and ARM).

Some of the discussions can be found here:
RFC: Implementing the Swift calling convention in LLVM and Clang
https://groups.google.com/forum/#!topic/llvm-dev/epDd2w93kZ0

In D18092#386031, @joker.eph wrote:

OK, but to be sure I understand correctly: the "optimization" is not part of this patch right? I only see plumbing the attribute to the codegen but no use of it.

Correct. Actually placing the variable in a register at the point of call requires specific backend support that's been separated.

Might worth mentioning your one sentence description in the attribute description?

Seems like a reasonable request to me. Manman?

Also, described as you just did, I wonder if the name couldn't have been more generic than "swifterror" (not that I have a great suggestion to express what you just described though...).

I would be fine with the feature having a more generic name.

Thanks for the pointers, I didn't have the background, will look...

Thanks Mehdi for reviewing!

Manman

docs/LangRef.rst
1070 ↗(On Diff #51959)

Loading and storing from the parameter will be optimized to reading and writing a specific register in backends that support swifterror.

A `swifterror argument must be produced by an alloca instruction tagged with the swifterror` keyword. Accessing the swifterror value in the caller will be optimized as well in backends that support swifterror.

rjmccall added inline comments.Mar 31 2016, 8:45 PM
lib/IR/Verifier.cpp
2493 ↗(On Diff #51959)

Should this be asserting that SwiftErrorArg is an alloca?

rjmccall added inline comments.Apr 1 2016, 9:20 AM
lib/IR/Verifier.cpp
1415 ↗(On Diff #51959)

You probably don't need to be this explicit; just "with pointer to pointer type" would be fine.

1909 ↗(On Diff #51959)

For StoreInst, you should check that it's the second operand.

You might also want to check that the operation isn't atomic or volatile.

And you should impose a similar restriction on swifterror allocas, I think, although there you also want to permit it to be passed as a swifterror argument. (Maybe that should be allowed here, too?)

mehdi_amini added inline comments.Apr 1 2016, 11:15 AM
docs/LangRef.rst
1067 ↗(On Diff #51959)

I'm not sure the first sentence is correct: the attribute can only apply to pointers but it does *indicate* that the parameter is a pointer.

1070 ↗(On Diff #51959)

Thanks to our nice whiteboard session earlier today and the pointer to the discussion, I have a much better understanding of what is going on :)

Things that may be useful to clarify (may enable further optimization in the middle end) is the aliasing part: the pointee is always an alloca and is only reachable from this pointer (can't alias with anything else in the callee). What about the error allocation itself (i.e. the pointee pointee)?

Also the text starts with "parameter" and does not mention the alloca.

I don't have a better name that would be more generic and carry the desired semantic (I thought about splitting it in multiple attributes to carry the various bits, but it may be overkill).

For what I'd like to see in the description, I'd imagine something along the line of:

This attribute is motivated to model and optimize Swift error handling. It can be attached to either a pointer-sized alloca, or a pointer-to-pointer function parameter. At call site, the actual argument that corresponds to a `swifterror` parameter has to be a `swifterror` alloca. There is at most one `swifterror` parameter in the list of function parameter. The `swifterror` attribute is part of the ABI for the function: it is undefined behavior to call a function from another translation unit with a declaration that omit the attribute. The backend calling convention for a function with a `swifterror` parameter can be implemented as an "in-out" register. To allow this optimization, it is illegal to use a `swifterror` value other than for a direct load or store (no GEP) or as a `swifterror` argument.
A `swifferror` pointer cannot alias anything, so is the pointee. (<- FIXME)

(note my English can probably benefit from some wordsmith)

manmanren updated this revision to Diff 52413.Apr 1 2016, 12:46 PM
manmanren marked 4 inline comments as done.Apr 1 2016, 12:51 PM

Upload new patch to address review comments.

Cheers,
Manman

docs/LangRef.rst
1070 ↗(On Diff #52413)

@Mehdi
I am not sure about adding the calling convention part here. Any attribute that is related to a calling convention should be part of the ABI, right?
Let me know what you think about the new description :]

@John, do you have any comments on the alias part and this description in general?

rjmccall added inline comments.Apr 1 2016, 1:19 PM
docs/LangRef.rst
1077 ↗(On Diff #52413)

I would say:

These constraints allow the calling convention to optimize access to swifterror
variables by associating them with a specific register at call boundaries rather than
placing them in memory. Since this does change the calling convention, a function
which uses the swifterror attribute on a parameter is not ABI-compatible with one
which does not.

1082 ↗(On Diff #52413)

I would say:

These constraints also allow LLVM to assume that a swifterror argument
does not alias any other memory visible within a function and that a swifterror
alloca passed as an argument does not escape.

mehdi_amini added inline comments.Apr 1 2016, 1:32 PM
docs/LangRef.rst
1077 ↗(On Diff #52413)

LGTM.

Thanks you're much better at putting words together that I am :)

1082 ↗(On Diff #52413)

It seems to me that this address the swift-error "fake pointer", but not the error itself. Can we (Do we want) to provide guarantee on the error object as well?

rjmccall added inline comments.Apr 1 2016, 1:38 PM
docs/LangRef.rst
1082 ↗(On Diff #52413)

No. To the extent that it's even true that the error object doesn't alias anything, it would be better to express that with other IR tools.

mehdi_amini accepted this revision.Apr 1 2016, 2:05 PM
mehdi_amini added a reviewer: mehdi_amini.

Manman: LGTM provided you add John's suggestions (no need to go through a new round of pre-commit review IMO).

docs/LangRef.rst
1082 ↗(On Diff #52413)

LGTM :)

This revision is now accepted and ready to land.Apr 1 2016, 2:05 PM
This revision was automatically updated to reflect the committed changes.