This is an archive of the discontinued LLVM Phabricator instance.

[llvm] infer rtlib from triple for codegen decisions
Changes PlannedPublic

Authored by nickdesaulniers on Feb 1 2023, 4:23 PM.

Details

Reviewers
None
Summary

alternative to https://reviews.llvm.org/D108928

https://discourse.llvm.org/t/proposal-split-built-ins-from-the-rest-of-compiler-rt/67978

To run unittests:

$ cd llvm/build; ninja UnitTests; cd -
$ ./llvm/build/unittests/TargetParser/TargetParserTests \
    --gtest_filter=TargetParserTest.getRTLib

Diff Detail

Event Timeline

nickdesaulniers created this revision.Feb 1 2023, 4:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 4:23 PM
nickdesaulniers requested review of this revision.Feb 1 2023, 4:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 4:23 PM
nickdesaulniers planned changes to this revision.Feb 1 2023, 4:24 PM
nickdesaulniers retitled this revision from alternative to https://reviews.llvm.org/D108928 to [llvm.Feb 1 2023, 4:26 PM
nickdesaulniers retitled this revision from [llvm to [llvm] infer rtlib from triple for codegen decisions.
nickdesaulniers edited the summary of this revision. (Show Details)
mstorsjo added inline comments.
llvm/lib/TargetParser/Triple.cpp
1235

Depending on the actual use of this function, I think we'd need to be more careful than this, to only return a non-unknown value for the platforms where we're absolutely certain that there can't be any other choice. For platforms like apple and android, this probably is the case - possibly for some of the BSDs as well (not entirely sure).

At least for mingw targets (which do hit the isGNUEnvironment() at the top), toolchains with both libgcc and compiler-rt are in active use.

With the only use of getRTLib() as suggested in this patch, it's harmless to return libgcc (it'd be equal to returning unknown), but with the function in place, the function may be used in other ways too.

nickdesaulniers retitled this revision from [llvm] infer rtlib from triple for codegen decisions to alternative to https://reviews.llvm.org/D108928.
nickdesaulniers edited the summary of this revision. (Show Details)
  • add unittest, slightly expand Triple::getRTLib with more cases.
nickdesaulniers planned changes to this revision.Feb 2 2023, 10:53 AM
  • use llvm::Triple::normalize to simplify tests
nickdesaulniers planned changes to this revision.Feb 2 2023, 10:56 AM
nickdesaulniers retitled this revision from alternative to https://reviews.llvm.org/D108928 to [llvm] infer rtlib from triple for codegen decisions.
nickdesaulniers edited the summary of this revision. (Show Details)
  • update description
nickdesaulniers planned changes to this revision.Feb 2 2023, 10:57 AM
mstorsjo added inline comments.Feb 2 2023, 12:20 PM
llvm/include/llvm/TargetParser/Triple.h
296

Some comments on the naming here...

There's really nothing like WINDOWS_RT - the OS itself doesn't have anything corresponding to it, but it's all a toolchain specific library - I think the most correct name for it right now would be VCRUNTIME.

Also as a nitpick/personal opinion, spelling it LIB_GCC feels weird to me, I'd just spell it LIBGCC.

llvm/lib/TargetParser/Triple.cpp
1235

Thanks - the change here seems to take care of my immediate concerns at least.

phosek added inline comments.Feb 3 2023, 1:06 AM
llvm/include/llvm/TargetParser/Triple.h
296

Another suggestion on the naming... we don't use ALL_CAPS for other enum values unless they're acronyms, so CompilerRT, Libgcc and VCRuntime would be more in line with the existing values.