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

Merge r324593 into the 6.0 branch : [builtins] Workaround for infinite recursion in c?zdi2 #35666

Closed
JDevlieghere opened this issue Feb 9, 2018 · 9 comments
Labels
bugzilla Issues migrated from bugzilla obsolete Issues with old (unsupported) versions of LLVM release:backport

Comments

@JDevlieghere
Copy link
Member

Bugzilla Link 36318
Version 6.0
OS All
Blocks #35997
CC @zmodem,@jrtc27,@tstellar
Fixed by commit(s) r324593

Extended Description

Is it OK to merge the following revision(s) to the 6.0 branch?

@JDevlieghere
Copy link
Member Author

@JDevlieghere
Copy link
Member Author

Let's put this on hold until https://reviews.llvm.org/D43146 has been landed.

@zmodem
Copy link
Collaborator

zmodem commented Feb 19, 2018

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.

@jrtc27
Copy link
Collaborator

jrtc27 commented Feb 19, 2018

__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).

@zmodem
Copy link
Collaborator

zmodem commented Feb 20, 2018

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.

@JDevlieghere
Copy link
Member Author

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.

@zmodem
Copy link
Collaborator

zmodem commented Nov 27, 2021

mentioned in issue #35997

1 similar comment
@zmodem
Copy link
Collaborator

zmodem commented Nov 27, 2021

mentioned in issue #35997

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@arsenm arsenm added release:backport obsolete Issues with old (unsupported) versions of LLVM labels Aug 13, 2023
@arsenm
Copy link
Contributor

arsenm commented Aug 13, 2023

Old release

@arsenm arsenm closed this as not planned Won't fix, can't repro, duplicate, stale Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla obsolete Issues with old (unsupported) versions of LLVM release:backport
Projects
None yet
Development

No branches or pull requests

4 participants