This patch is needed because developers expect "GCCBuiltin" items to be the GCC intrinsics equivalent and not the Clang internals.
Details
- Reviewers
efriedma RKSimon spatel craig.topper xbolva00 - Group Reviewers
Restricted Project - Commits
- rGd0a4450ecdaf: Rename GCCBuiltin into ClangBuiltin
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
As mentioned in https://reviews.llvm.org/D127409, it would avoid confusions to name it ClangBuiltin instead of keeping GCCBuiltin.
LGTM for libc++abi once my comment is addressed, which should make the libc++ CI pass. Please don't submit this until you have a green check-mark from the libc++ CI.
llvm/include/llvm/ADT/Triple.h | ||
---|---|---|
1031 | Comment applies to test_demangle.pass.cpp (it's a binary file so I can't comment on it). The test is currently failing because you need to change _ZN4llvm9Intrinsic25getIntrinsicForClangBuiltinEPKcS2_ to _ZN4llvm9Intrinsic27getIntrinsicForClangBuiltinEPKcS2_. |
I finally took time to fix the failing test.
For some reason, I can't seem to push without marking libcxxabi/test/test_demangle.pass.cpp as a binary file with arc diff HEAD~2 --update D127460. I hope this is fine...
Why are you messing with test_demangle.pass.cpp? The demangler doesn't care what symbols we actually define in LLVM... it's just a bunch of hardcoded testcases. So it doesn't matter if it continues to refer to GCCBuiltin.
Other changes look fine.
llvm/lib/IR/Function.cpp | ||
---|---|---|
1429–1430 | Should this be updated to use GET_LLVM_INTRINSIC_FOR_CLANG_BUILTIN? |
llvm/lib/IR/Function.cpp | ||
---|---|---|
1429–1430 | I think it should for coherency. I'll send an update. | |
1429–1430 | When replacing with CLANG, it cannot build. After some investigations, it's because of this: llvm/utils/TableGen/IntrinsicEmitter.cpp:908: OS << "#ifdef GET_LLVM_INTRINSIC_FOR_" << CompilerName << "_BUILTIN\n"; So I'll let it as is for the time. |
llvm/utils/TableGen/IntrinsicEmitter.cpp | ||
---|---|---|
882–883 | IsGCC? |
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
388 | Replace "GCC" with "Clang" ? |
Is there anything else for me to do in here so it gets merged? (Sorry if I missed it but I couldn't find out the exact merge process in the documentation)
This is my first LLVM contribution so I don't think I do? If I do have a commit access anyway, do you have a link to the documentation where it explains what I'm supposed to do by any chance?
Patch description needs to be added, especially answer the question why we should rename it, why we want it..
Rename GCCBuiltin into ClangBuiltin.
This patch is needeed because developers expect "GCCBuiltin" items to be the GCC intrinsics equivalent and not the Clang internals.
I updated the description to explain why it is needed. I'm not sure how big it's supposed to be so I didn't enter into the details though.
Thanks! Once this is approved, I'll do it then.
So from the documentation:
Prior to obtaining commit access, it is common practice to request that someone with commit access commits on your behalf. When doing so, please provide the name and email address you would like to use in the Author property of the commit.
So as git commit author I use:
"Guillaume Gomez <guillaume1.gomez@gmail.com>"
Can someone commit on my behalf please?
Thanks in advance!
- Rename GCCBuiltin into ClangBuiltin
- Update #define name as well
- Rename IsGCC function parameter into IsClang
Would it make sense to also introduce a ClangTargetBuiltin for the cases where we use TARGET_BUILTIN on LLVM side now?
A lot of the builtins we have in clang are TARGET_BUILTIN() with constrained availability (~5K, if my grep-foo works correctly).
Right now we attempt to keep things in sync manually, but we don't always manage to do that correctly. Having one-shot for such a common case would help with that a bit.
Comment applies to test_demangle.pass.cpp (it's a binary file so I can't comment on it). The test is currently failing because you need to change _ZN4llvm9Intrinsic25getIntrinsicForClangBuiltinEPKcS2_ to _ZN4llvm9Intrinsic27getIntrinsicForClangBuiltinEPKcS2_.