-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Merge r324593 into the 6.0 branch : [builtins] Workaround for infinite recursion in c?zdi2 #35666
Comments
Let's put this on hold until https://reviews.llvm.org/D43146 has been landed. |
Can you provide more context on what this is fixing? It needs to get committed soon if it's to make the 6.0 release. |
__builtin_clz is int -> int, and often implemented using a native clz (or similar) instruction. However, on 64-bit architecture which don't have such an instruction, rather than calling the __clzsi2 function (as one would expect, given SI is the GCC internal name for int), it instead uses __clzdi2, the 64-bit (DI) version. Since compiler-rt's implementation of __clzdi2 uses a divide-and-conquer approach of calling __builtin_clz on each half of the 64-bit value, on these architectures, the __builtin_clz ends up calling __clzdi2 itself, thus leading to non-terminating recursion, whereas the intention is that, if there's no clz instruction, __clzdi2 ends up calling compiler-rt's __clzsi2. Thus the patch simply identifies these problematic platforms and re-defines __builtin_clz to be __clzsi2 itself. The same is true for __builtin_ctz and __ctz{d,s}i2 (which will also make use of a ctz OR clz (clz(x & (~x+1)) is 31 - ctz(x)) instruction if available). |
Is this a regression from 5.0.0? I'm nervous about merging this to 6.0.0 since we're late in the process, and it's not a completely straight-forward fix. Unless it's really fixing a regression from the previous release, it might have to wait until 6.0.1. |
I agree, this is not a regression and should go in 6.0.1, assuming D43146 has landed by then. I see no point in getting only a partial workaround in 6.0. |
mentioned in issue #35997 |
1 similar comment
mentioned in issue #35997 |
Old release |
Extended Description
Is it OK to merge the following revision(s) to the 6.0 branch?
The text was updated successfully, but these errors were encountered: