This is an archive of the discontinued LLVM Phabricator instance.

Correct Vectorcall Register passing and HVA Behavior
ClosedPublic

Authored by erichkeane on Dec 7 2016, 9:56 AM.

Details

Summary

This review is the front-end compatriot to this review: https://reviews.llvm.org/D27392

The vectorcall calling convention register assignment is subtly broken in two cases. First, it didn't properly handle homogeneous vector aggregates (HVAs). Second, the vectorcall specification requires that only the first 6 parameters be eligible for register assignment. This patch fixes both issues.

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane updated this revision to Diff 80620.Dec 7 2016, 9:56 AM
erichkeane retitled this revision from to Correct Vectorcall Register passing and HVA Behavior.
erichkeane updated this object.
erichkeane added a reviewer: cfe-commits.
majnemer added inline comments.Dec 7 2016, 12:13 PM
lib/CodeGen/TargetInfo.cpp
1688 ↗(On Diff #80620)

Formatting.

3799–3804 ↗(On Diff #80620)

Please consistently brace here.

3871 ↗(On Diff #80620)

I was OK with 6 showing up in prior code because there was a comment but there isn't such a comment down here. Maybe we should have this 6 live in an enum?

3871–3879 ↗(On Diff #80620)

And here,

erichkeane marked an inline comment as done.Dec 7 2016, 12:23 PM
erichkeane added inline comments.
lib/CodeGen/TargetInfo.cpp
1688 ↗(On Diff #80620)

I don't see what you mean? This area went through clang-format? What did I miss?

3871 ↗(On Diff #80620)

Thats a good point. Any reason it couldn't just be a static constant in each of the ABIInfos? Or am I missing your point?

majnemer added inline comments.Dec 7 2016, 12:46 PM
lib/CodeGen/TargetInfo.cpp
1688 ↗(On Diff #80620)

Your code has "if(Count < 6)", I'm incredibly surprised that clang-format would format your code like this. There should be a space after the if and before the left-parenthesis.

majnemer added inline comments.Dec 7 2016, 12:49 PM
lib/CodeGen/TargetInfo.cpp
3871 ↗(On Diff #80620)

It could be but LLVM usually prefers enums over class-scope static variables. They require an awkward definition somewhere should they be ODR used.

erichkeane added inline comments.Dec 7 2016, 12:49 PM
lib/CodeGen/TargetInfo.cpp
1688 ↗(On Diff #80620)

Ah, thanks! I do go through my patches and 'ctrl-K' for clang-format, so I don't know if I missed this section or what, but I've fixed it as a part of the other fixes here, and the re-clang-formatting took care of it. Thanks for the clarification!

rnk added inline comments.Dec 7 2016, 12:49 PM
lib/CodeGen/TargetInfo.cpp
3871 ↗(On Diff #80620)

I think some of us who have spent time implementing the C++ ABI rules around static const int data members feel that you should use an enumerator in an unnamed enum instead because the ABI rules are easier to follow. With static data members, you sometimes have to worry about providing a definition like so:

struct A { static const int kThing = 42; }; // in a .h
const int A::kThing; // in some .cc file, needed "sometimes"

Simpler:

struct A { enum : int { kThing = 42 }; };
erichkeane updated this revision to Diff 80641.Dec 7 2016, 12:50 PM

Fixing issues brought up by David.

erichkeane marked 9 inline comments as done.Dec 7 2016, 12:51 PM
erichkeane added inline comments.
lib/CodeGen/TargetInfo.cpp
3871 ↗(On Diff #80620)

Fair enough, thanks for the education guys!

erichkeane updated this revision to Diff 80913.Dec 9 2016, 10:04 AM
erichkeane marked an inline comment as done.

Based on the tests, I was able to track down the 2 locations that stopped working. I've updated them appropriately.

It seems though that this Template Decl Instantiation is the ONLY time that the attributes aren't just initialized immediately. This gets me wondering if I should just create a pair of functions for it specifically? It would likely be a little bit of re-work on InstatiateAttrs (perhaps separate the implementation to take the instantiateTemplateAttrribute 'function' as a parameter), but would that be preferential?

erichkeane added a comment.EditedDec 11 2016, 5:05 PM

FWIW, the LLVM side of this patch seems to have been committed: https://reviews.llvm.org/D27148

Gah, that one is regcall based, close enough in number i misread it. Sorry about that.

rnk edited edge metadata.Dec 27 2016, 11:08 AM

I think you uploaded the wrong patch on the last upload, the code looks like it's related to attribute((deprecated)) now?

erichkeane updated this revision to Diff 82887.Jan 3 2017, 7:50 AM
erichkeane edited edge metadata.

Sorry about that, I have no idea how I messed up the process, I must have thought this was a different window. I've updated it with the latest version of this diff that I have which seems to be the one I intended initially.

Additionally, I note that the back end one went in here: https://reviews.llvm.org/D27392

Thanks,
Erich

PS: I apologize for the delay, I've been out of internet-communication over the last 2 weeks!

rnk added inline comments.Jan 4 2017, 10:58 AM
include/clang/CodeGen/CGFunctionInfo.h
136 ↗(On Diff #82887)

Marking HVAs with inreg is an x86-specific convention. Let's move this down to TargetInfo.cpp and call it something like getDirectX86HVA or something.

139 ↗(On Diff #82887)

In the long run, I think ABIArgInfo::Direct is too overloaded and needs to be split. The implementation of a single direct argument in EmitFunctionPrologue is *174* lines. It would be much cleaner to make some new ABIArgInfo types instead of having these "can be flattened" flags and things.

lib/CodeGen/TargetInfo.cpp
1507 ↗(On Diff #82887)

You don't seem to do anything in this if block if !IsHva, so maybe fold it back into the initial check like it was, and then break out the case for regcall and vectorcall separately.

1678–1679 ↗(On Diff #82887)

Rewrap this. clang-format breaks comments without re-flowing, since it doesn't know what should flow together.

erichkeane marked 3 inline comments as done.Jan 4 2017, 11:30 AM

patch incoming

include/clang/CodeGen/CGFunctionInfo.h
139 ↗(On Diff #82887)

I think you're right, this was my first run-in with what "direct" means and it took a lot to figure out what it even MEANS. I definitely think that it is worth a refactoring in the future. I notice that a lot of the ABIArgInfo /ABIInfo related things could strongly benefit from a refactoring as well.

lib/CodeGen/TargetInfo.cpp
1507 ↗(On Diff #82887)

I did this same thing in 2 places, so I suspect I had SOME /*good*/ reason for splitting it out like this, but I cannot for the life of me remember it. Changed.

erichkeane updated this revision to Diff 83092.Jan 4 2017, 11:31 AM
erichkeane marked an inline comment as done.

Reid's comments

rnk added a comment.Jan 4 2017, 11:37 AM

Last thing, I promise.

lib/CodeGen/TargetInfo.cpp
1678 ↗(On Diff #83092)

Can we out the body of this if into a separate helper? The much more common case is to do one pass over the arguments, and this two-pass block obscures that.

3882 ↗(On Diff #83092)

Ditto, can we extract the two-pass code into a helper?

erichkeane marked 2 inline comments as done.Jan 4 2017, 11:45 AM

Good idea, building now!

erichkeane updated this revision to Diff 83116.Jan 4 2017, 1:22 PM

Refactor per @rnk s suggestion.

rnk accepted this revision.Jan 4 2017, 4:26 PM
rnk edited edge metadata.

lgtm, thanks!

This revision is now accepted and ready to land.Jan 4 2017, 4:26 PM
This revision was automatically updated to reflect the committed changes.