This is an archive of the discontinued LLVM Phabricator instance.

[builtins] Fix c?zdi2 on sparc64/Linux and ignore riscv32
ClosedPublic

Authored by jrtc27 on Feb 9 2018, 2:41 PM.

Details

Summary

On sparc64/Linux, sparc64 isn't defined; the canonical way of
checking for sparc64 is sparc && arch64, which also works on the
BSDs and Solaris. Since this problem does not occur on 32-bit
architectures, riscv32 can be ignored. This fixes and refines rL324593.

Diff Detail

Event Timeline

JDevlieghere created this revision.Feb 9 2018, 2:41 PM
Herald added subscribers: Restricted Project, fedor.sergeev, jyknight. · View Herald TranscriptFeb 9 2018, 2:41 PM
jrtc27 added a subscriber: jrtc27.EditedFeb 9 2018, 2:45 PM

Actually this might be ok on sparc32? I assume __builtin_ctz will instead resolve to _ctzsi2 being a 32-bit architecture? In which case, shouldn't the __riscv__ one be checking __SIZEOF_POINTER__ == 8?

Actually this might be ok on sparc32? I assume __builtin_ctz will instead resolve to _ctzsi2 being a 32-bit architecture? In which case, shouldn't the __riscv__ one be checking __SIZEOF_POINTER__ == 8?

Feel free to commandeer this revision with what you think should be the proper solution. As Hans mentioned in https://llvm.org/PR36318 we should do it soon if we want to have this in the 6.0 release.

jrtc27 commandeered this revision.Feb 19 2018, 8:42 AM
jrtc27 added a reviewer: JDevlieghere.
jrtc27 updated this revision to Diff 134930.Feb 19 2018, 8:43 AM

[builtins] Fix c?zdi2 on sparc64/Linux and refine mips64/riscv tests

On sparc64/Linux, sparc64 isn't defined; the canonical way of
checking for sparc64 is sparc && arch64, which also works on the
BSDs and Solaris. Newer mips64 CPUs have a clz instruction which is used
for both clz and ctz (except in mips16 mode), so those do not need the
workaround, nor does riscv32. This fixes and refines rL324593.

jrtc27 retitled this revision from [builtin] Update c?zdi2 to cover Linux/sparc to [builtins] Fix c?zdi2 on sparc64/Linux and refine mips64/riscv tests.Feb 19 2018, 8:44 AM
jrtc27 edited the summary of this revision. (Show Details)
joerg added a comment.Feb 19 2018, 9:35 AM

We tried to keep the condition simple. I.e. does the compiler on any of those platforms ever use the libcall? If not, it is IMO not worth the complexity.

We tried to keep the condition simple. I.e. does the compiler on any of those platforms ever use the libcall? If not, it is IMO not worth the complexity.

Ok, I can see the argument for getting rid of the mips64 complexity, but I'd say the __SIZEOF_POINTER__ check should stay for riscv, since it definitely won't be a problem for riscv32. Would you agree with that?

jrtc27 updated this revision to Diff 134996.Feb 19 2018, 5:09 PM

Dropped complicated logic for mips64

jrtc27 edited the summary of this revision. (Show Details)Feb 19 2018, 5:10 PM
jrtc27 retitled this revision from [builtins] Fix c?zdi2 on sparc64/Linux and refine mips64/riscv tests to [builtins] Fix c?zdi2 on sparc64/Linux and ignore riscv32.
kito-cheng requested changes to this revision.Feb 19 2018, 6:45 PM
kito-cheng added inline comments.
lib/builtins/clzdi2.c
22 ↗(On Diff #134996)

RISC-V define __riscv instead of __riscv__.

This revision now requires changes to proceed.Feb 19 2018, 6:45 PM
jrtc27 updated this revision to Diff 135146.Feb 20 2018, 2:18 PM

Updated to check __riscv instead of the deprecated __riscv__

jrtc27 marked an inline comment as done.Feb 20 2018, 2:18 PM
kito-cheng accepted this revision.EditedFeb 20 2018, 5:49 PM

LGTM for RISC-V part :)

This revision is now accepted and ready to land.Feb 20 2018, 5:49 PM
compnerd requested changes to this revision.Feb 20 2018, 6:44 PM

Please clang-format the conditional (we keep the operator on the previous line).

I'm wondering if this is becoming a pretty complex set of cases and it's better to just do __SIZEOF_POINTER__ == 8. The question is, how does it impact x86_64 and aarch64, and if those are not impacted in a meaningful way, I think that we should just go ahead and simplify the condition to all 64-bit targets. Then again, this is a GCC specific codepath, and I'm tempted to say, we can afford the overhead of an additional call.

This revision now requires changes to proceed.Feb 20 2018, 6:44 PM

Please clang-format the conditional (we keep the operator on the previous line).

I'm wondering if this is becoming a pretty complex set of cases and it's better to just do __SIZEOF_POINTER__ == 8. The question is, how does it impact x86_64 and aarch64, and if those are not impacted in a meaningful way, I think that we should just go ahead and simplify the condition to all 64-bit targets. Then again, this is a GCC specific codepath, and I'm tempted to say, we can afford the overhead of an additional call.

I guess for 64-bit architectures with a native clz instruction we would expect that instruction to be used for the DI version too, and therefore not be calling __clzdi2 in the first place? I.e. the only 64-bit platforms which actually call __clzdi2 are the ones where we have a problem? I could be wrong about this though.

Is there any chance this PR can get completed?

jrtc27 updated this revision to Diff 155561.Jul 14 2018, 10:31 AM

Fixed formatting with the help of clang-format.

JDevlieghere accepted this revision.Aug 28 2018, 6:53 AM

This LGTM. @compnerd can you live with this?

compnerd accepted this revision.Sep 11 2018, 9:11 AM

Sure, I think I can live with this.

This revision is now accepted and ready to land.Sep 11 2018, 9:11 AM

@JDevlieghere Can you commit this fix on behalf of jrtc27? He's currently on vacation.

Can someone commit this, please?

Landing it per request as it's approved (just going to be a second, running it on a buildbot first).

This revision was automatically updated to reflect the committed changes.