Page MenuHomePhabricator

Add attributes for preserve_mostcc/preserve_allcc calling conventions to the C/C++ front-end
ClosedPublic

Authored by swiftix on Mar 9 2016, 7:25 PM.

Details

Summary

Till now, preserve_mostcc/preserve_allcc calling convention attributes were only available at the LLVM IR level. This patch adds attributes for preserve_mostcc/preserve_allcc calling conventions to the C/C++ front-end.

The code was mostly written by Juergen Ributzka. I just added support for the AArch64 target and tests.

Diff Detail

Repository
rL LLVM

Event Timeline

swiftix updated this revision to Diff 50228.Mar 9 2016, 7:25 PM
swiftix retitled this revision from to Add attributes for preserve_mostcc/preserve_allcc calling conventions to the C/C++ front-end.
swiftix updated this object.
swiftix added reviewers: ributzka, aaron.ballman.
swiftix added a subscriber: cfe-commits.

Hi Aaron, Hi Juergen,

Could you review this simple patch? Is it OK to merge or are there any changes to be made?

Thanks,

Roman
aaron.ballman edited edge metadata.Mar 11 2016, 2:54 PM

Could you review this simple patch? Is it OK to merge or are there any changes to be made?

The usual rule of thumb is to wait about a week before pinging a review -- sometimes the backlogs for reviews are kind of long. Also, all of the previous comments on the review appear to have gone missing, which makes the review kind of tough. It is possible to add cfe-commits to the subscribers list directly instead of starting an entirely new review (it's under the Action drop-down where you can add subscribers as well as reviewers).

include/clang/Basic/Attr.td
1394 ↗(On Diff #50228)

Do these attributes do anything on targets other than AArch64 and x86-64? If not, these should probably be inheriting from TargetSpecificAttr. (With tests confirming the behavior on other targets.)

include/clang/Basic/AttrDocs.td
2137 ↗(On Diff #50228)

This should go under DocCatCallingConvs instead of DocCatVariable (especially since calling conventions apply to functions, not variables, generally speaking).

2165 ↗(On Diff #50228)

Objective-C (here and elsewhere)?

2176 ↗(On Diff #50228)

Same category issue as above.

test/CodeGen/preserve_most.c
1 ↗(On Diff #50228)

I would combine these tests with preserve_all.c and just have one file checking both attributes (since they're highly related).

test/Sema/preserve_most-call-conv.c
1 ↗(On Diff #50228)

Same suggestion here as above.

Thanks for the hint about adding subscribers, because I was not aware it is possible in the Web-GUI. That was the reason why a new review was created.

And thanks for the comments. I'll address your comments and upload the new patch for review.

swiftix added inline comments.Mar 11 2016, 5:41 PM
include/clang/Basic/Attr.td
1394 ↗(On Diff #50228)

These attributes are not target-specific, IMHO, and they would make sense for other targets as well. But they are currently implemented only for AArch64 and x86-64.

swiftix updated this revision to Diff 50508.Mar 11 2016, 5:57 PM
swiftix edited edge metadata.

This patch revision addresses all issues mentioned by reviewers:

  • DocCatCallingConvs is used instead of DocCatVariable.
  • ObjectiveC is replaced by Objective-C in the docs.
  • Tests for preserve_most and preserve_all are combined into one file.
aaron.ballman accepted this revision.Mar 14 2016, 10:50 AM
aaron.ballman edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Mar 14 2016, 10:50 AM
This revision was automatically updated to reflect the committed changes.