Closed andrewrk closed 1 year ago
@llvm/issue-subscribers-backend-mips
@llvm/issue-subscribers-bug
Also observed with -target powerpc-unknown-linux-unknown
.
@llvm/issue-subscribers-backend-powerpc
Also observed on powerpc64 and riscv64.
Hm. Since this happens on several backends it seems to be a problem on a higher level potentially. Not sure whom to ping here @RKSimon - do you know what this might be
udiv of 1 by X should have folded to zext(icmp_eq X, 1) in InstCombine - is something generating this in the backend?
But general division by illegal types is barely supported: #44994 #45013
@llvm/issue-subscribers-backend-mips
I very likely have more regressions to report against the release/16.x branch after this is solved. Please let me know if I need to make a workaround for this issue for now so that I can find the other possible regressions in order to ensure the quality of llvm 16.0.0 is high.
These lowerings can be done by calling the compiler_rt functions __umodei4
and __udivei4
.
LLVM 15.x did it, why can't LLVM 16.x do it?
This regressed in https://github.com/llvm/llvm-project/commit/6d13b80fcb1a338ef29eefd54677fdbcdd7c0e4d. I believe reverting that commit should be sufficient to solve this issue.
cc @mgehre-amd
These experimental, non-standard libcalls have been removed again, in favor of generating the necessary functions directly into the module. I believe this requires some per-target configuration that likely hasn't happened for anything but the major targets.
I think that was a mistake. The whole point of compiler_rt is to have libcalls such as these. Putting the functions directly in the module is clearly a workaround for some third party project's limitations. Instead, those limitations should be solved by said third parties.
The situation is pretty unfortunate, but I believe the consensus is that the libgcc situation doesn't really leave any other choice. There's some discussion on https://reviews.llvm.org/D120327 and a more recent one in https://discourse.llvm.org/t/proposal-split-built-ins-from-the-rest-of-compiler-rt/67978 (which is not about the integer division builtins, but happens to touch on many of the same topics).
For the record https://reviews.llvm.org/D126644 introduced the new approach and https://reviews.llvm.org/D130076 enabled it for X86, ARM and AArch64.
Thank you for providing this context. I am sorry I missed the discussion.
I think it is too bad that LLVM is introducing yet more complicated machinery when there is already a solution to the problem, compiler-rt. Maybe the gnu folks would add these functions to libgcc if you kept the pressure up.
Can there please be an option that I can enable to make LLVM do the straightforward libcall lowerings? It worked perfectly before, and now it is regressed. I don't want every object file to have bloated compiler_rt functions in them for no reason. I also want mips, powerpc, and riscv64 to continue working flawlessly, like they did with LLVM 15.
I believe that the codegen part only temporarily worked in LLVM 15, but not in any earlier version of LLVM. And even in LLVM 15, the library functions __divei4 etc were not in compiler-rt, so there was never a fully working end-to-end solution.
In LLVM 16 and later, those big integer divisions are supported end-to-end on X86, ARM and AArch64. I suggest to enable it for the targets you care about by creating a PR similar to https://reviews.llvm.org/D130076.
There was, actually, a fully working end-to-end solution. Don't forget that LLVM is not merely the internal compiler implementation details of clang, but a modular and reusable compiler backend technology.
Zig 0.10.x, which uses LLVM 15.x, supports multiplication and division for arbitrary sized integers via LLVM's __divei4 etc lowerings, and these are being used for a non-heap-allocating RSA implementation, used for TLS. It not only works on x86_64 and aarch64, but also on mips, powerpc, powerpc64, and riscv64.
Given that we are already at RC2, I don't think I can realistically do your suggestion and expect it to get landed by the time 16.x is tagged. I'm staring in the face of a major regression on all four of these targets.
What should I tell my users? "Sorry, we made a mistake by using LLVM for our compiler backend. They actually don't really care about regressions to targets other than x86 and ARM, or compiler frontends other than clang."?
I'm not opposed to bringing _divei4 back as a fallback/alternative for the current implementation. Feel free to open a PR that reverts 6d13b80fcb1a338ef29eefd54677fdbcdd7c0e4d.
If everyone agrees I can revert that commit on the release branch.
If everyone agrees I can revert that commit on the release branch.
Not sure this is a good idea, but this decision should probably be up to @efriedma-quic and @MaskRay. If we do this, these builtins will probably have to stay around forever, so there would have to be consensus for that.
Zig 0.10.x, which uses LLVM 15.x, supports multiplication and division for arbitrary sized integers via LLVM's __divei4 etc lowerings, and these are being used for a non-heap-allocating RSA implementation, used for TLS. It not only works on x86_64 and aarch64, but also on mips, powerpc, powerpc64, and riscv64.
So ... assuming the new inline expansions were correctly generated on all platforms, would those actually be usable for this use case? Does your current __divei4
implementation provide constant time guarantees?
Given that we are already at RC2, I don't think I can realistically do your suggestion and expect it to get landed by the time 16.x is tagged. I'm staring in the face of a major regression on all four of these targets.
The 16.x release is still two weeks away, so there's still time to add support for these targets.
What should I tell my users? "Sorry, we made a mistake by using LLVM for our compiler backend. They actually don't really care about regressions to targets other than x86 and ARM, or compiler frontends other than clang."?
There's clearly been a communications failure here -- from the LLVM perspective, this feature was never implemented in the first place, only a half-finished implementation stub happened to be in-tree at the time LLVM 15 was cut. It's unfortunate that this ended up being interpreted as a stable builtins ABI guarantee by zig. Now we have to deal with it somehow.
These experimental, non-standard libcalls have been removed again, in favor of generating the necessary functions directly into the module. I believe this requires some per-target configuration that likely hasn't happened for anything but the major targets.
Are you referring to __umodei4
and __udivei4
?
The original features https://reviews.llvm.org/D120327 (with __divmodei5
and __udivmodei5
compiler-rt/lib/builtins
functions) and https://reviews.llvm.org/D120329 were meant to have a GCC discussion (the naming collides with de facto namespace of libgcc functions) about but the discussion has not happened, so relanding the feature (e.g. reverting 6d13b80fcb1a338ef29eefd54677fdbcdd7c0e4d) will be premature.
I agree with the point that Zig relying on some implementation stubs happening to be in LLVM 15 is unfortunate. To be safe, release/16.x
should be similar to release/14.x
(clean, no such function) instead of release/15.x
(stub implementation) since we haven't resolved the builtins/libgcc interop problem.
https://reviews.llvm.org/D133691 adds some target hooks that apparently need to be implemented on a per-target basis. Extending those hooks to other targets, or adding some more reasonable default, should be straightforward to implement, and safe to cherry-pick to 16.x
I've put up https://reviews.llvm.org/D144871, which should avoid the legalization failure. I think this is good enough for LLVM 16, though long term it would certainly be nice if one could chose a libcall lowering if one provides those libcalls.
Thank you @nikic - I really appreciate this. I am happy to report that Zig has all compiler-rt tests passing for all targets in the llvm16 branch against release/16.x (abf32977809e8a04c9c98b9f3c72d43e37931ee9) + your one-liner to TargetLoweringBase.cpp.
@tstellar we should make sure that this is fixed/merged before 16.0.0.
/cherry-pick ddccc5ba4479a36dd4821f0948e118438fbf2e56
/branch llvm/llvm-project-release-prs/issue60531
Thanks @nikic !
/pull-request llvm/llvm-project-release-prs#332
LLVM version release/16.x branch commit 3f9b92d027f5d0bd23f852b66d47c5061363532d
reduced.ll