Page MenuHomePhabricator

[X86] Support of no_caller_saved_registers attribute (Clang part)
AbandonedPublic

Authored by aaboud on Jul 6 2016, 7:24 AM.

Details

Summary

This patch implements the Clang part for no_caller_saved_registers attribute as appear in interrupt and exception handler proposal

Diff Detail

Event Timeline

aaboud updated this revision to Diff 62861.Jul 6 2016, 7:24 AM
aaboud retitled this revision from to [X86] Support of no_caller_saved_registers attribute (Clang part).
aaboud updated this object.
aaboud added a subscriber: cfe-commits.
aaron.ballman added inline comments.Jul 11 2016, 1:18 PM
include/clang/Basic/Attr.td
1657

Any particular reason this isn't (also) a C++-style attribute under the clang namespace?

include/clang/Basic/AttrDocs.td
2177

How does this interact with calling convention attributes? For instance, if I have a function that is explicitly marked cdecl and has this attribute, what is the behavior for EAX, ECX, and EDX?

Also, why is this an attribute rather than a calling convention? I'm wondering how this works through function pointers, for instance (whether it is intended to work or not).

test/SemaCXX/attr-x86-no_caller_saved_registers.cpp
15

Should also have a sema test that this attribute accepts no arguments.

aaboud marked an inline comment as done.Jul 24 2016, 4:46 AM
aaboud added inline comments.
include/clang/Basic/Attr.td
1657

I am not sure about that, I was just following the way the "interrupt" attribute was defined.
Do you think we should change both to consider C++ style attribute as well?

include/clang/Basic/AttrDocs.td
2177

Please, correct me if I am mistaken, but calling convention determine not only what the callee and the caller function saved registers are, but also how we move parameters to the called function, right?

The idea behind no_caller_saved_registers attribute, is that it can be combined with any other calling convention, and it only override the callee/caller saved register list, but not the way parameters are passed to the called function.

So, using a calling convention for this attribute will not be right.

H.J., can you confirm this?

aaboud updated this revision to Diff 65269.Jul 24 2016, 4:56 AM

Updated test according to comments.

hjl.tools edited edge metadata.Jul 25 2016, 8:15 AM

no_caller_saved_registers attribute can be used with any calling conventions.

The example Aaron sent in email is a good one:

void func(int, int, int, int) __attribute__((no_caller_saved_registers, cdecl));

int main() {
  void (*fp)(int, int, int, int) __attribute__((cdecl)) = func;
  func(1, 2, 3, 4);
  fp(1, 2, 3, 4); // Not the same as the above call to func()?
}

Clang should at least be giving a warning for this just like it does if you try to mix cdecl and stdcall on IA-32 or if you mix regparm(2) and regparm(3).

The current patch silently accepts the mismatch.

aaboud updated this revision to Diff 66810.Aug 4 2016, 8:46 AM
aaboud edited edge metadata.

Made "no_caller_saved_registers" part of function prototype.
This will allow Clang to yell a warning when there is a mismatch between function pointer and assigned function value.

Thanks to Erich for implementing this addition change.

aaron.ballman added inline comments.Aug 4 2016, 10:01 AM
include/clang/Basic/Attr.td
1657

Yes, though interrupt should be handled in a separate patch since it's a logically separate change.

include/clang/Basic/AttrDocs.td
2177

Please, correct me if I am mistaken, but calling convention determine not only what the callee and the caller function saved registers are, but also how we move parameters to the called function, right?

Correct.

The idea behind no_caller_saved_registers attribute, is that it can be combined with any other calling convention, and it only override the callee/caller saved register list, but not the way parameters are passed to the called function.

So, using a calling convention for this attribute will not be right.

Okay, that makes sense to me, but we should document how this interacts with calling convention attributes more clearly. Specifically, we should call out that this does not affect the calling convention and give an example of what gets generated for a function that combines a caller-saved calling convention with this attribute.

include/clang/CodeGen/CGFunctionInfo.h
479 ↗(On Diff #66810)

This is unfortunate as it will bump the bit-field length to 33 bits, which seems rather wasteful. Are there any bits we can steal to bring this back down to a 32-bit bit-field?

erichkeane edited edge metadata.Aug 4 2016, 10:40 AM

Response on CGFucntionInfo.

include/clang/CodeGen/CGFunctionInfo.h
479 ↗(On Diff #66810)

I implemented this additional patch, but don't really know a TON about this area, so I have a few ideas but would like to get direction on it if possible. I had the following ideas of where to get a few more bits:

CallingConvention/EffectiveCallingConvention/ASTCallingConvention: This structure stores a pre-converted calling convention to the llvm::CallingConv enum (llvm/include/llvm/IR/CallingConv.h). I'll note that the legal values for this go up to 1023 (so 8 bits isn't enough anyway!), though only up to 91 are currently used.

HOWEVER, the clang CallingConv (include/clang/Basic/Specifiers.h :233) only has 16 items in it. If we were instead to store THAT and convert upon access (a simple switch statement, already used constructing this value, see ClangCallConvToLLVMCallConv), we could get away with 6 or 7 bits each, saving this 3-6 bits total, yet have way more than enough room for expansion.

HasRegParm: This field might be possible to eliminate. According to the GCC RegParm docs (I don't see a clang one?) here (https://gcc.gnu.org/onlinedocs/gcc/x86-Function-Attributes.html), ", the regparm attribute causes the compiler to pass arguments number one to number if they are of integral type in registers EAX, EDX, and ECX instead of on the stack".

It seems that 0 for "RegParm" should be illegal, so I wonder if we can treat "RegParm==0" as "HasRegParm==false" and eliminate storing that.

In my opinion, the 1st one gives us way more room for the future and corrects a possible future bug. The 2nd is likely a lower touch, though it could possibly change behavior (see discussion here https://www.coreboot.org/pipermail/coreboot/2008-October/040406.html) as regparm(0) is seemingly accepted by both compilers. I DO note that this comment notes that 'regparm 0' is the default behavior, so I'm not sure what change that would do.

Either way, I suspect this change should be a separate commit (though I would figure making it a pre-req for this patch would be the right way to go). If you give some guidance as to which you think would be better, I can put a patch together.

-Erich

aaron.ballman added inline comments.Aug 4 2016, 11:35 AM
include/clang/CodeGen/CGFunctionInfo.h
479 ↗(On Diff #66810)

I think that unsigned ASTCallingConvention : 8; can be safely reduced. This tracks a clang::CallingConv value, the maximum of which is 15. So I think the way to do this is to reduce ASTCallingConvention from 8 to 7 bits and then pack yours in as well.

Response on CGFucntionInfo.

include/clang/CodeGen/CGFunctionInfo.h
479 ↗(On Diff #66810)

Ah! I missed that this was the case. That said, it could likely be reduced to 6 if we really wished (currently 16 items, 6 gives us room for 32). Perhaps something to keep in our pocket for the next time someone needs a bit or two here.

I'll update the diff for Amjad if possible.

aaron.ballman added inline comments.Aug 4 2016, 11:48 AM
include/clang/CodeGen/CGFunctionInfo.h
479 ↗(On Diff #66810)

I'm on the fence about 6 vs 7 and see no harm in reducing it to either value, but have a *very* slight preference for 7 so that the bit-field grouping continues to add up to 32-bits. However, it's your call.

majnemer added inline comments.
include/clang/CodeGen/CGFunctionInfo.h
479 ↗(On Diff #66810)

I'd go with 6 and have another bitfield, unsigned UnusedBits : 1;

We use this paradigm elsewhere.

erichkeane added inline comments.Aug 4 2016, 2:46 PM
include/clang/CodeGen/CGFunctionInfo.h
479 ↗(On Diff #66810)

That seems like a solid solution. I'll add it to another diff and give it to Amjad to update this patch. Not sure if he has the ability to (or if it is too late), but I don't have permissions to alter this commit for him.

joerg added a subscriber: joerg.Aug 5 2016, 9:15 AM

For what it is worth, this certainly seems to be misnamed. By nature, if it doesn't preserve at least the stack pointer, there is no way to recover on return, right?

aaboud marked 3 inline comments as done.Aug 7 2016, 4:08 AM

For what it is worth, this certainly seems to be misnamed. By nature, if it doesn't preserve at least the stack pointer, there is no way to recover on return, right?

I am not sure I understand what you mean!
When you said "if it does not preserve the stack pointer", did you mean the "caller"?

Maybe it need more clarification on that, but stack pointer should be saved by the caller (via the call instruction), but even in this case the callee must preserve the stack pointer, right? otherwise the return will not work. So, from callee point of view it still cannot assume that any register is a scratch register and it need to preserve it before modifying.

aaboud marked an inline comment as not done.Aug 7 2016, 4:19 AM
aaboud added inline comments.
include/clang/Basic/Attr.td
1657

How about using GCC Spelling instead of GNU and C++?

  1. These attributes are compatible with GCC.
  2. This is what is said about GCC class:
// The GCC spelling implies GNU<name, "GNU"> and CXX11<"gnu", name> and also
// sets KnownToGCC to 1. This spelling should be used for any GCC-compatible
// attributes.
class GCC<string name> : Spelling<name, "GCC"> {
  let KnownToGCC = 1;
}
aaron.ballman added inline comments.Aug 7 2016, 6:40 AM
include/clang/Basic/Attr.td
1657

The GCC spelling should only be used if the attribute is also supported by GCC, which this one is not. So the spelling should be GNU<"no_caller_saved_registers">, and CXX11<"clang", "no_caller_saved_registers">.

For what it is worth, this certainly seems to be misnamed. By nature, if it doesn't preserve at least the stack pointer, there is no way to recover on return, right?

This is the best name we came up with and has been implemented in GCC.

aaron.ballman edited edge metadata.Aug 8 2016, 8:39 AM

For what it is worth, this certainly seems to be misnamed. By nature, if it doesn't preserve at least the stack pointer, there is no way to recover on return, right?

This is the best name we came up with and has been implemented in GCC.

What version of GCC supports this attribute? I tried 6.1.0 and it is unknown there.

For what it is worth, this certainly seems to be misnamed. By nature, if it doesn't preserve at least the stack pointer, there is no way to recover on return, right?

This is the best name we came up with and has been implemented in GCC.

What version of GCC supports this attribute? I tried 6.1.0 and it is unknown there.

I see reference to it in this bug against 6.0: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68661

For what it is worth, this certainly seems to be misnamed. By nature, if it doesn't preserve at least the stack pointer, there is no way to recover on return, right?

This is the best name we came up with and has been implemented in GCC.

What version of GCC supports this attribute? I tried 6.1.0 and it is unknown there.

It is implemented in GCC 7.

ABataev added inline comments.Oct 17 2016, 11:38 AM
include/clang/AST/Type.h
2934 ↗(On Diff #66810)

s/noCallerSavedRegs/NoCallerSavedRegs/g

2987 ↗(On Diff #66810)

s/noCallerSavedRegs/NoCallerSavedRegs/g

2990–2991 ↗(On Diff #66810)

Remove else, just return

lib/Sema/SemaDecl.cpp
2885 ↗(On Diff #66810)

Add a comment with the name of parameter for true argument

lib/Sema/SemaDeclAttr.cpp
1693

attr does follow rules of LLVM Coding standard. Must be Attr

1701

s/attr/Attr/g

1701

s/CheckNoCallerSavedRegsAttr/checkNoCallerSavedRegsAttr/g

ABataev resigned from this revision.Feb 13 2017, 11:44 AM

There appears to be two reviews out for this same functionality. You should probably close one of the reviews (but still address the comments from it).

aaboud abandoned this revision.Apr 12 2017, 4:38 AM
aaboud marked an inline comment as not done.

Changes in this patch are being reviewed in a new patch D31871.

MatzeB added a subscriber: MatzeB.Apr 27 2017, 10:03 AM

Just out of interested: I can see how __attribute__ ((interrupt)) is useful, but in what situations would you use no_caller_saved_registers?

Just out of interested: I can see how __attribute__ ((interrupt)) is useful, but in what situations would you use no_caller_saved_registers?

Actually please answer this question by documenting the attribute in docs/LangRef.rst in the llvm part of the changes.