Remove remaining traces of AT&T assembly syntax #911
Conversation
compiler-builtins/src/mem/x86_64.rs
Outdated
| core::arch::asm!( | ||
| "repe movsb (%rsi), (%rdi)", | ||
| asm!( | ||
| "repe movsb [rdi], rsi", |
There was a problem hiding this comment.
In the original asm both rsi and rdi are dereferenced, here it’s only rdi. Is this still equivalent for some reason?
There was a problem hiding this comment.
I don't think it is. I fixed this.
| core::arch::asm!( | ||
| "repe movsb (%rsi), (%rdi)", | ||
| asm!( | ||
| "repe movsb [rdi], [rsi]", |
There was a problem hiding this comment.
| "repe movsb [rdi], [rsi]", | |
| "rep movsb [rdi], [rsi]", |
The repe mnemonic is for "repeat while equal" and only makes sense for the string comparison operations
https://www.felixcloutier.com/x86/rep:repe:repz:repne:repnz
(but the encoding is the same so I guess assemblers don't care)
| core::arch::asm!( | ||
| "repe stosb %al, (%rdi)", | ||
| asm!( | ||
| "repe stosb [rdi], al", |
There was a problem hiding this comment.
| "repe stosb [rdi], al", | |
| "rep stosb [rdi], al", |
| inout("rsi") src.add(count - 1) => _, | ||
| // We modify flags, but we restore it afterwards | ||
| options(att_syntax, nostack, preserves_flags) | ||
| options(nostack, preserves_flags) |
There was a problem hiding this comment.
This definitely shouldn't have preserves_flags. Flags are modified by each of add,sub,test. The comment refers to how std/cld are used to set/clear the direction flag, but that's separate since it must be cleared before exiting the inline assembly.
Actually, what is the point of the test instruction here? It only affects flags, and none of the subsequent instructions depend on any flags.
There was a problem hiding this comment.
This diff gave me a similar question about
compiler-builtins/compiler-builtins/src/x86_64.rs
Lines 20 to 26 in e1d8a49
test overwritten by the sub then cmp before ja checks them?
There was a problem hiding this comment.
Yeah, that looked weird at first, but I think the test there just used as a way to load from memory without modifying any registers (except flags). So the loop is just reading from memory at 4kB intervals for the side-effects only, which is exactly what a chkstk should be doing.
compiler-builtins/src/x86.rs
Outdated
| "test %ecx,(%ecx)", | ||
| "lea 4(%esp),%eax", // load pointer to the return address into eax | ||
| "mov %ecx,%esp", // install the new top of stack pointer into esp | ||
| "mov -4(%eax),%ecx", // restore ecx | ||
| "push (%eax)", // push return address onto the stack | ||
| "sub %esp,%eax", // restore the original value in eax | ||
| "3:", | ||
| "sub ecx, eax", | ||
| "test [ecx], ecx", | ||
| "lea eax, [esp + 4]", // load pointer to the return address into eax | ||
| "mov esp, ecx", // install the new top of stack pointer into esp | ||
| "mov ecx, [eax - 4]", // restore ecx | ||
| "push [eax]", // push return address onto the stack | ||
| "sub eax, esp", // restore the original value in eax |
There was a problem hiding this comment.
Nit: comments used to be aligned
|
@quaternic the issues you mentioned seem to be preexisting. Would you mind putting up a PR to fix them? We can do that independent of the syntax swap. Then @TDecking you won't have to make any changes aside from resolving conflicts. Would you be able to rebase? |
|
@tgross35 I can, but I'd much prefer doing that in intel syntax, so merging this with those preexisting issues would be fine. |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I've rebased and removed the final instances of |
tgross35
left a comment
tgross35
left a comment
There was a problem hiding this comment.
LGTM as a 1:1 change, thanks for the updates!
* `repe` is "repeat while equal", which only makes sense for string comparisons. Change it to `rep`. (The encoding is the same so there is no performance change.) * Remove an unneeded `test`. This was added in ae557bd ("Skip rep movsb in copy_backward if possible"). The `jz` was removed in ef37a23 ("Remove branches around rep movsb/stosb") but the `test` was missed. * Remove an incorrect `preserves_flags`; `add` and `sub` affect flags. Discussion: #911 Fixes: ef37a23 ("Remove branches around rep movsb/stosb") Fixes: c30322a ("Align destination in mem* instructions.") [ Added details to the commit message - Trevor ]
4 participants
The current minimum LLVM version seems to be 18, way larger than required for resolving this FIXME.