Skip to content
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

-march=i386 should not generate bswap/cmpxchg/xadd instructions #58470

Open
manxorist opened this issue Oct 19, 2022 · 7 comments
Open

-march=i386 should not generate bswap/cmpxchg/xadd instructions #58470

manxorist opened this issue Oct 19, 2022 · 7 comments

Comments

@manxorist
Copy link

manxorist commented Oct 19, 2022

While investigating a totally unrelated problem, I ran across Clang generating a BSWAP instruction with -march=i386. BSWAP is a 486 instruction (see https://www.felixcloutier.com/x86/bswap) and should thus not be available when targeting 386.

Clang generates BSWAP for both, a call to __builtin_bswap32, and as an optimization of a pure C implementation.

// -m32 -march=i386 -O3 -Wall -Wextra -Wpedantic

int foo(int x) {
    return __builtin_bswap32(x);
}

unsigned int foo2(unsigned int x) {
    return 0u
        | (x & 0x000000ffu) << 24
        | (x & 0x0000ff00u) << 8
        | (x & 0x00ff0000u) >> 8
        | (x & 0xff000000u) >> 24
        ;
}

(https://godbolt.org/z/8zhK7GnsM)

Now, I am not actually sure if Clang even wants to be able to target 386-class CPUs at all (I was not able to find any documentation indicating a positive or negative answer to that), however, it does accept -march=i386 without any warning or error. IMHO, it should either generate proper instructions honoring the given target, or just fail accepting the target CPU in the first place.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 19, 2022

@llvm/issue-subscribers-backend-x86

@phoebewang
Copy link
Contributor

I think we don't divide enough in legacy instructions. One of the reason I guess is we don't have corresponding bit in CPUID as well. It doesn't look much value to me to do such things for a rather old product.

@manxorist
Copy link
Author

manxorist commented Oct 19, 2022

It's not exactly CPUID, but there is a documented way to check for 486: Check if you can toggle AC in EFLAGS.

For the 486 to Pentium differences (CMPXCHG8B (https://www.felixcloutier.com/x86/cmpxchg8b:cmpxchg16b)), Clang actually does the right thing:

#include <stdatomic.h>

_Bool cmpxchg8b(
    volatile _Atomic unsigned long long * obj,
    unsigned long long * expected,
    unsigned long long desired)
{
    return atomic_compare_exchange_strong(obj, expected, desired);
}

(https://godbolt.org/z/PnM1hexav)

So, there is some existing work for supporting very old CPUs.

However, for 486 CMPXCHG (https://www.felixcloutier.com/x86/cmpxchg), it's wrong again:

#include <stdatomic.h>

_Bool cmpxchg(
    volatile _Atomic unsigned long * obj,
    unsigned long * expected,
    unsigned long desired)
{
    return atomic_compare_exchange_strong(obj, expected, desired);
}

(https://godbolt.org/z/ffGMo9596)

Also, 486 XADD (https://www.felixcloutier.com/x86/xadd):

#include <stdatomic.h>

unsigned long cmpxchg(
    volatile _Atomic unsigned long * obj,
    unsigned long arg)
{
    return atomic_fetch_add(obj, arg);
}

(https://godbolt.org/z/Tav5hGd9Y)

The only other 486 instructions (INVD, WBINVD, INVLPG) are likely not something that Clang would generate by itself.

So, it looks like it's really just i386 for 3 instructions that might need some work.

@manxorist manxorist changed the title -march=i386 should not generate bswap instruction -march=i386 should not generate bswap/cmpxchg/xadd instructions Oct 19, 2022
@manxorist
Copy link
Author

If Clang does not care about i386 at all, just returning an error for -march=i386 would also be fine for me. But that is probably not something for me alone to decide.

@efriedma-quic
Copy link
Collaborator

The last time anyone looked at stuff related to ancient CPUs was https://reviews.llvm.org/D59566 , I think, which fixed the use of cmpxchg8b on 486.

There's basically zero interest in building open source operating systems for Intel CPUs older than 486. Linux dropped 386 support a decade ago, NetBSD also requires a 486 minimum. If you're interested, you could try to implement it, though. The main thing would be teaching the backend that i386 doesn't have native atomics. (If we don't have cmpxchg, then we can't implement arbitrary atomic operations, so all atomic ops have to fallback to libatomic.)

@manxorist
Copy link
Author

Correct, there are no major systems still targeting i386, so this issue has not of any actual practical relevance as far as I can see. There is FreeDOS which still runs on i386 and there is DJGPP, but as far as I know, Clang cannot target DJGPP.
And as said initially, I do not have an actual use case which this issue affects either, I just noticed while looking at something different.
So, this is currently not of any priority for me to work on either.

@jckarter
Copy link
Contributor

It's reasonable for LLVM not to support 80386 targets, but It would be nice if -march=i386 at least raised an error as long as it isn't properly supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants