RuntimeLibcalls: Stop opting out of exp10 #148604
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
arsenm
requested review from
DanielKristofKiss,
dpaoliello,
efriedma-quic,
nikic and
RKSimon
|
@llvm/pr-subscribers-backend-x86 Author: Matt Arsenault (arsenm) ChangesThis changes the behavior on old darwin triples for x86_fp80; Full diff: https://github.com/llvm/llvm-project/pull/148604.diff 4 Files Affected:
|
|
@llvm/pr-subscribers-llvm-ir Author: Matt Arsenault (arsenm) ChangesThis changes the behavior on old darwin triples for x86_fp80; Full diff: https://github.com/llvm/llvm-project/pull/148604.diff 4 Files Affected:
|
arsenm
force-pushed
the
users/arsenm/runtime-libcalls/stop-opt-out-exp10
branch
from
d2d9452 to
b2fc78b
Compare
arsenm
force-pushed
the
users/arsenm/runtime-libcalls/x86-mostly-move-libcalls-tablegen
branch
from
1a5ee21 to
5915e12
Compare
arsenm
force-pushed
the
users/arsenm/runtime-libcalls/stop-opt-out-exp10
branch
from
a64799d to
c35ac3c
Compare
arsenm
force-pushed
the
users/arsenm/runtime-libcalls/x86-mostly-move-libcalls-tablegen
branch
from
5915e12 to
27fd017
Compare
arsenm
force-pushed
the
users/arsenm/runtime-libcalls/stop-opt-out-exp10
branch
from
c35ac3c to
465db65
Compare
|
For my understanding: this is expected to ICE because there is no possible libcall here (since |
These are equivalent. There needs to be a known symbol that provides the behavior needed to implement exp10.fp128. The "default target lib" concept doesn't really exist, it's a historical implementation artifact. The compiler needs accurate knowledge about a concrete target
In theory the backend needs to do something to provide the intrinsic behavior. This is currently built on top of emitting libm calls to shoddy host math libraries. In a better world we would have complete coverage of all functions x types in compiler-rt and ignore the host libraries, or rip out most math intrinsics and disallow types we don't have a direct compiler implementation of |
|
Except for target specific types (x86_fp80 and ppc_fp128) I think it is preferable if missing FP libcalls that cannot be legalized result in a linker error instead of a compiler error, which allows the user to provide them. At least when considering the world we live in now, rather than possible future designs. |
|
Actually, is there anything unique about
In theory this could lower to the C23 Edit: sorry, I see you aren't talking about x86_fp80 here. |
It would be nice if there was a triple flag to indicate presence of a backend-agnostic C23-compliant library, so e.g. I know we talked about something like this on Discord a while back. |
Yeah, that's my understanding as well.
Yes, what specifically concerns me is that change to fp128 behavior in this PR. I don't think that should ever result in a hard compiler error. I wasn't aware that the f64x suffix is a thing. That does seem like a reasonable fallback for targets where long double is not x86_fp80. But I'm also not sure this is really worth bothering with. Is there any real interest in having fp80 work across all x86 targets? |
I don't think it's reasonable to support this kind of use case for a known triple. Maybe for something defined as freestanding, but not a definitive target. In theory the compiler should be using this knowledge of this-call-exists-or-not to use a fallback path to avoid using that call. If we have to pretend phantom calls exist in the off chance someone is hacking around library defects, I don't see how you can ever move beyond broken host libraries |
|
Plus this is one of the odd cases that happened to emit some kind of code. Most of these untested, accidental libcall cases hit asserts and crashes in codegen so it's not like a designed system |
|
For rust I suppose we could switch to calling the Regardless whether the
I'd like to add the type to |
I agree with this. A linker error we can straightforwardly address by implementing the missing function in Zig's compiler-rt. An ICE from LLVM is significantly more annoying to work around.
In Zig, we currently lower
I don't believe Zig emits |
I think that would be a reasonable position to take if the fallback path to avoid the call actually existed. But it does not, so we should not unnecessarily force an error. The longer term solution here would be along the lines of what @tgross35 suggested, that is the ability to override LLVM's default assumptions about libcalls for the targets, to be able to say things like "I'm actually linking against compiler-rt rather than libgcc" or "I will link against a full featured (glibc-style) libm". But I think that until that is supported, we should not change the status quo of how the FP libcall fallbacks work.
I don't think that's a great option, as LLVM will no longer understand these libcalls and won't be able to e.g. constant fold them.
I think neither does Rust, so the behavior for this particular builtin changing wouldn't be a big deal. But if this were to extend to other FP libcalls, there'd be a problem. |
|
Can |
I'm more or less working towards this
We don't actually have a notion of FP libcall fallbacks. We have a configured set of calls to provide a feature, with a set of defaults. It's a poorly maintained opt-out system that has always errored on missing calls. Other intrinsics that require a libcall to implement will also hard error. e.g. you will get the same error in the exp10 f32 and f64 cases on Darwin. This change just makes the long double cases consistent. I could lie and add only exp10l but that does not seem like the intended behavior
It also doesn't address the problem. The compiler needs to know the set of target library calls, and can synthesize calls that were not in the original program.
exp10 is unique in that it had explicit opt-outs from the default. I don't think there is another example like this among the math functions. The closest would be a few missing f32 calls on windows, but those have legalization paths to promote or expand. If there are other platform unsupported math calls, the work to properly report that was not done. It's also unique in that TargetLibraryInfo deliberately does not report it as available by default, so there's a mismatch with RuntimeLibcalls |
|
Here is the status quo: https://godbolt.org/z/PPzn9zMs8 The f32 and f64 cases hard error. The f128 case incorrectly codegens to exp10l, and the x86_fp80 case codegens to a function which does not exist. This makes all 4 cases consistently error |
This changes the behavior on old darwin triples for x86_fp80; it now turns into an error instead of emitting exp10l. The comments in TargetLibraryInfo suggest it never existed.
Co-authored-by: Trevor Gross <[email protected]>
arsenm
force-pushed
the
users/arsenm/runtime-libcalls/stop-opt-out-exp10
branch
from
465db65 to
9c031f5
Compare
There was a problem hiding this comment.
I'm still not really convinced that your general approach here is an improvement over the status quo. exp10 is going to be available on any platform using glibc/musl, regardless of architecture. Excluding it from the default set for the sake of darwin platforms, and then having to add it back for every single architecture does not seem like a good tradeoff to me. Wouldn't it be better to remove it instead for the two arches where darwin is actually relevant?
Alternatively, it seems to me like you need a mechanism that allows you to exclude exp10 on darwin in a way that does not require listing this for each arch.
| ; CHECK-ALL-LABEL: test_exp10: | ||
| ; CHECK-F128: exp10f128 | ||
| ; CHECK-USELD: exp10l | ||
| ; CHECK-S390X: exp10l |
There was a problem hiding this comment.
No need for separate S390X prefix.
I very deliberately do not want any opt-out mechanism. All target based decisions should be explicitly opt-in, and all the places that are opt-out were a mistake. It results in falsely reporting things that don't exist as available, like this. And hard to untangle messes. This is a staging step to getting all logic into tablegen to work towards that |
I don't mind it being opt-in, my concern here is that you require a per-arch opt-in, even though the decision should be largely arch-independent (it should depend on os/env). Like for example, you added the exp10 functions to most of the target system libraries here, but not to all of them. What about hexagon, msp430 and xcore? Were those intentionally omitted? Even if it's not part of the default set, I think this can be structured in a better way that does not require repeating certain parts of the system library in each arch. We should extract the commonalities to the degree it is possible. |
|
Looking at the follow-up patch #148780 answers my questions about hexagon/msp/xcore... I guess what I'm looking for is to basically have most targets share this libcall list... ...without having to repeat it in each target. |
That's the end of it though, this series is enough to get all of the generic code into tablegen which makes doing anything else easier. Nowhere else in llvm treats the OS/environment as the top target concept, but this could be transposed if it makes it smaller, I guess. Eventually I want to delete DefaultRuntimeLibcallImpls and replace it with named subsets of libc and compiler-rt, but I have to reverse engineer what is actually intended to be added and what combinations are meaningful. It's a pain figuring it out from what we have now, where there's no documentation and barely any tests. The current logic is special casing these specific cases, so that's what it gets ported to. |
This changes the behavior on old darwin triples for x86_fp80;
it now turns into an error instead of emitting exp10l. The comments
in TargetLibraryInfo suggest it never existed.