Conversation
There was a problem hiding this comment.
When compiled with opt-level=1 or higher, these produce nearly identical asm on linux with minor changes to register choices loading HAVE_LSE_ATOMICS.
TODO: figure out why tests now need
--features unmangled-namesto build now.
naked functions are special and always get a public module https://github.com/pmur/compiler-builtins/blob/6ad91ea6dbae4f2d0ce36989e0a2874c43d8f025/compiler-builtins/src/macros.rs#L378-L401. I think you could fix tests by removing one ::$name from this line
compiler-builtins/builtins-test/tests/lse.rs
Line 135 in 9e64861
Questions: Do the test suites also run with opt-level=1? This should work at 0 (it did for my testing).
We run with a few options, unoptimized and opt-level=3 should both be covered. Any reason for opt-level=1 specifically?
Lines 67 to 73 in 9e64861
| "1:", | ||
| "ret", | ||
| have_lse = sym crate::aarch64_outline_atomics::HAVE_LSE_ATOMICS, | ||
| let mut expected = expected; |
There was a problem hiding this comment.
The param can just be made mut expected: int_ty!($bytes) (applies a few places)
There was a problem hiding this comment.
That currently runs afoul of intrinsics!. I am not sure how (or if) I can teach it how to accept a mut arg.
6ad91ea to
b470ee2
Compare
No, only that it is where the codegen becomes comparable to the previous implementation. Thanks for the incredible fast code review 😮 . |
These functions should follow aapcs, so a strictly asm version shouldn't be needed. This resolves some of the ugliness of reading HAVE_LSE_ATOMICS and enabling bti on various targets. When compiled with opt-level=1 or higher, these produce nearly identical asm on linux with minor changes to register choices loading HAVE_LSE_ATOMICS.
b470ee2 to
120cac7
Compare
|
Attaching a diff of the naked asm vs native asm implementation with optimization enabled. It's ~1300 lines like: - 0: 90000010 adrp x16, 0 <compiler_builtins::aarch64_outline_atomics::HAVE_LSE_ATOMICS>
- 4: 39400210 ldrb w16, [x16]
- 8: 34000070 cbz w16, 14 <__aarch64_cas16_acq_rel+0x14>
+ 0: 90000008 adrp x8, 0 <compiler_builtins::aarch64_outline_atomics::HAVE_LSE_ATOMICS>
+ 4: 39400108 ldrb w8, [x8]
+ 8: 34000068 cbz w8, 14 <__aarch64_cas16_acq_rel+0x14>
c: 4860fc82 caspal x0, x1, x2, x3, [x4]
10: d65f03c0 ret
14: aa0003f0 mov x16, x0Notes about methodology: Edit: Fix diff old/new. |
These functions should follow aapcs, so a strictly asm version shouldn't be needed. This resolves some of the ugliness of reading HAVE_LSE_ATOMICS and enabling bti on various targets.
When compiled with opt-level=1 or higher, these produce nearly identical asm on linux with minor changes to register choices loading HAVE_LSE_ATOMICS.
Some minor macro changes are made to ensure existing lse tests continue to build and run.TODO: collate and post asm differences. They should be minor.TODO: figure out why tests now need--features unmangled-namesto build now.Questions: Do the test suites also run with opt-level=1? This should work at 0 (it did for my testing).