-
Notifications
You must be signed in to change notification settings - Fork 34
[ATfE] Enable v7-A and v7-R variants for LLVM-libc tests #486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| // The table may also be relocated, so we make it position-independent by | ||
| // having a table of handler addresses and loading the address to pc. | ||
| __attribute__((naked, section(".vectors"), aligned(32))) void vector_table() { | ||
| [[gnu::section(".vectors"), gnu::aligned(32), gnu::used, gnu::naked, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change in syntax here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly most of the code should use this [[c++11]] type syntax. It's more modern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is C++ syntax then, not C.
I'm not against it, but we must be sure that this will be compatible with all configurations, especially with a C-only compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this attribute syntax has been backported, at least on Clang. Also, this library is compiled on our systems, and this attribute will not affect user code at all, so as long as we don't use very old versions that are unsupported by Clang, we should be OK.
https://stackoverflow.com/questions/74574618/old-style-vs-c11-attribute-syntax
| // Size is more important than speed here so don't unroll the loop. | ||
| _Pragma("clang loop unroll(disable) vectorize(disable)") for ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, memcpy itself has unrolled loops, so I think you shouldn't worry about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to prevent an internal LLVM-libc issue (see line 38)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean the pragma to prevent loop unrolling. Since even memcpy itself uses unrolling, having this pragma here is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see what you mean. Here I'm removing the call to memcpy and using a manual copy.
Support added in llvm/llvm-project#153576
Also update bootcode to support v7-A/v7-R outside of hermetic tests.