search

odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.55k stars 570 forks source link

enable_target_features should error if the target machine microarchitecture does not support them #3103

Closed JesseRMeyer closed 4 months ago

JesseRMeyer commented 7 months ago

https://github.com/odin-lang/Odin/blob/ae0be9c78587a13857f03cdc08602cf2e6a9282b/core/simd/x86/abm.odin#L6

Simply importing simd/x86 forcibly enables CPU target features, even if a target microarchitecture is explicitly given which lacks support.

Yawning commented 7 months ago

I stated my opinion on Discord, but I'll do it again here for the record.

Yes, there is a bug here, but I disagree that there should be an error. In my view the bug is that enable_target_feature isn't function-scoped. Doing CPUID based dynamic dispatch is a very common thing to do, and the whole point of intrinsics is as a replacement for an inline assembler (that odin lacks).

It should be possible to compile for -march:prehistoric, and support modern hardware (eg: AVX2), by peeking at the CPUID and calling a specialized implementation. Forcing the target to be -march:modern in order to get the compiler to emit modern instuctions at all makes this impossible.

JesseRMeyer commented 7 months ago

What is the purpose of the -microarch flag in your eyes if the compiler generates all such instructions and dynamically dispatches at runtime?

Yawning commented 7 months ago

What is the purpose of the -microarch flag in your eyes if the compiler generates all such instructions and dynamically dispatches at runtime?

In my view:

Under this model, -microarch provides the default, enable_target_feature overrides on a per-function basis. So the compiler will not emit say AVX2, except when the developer chooses to use intrinsics, and only for the intrinsic calls. This would (it currently does not, because the feature implementation is incorrect) allow creating binaries that run on less-modern architectures, that also can also, at runtime depending on the hardware, leverage more modern features. My view is that the moment intrinsics (which are a more ergonomic alternative to inline assembly) is involved, the developer should be assumed to know what they are doing, and it is on them to check CPUID before calling their routine that uses AVX2, especially if they are going through the trouble of setting -microarch to something older.

What you want is functionally equivalent to "get rid of enable_target_feature" because each -microarch has every single feature supported, and the compiler will freak out the moment it's asked to emit something unsupported. If the instruction subset as defined by microarch can't be overridden, there's no point to having the annotation at all.