This is an archive of the discontinued LLVM Phabricator instance.

Rename GCCBuiltin into ClangBuiltin
ClosedPublic

Authored by GuillaumeGomez on Jun 9 2022, 4:54 PM.

Details

Summary

This patch is needed because developers expect "GCCBuiltin" items to be the GCC intrinsics equivalent and not the Clang internals.

Diff Detail

Event Timeline

GuillaumeGomez created this revision.Jun 9 2022, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 4:54 PM
GuillaumeGomez requested review of this revision.Jun 9 2022, 4:54 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 9 2022, 4:54 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

As mentioned in https://reviews.llvm.org/D127409, it would avoid confusions to name it ClangBuiltin instead of keeping GCCBuiltin.

cc @craig.topper

ldionne accepted this revision as: Restricted Project.Jun 13 2022, 7:49 AM
ldionne added a subscriber: ldionne.

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_.

Fix libcxxabi mangling test

Fix libcxxabi mangling test

GuillaumeGomez added 1 blocking reviewer(s): Restricted Project.Jun 13 2022, 12:51 PM

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.

@efriedma It was falling the tests because of that, which is why I updated it.

RKSimon added inline comments.Jun 14 2022, 2:00 AM
llvm/lib/IR/Function.cpp
1429–1430

Should this be updated to use GET_LLVM_INTRINSIC_FOR_CLANG_BUILTIN?

GuillaumeGomez added inline comments.Jun 14 2022, 4:20 AM
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.

Update #define as well

I applied the changes suggested by @RKSimon and @efriedma.

llvm/include/llvm/ADT/Triple.h
1031

You're right, I removed this change as it's unrelated.

llvm/lib/IR/Function.cpp
1429–1430

I fixed the error so it's now done. Thanks for the suggestion!

RKSimon added inline comments.Jun 14 2022, 7:38 AM
llvm/utils/TableGen/IntrinsicEmitter.cpp
882–883

IsGCC?

Rename IsGCC function parameter into IsClang

Applied @RKSimon's suggestion.

llvm/utils/TableGen/IntrinsicEmitter.cpp
882–883

I didn't even notice when updating it... Updating to IsClang.

LGTM (with one minor) - @efriedma any more feedback?

llvm/utils/TableGen/IntrinsicEmitter.cpp
82

"Clang builtins"

No other comments

spatel added inline comments.Jun 14 2022, 12:01 PM
llvm/include/llvm/IR/Intrinsics.td
388

Replace "GCC" with "Clang" ?

  • Update #define name as well
  • Rename IsGCC function parameter into IsClang
GuillaumeGomez marked an inline comment as done.Jun 14 2022, 12:06 PM

Fixed @spatel's comment.

llvm/include/llvm/IR/Intrinsics.td
388

Good catch, thanks!

RKSimon accepted this revision.Jun 15 2022, 1:47 AM

LGTM

I don't think this is really blocked on the libc team any more?

ldionne accepted this revision as: Restricted Project.Jun 15 2022, 2:41 PM
This revision is now accepted and ready to land.Jun 15 2022, 2:41 PM
GuillaumeGomez marked an inline comment as done.Jun 22 2022, 2:14 AM

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)

It looks like this is good to go - do you have commit access?

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?

xbolva00 requested changes to this revision.Jun 22 2022, 3:06 AM
xbolva00 added a subscriber: xbolva00.

Patch description needs to be added, especially answer the question why we should rename it, why we want it..

This revision now requires changes to proceed.Jun 22 2022, 3:06 AM

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?

https://llvm.org/docs/DeveloperPolicy.html#id17

Rename GCCBuiltin into ClangBuiltin.

This patch is needeed because developers expect "GCCBuiltin" items to be the GCC intrinsics equivalent and not the Clang internals.

Patch description needs to be added, especially answer the question why we should rename it, why we want it..

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.

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?

https://llvm.org/docs/DeveloperPolicy.html#id17

Thanks! Once this is approved, I'll do it then.

RKSimon edited the summary of this revision. (Show Details)Jun 22 2022, 5:38 AM
xbolva00 accepted this revision.Jun 22 2022, 5:42 AM
This revision is now accepted and ready to land.Jun 22 2022, 5:42 AM

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
This revision was landed with ongoing or failed builds.Jun 22 2022, 11:49 AM
This revision was automatically updated to reflect the committed changes.
tra added a subscriber: tra.Mar 20 2023, 3:09 PM

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.

llvm/include/llvm/IR/IntrinsicsVE.td