This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Emit warning for ieeelongdouble on older GNU toolchain
ClosedPublic

Authored by qiucf on Oct 31 2021, 10:27 PM.

Details

Summary

GCC 12 should have proper support for IEEE-754 compliant 128-bit floating point in libstdc++. So warning is needed when linking against older libstdc++ versions or LLVM libc++.

Glibc starts supporting float128 in both header and libraries since 2.32.

Diff Detail

Event Timeline

qiucf created this revision.Oct 31 2021, 10:27 PM
qiucf requested review of this revision.Oct 31 2021, 10:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2021, 10:27 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jsji added inline comments.Nov 1 2021, 6:40 AM
clang/test/Driver/ppc-float-abi-warning.cpp
3

Can we add RUN line to test when we are using libc++?

5

So this assume that all our buildbots are using old glibc? What if we have mixed envs, eg: some with newer glibc?

qiucf updated this revision to Diff 398906.Jan 11 2022, 4:22 AM
qiucf marked 2 inline comments as done.
qiucf edited the summary of this revision. (Show Details)
qiucf updated this revision to Diff 399262.Jan 12 2022, 2:07 AM
qiucf edited the summary of this revision. (Show Details)

Also check glibc version.

qiucf updated this revision to Diff 399536.Jan 12 2022, 7:56 PM
jsji accepted this revision as: jsji.Jan 20 2022, 1:18 PM

LGTM.

This revision is now accepted and ready to land.Jan 20 2022, 1:18 PM
nemanjai added inline comments.Jan 21 2022, 11:53 AM
clang/lib/Driver/ToolChains/PPCLinux.cpp
50

Seems that this will be less frequent than every compiler invocation, so we should probably check this first and early exit if the option isn't specified.

qiucf updated this revision to Diff 402405.Jan 23 2022, 11:23 PM
qiucf marked an inline comment as done.
This revision was landed with ongoing or failed builds.Jan 23 2022, 11:27 PM
This revision was automatically updated to reflect the committed changes.

GCC 12 should have proper support for IEEE-754 compliant 128-bit floating point in libstdc++.

Yes, but it's unconditionally disabled when including the headers with Clang.

qiucf added a comment.Feb 8 2022, 8:37 AM

GCC 12 should have proper support for IEEE-754 compliant 128-bit floating point in libstdc++.

Yes, but it's unconditionally disabled when including the headers with Clang.

Is that because clang lacks something required by this feature? (for example. clang-12 doesn't have __ibm128 and many builtins) If so, clang-14 should have these fixed.

Is that because clang lacks something required by this feature? (for example. clang-12 doesn't have __ibm128 and many builtins) If so, clang-14 should have these fixed.

When I implemented the libstdc++ changes Clang didn't have any of your work yet. Now that everything seems to be present, and __LONG_DOUBLE_IEEE128__ and __LONG_DOUBLE_IBM128__ are defined as appropriate, I think I can enable it for clang. I'll look into that tomorrow.

qiucf added a comment.Feb 8 2022, 8:55 AM

Is that because clang lacks something required by this feature? (for example. clang-12 doesn't have __ibm128 and many builtins) If so, clang-14 should have these fixed.

When I implemented the libstdc++ changes Clang didn't have any of your work yet. Now that everything seems to be present, and __LONG_DOUBLE_IEEE128__ and __LONG_DOUBLE_IBM128__ are defined as appropriate, I think I can enable it for clang. I'll look into that tomorrow.

Thanks! I verified latest LLVM/Clang with latest libstdc++ in some programs and they look okay. If you found any more issues, I still have time to get them fixed in clang 14 rc2.

tbaeder added a subscriber: tbaeder.Mar 8 2022, 5:04 AM

Hi @qiucf,

I'm running into a problem with the testsuite that's caused by this patch. I think the comment from @jsji about newer glibc was never addressed.

When running the testsuite on ppc64le with a glibc newer than 2.34, the the warning mentioned in ppc-float-abi-warning.cpp is not emitted when using libstdc++.

In PPCLinuxToolChain::SupportIEEEFloat128():

bool HasUnsupportedCXXLib =
    ToolChain::GetCXXStdlibType(Args) == CST_Libcxx &&
    GCCInstallation.getVersion().isOlderThan(12, 1, 0);

return GlibcSupportsFloat128(Linux::getDynamicLinker(Args)) &&
       !(D.CCCIsCXX() && HasUnsupportedCXXLib);

When using libstdc++, HasUnsupportedCXXLib will always be false since it's not CST_Libcxx...

I don't understand the HasUnsupportedCXXLib variable tbh, is there a typo in the stdlib check, or in a &&/|| mixup?
Right now it is only true if we're using libc++ and the GCC installation is older than 12.1.0.

I will try one approach in the meantime, but your opinion would be appreciated.

Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 5:04 AM