[NFC] Rename Intrinsic::getDeclaration to getOrInsertDeclaration
#111752
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rename the function to reflect its correct behavior and to be consistent with `Module::getOrInsertFunction`. This is also in preparation of adding a new `Intrinsic::getDeclaration` that will have behavior similar to `Module::getFunction` (i.e, just lookup, no creation).
jurahul
force-pushed
the
rename_intrinsic_get_declaration
branch
from
6cab160
to
b4e9093
Compare
llvmbot
added
clang
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/7028 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/6731 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/6436 Here is the relevant piece of the build log for the reference |
ichaer
added a commit
to splunk/ichaer-llvm-project
that referenced
this pull request
Oct 11, 2024
…ent-indentonly * llvm-trunk/main: (6379 commits) [gn build] Port 1c94388 [RISCV] Introduce VLOptimizer pass (llvm#108640) [mlir][vector] Add more tests for ConvertVectorToLLVM (7/n) (llvm#111895) [libc++] Add output groups to run-buildbot (llvm#111739) [libc++abi] Remove unused LIBCXXABI_LIBCXX_INCLUDES CMake option (llvm#111824) [clang] Ignore inline namespace for `hasName` (llvm#109147) [AArch64] Disable consecutive store merging when Neon is unavailable (llvm#111519) [lldb] Fix finding make tool for tests (llvm#111980) Turn `-Wdeprecated-literal-operator` on by default (llvm#111027) [AMDGPU] Rewrite RegSeqNames using !foreach. NFC. (llvm#111994) Revert "Reland: [clang] Finish implementation of P0522 (llvm#111711)" Revert "[clang] CWG2398: improve overload resolution backwards compat (llvm#107350)" Revert "[clang] Implement TTP P0522 pack matching for deduced function template calls. (llvm#111457)" [Clang] Replace Intrinsic::getDeclaration with getOrInsertDeclaration (llvm#111990) Revert "[NVPTX] Prefer prmt.b32 over bfi.b32 (llvm#110766)" [RISCV] Add DAG combine to turn (sub (shl X, 8-Y), (shr X, Y)) into orc.b (llvm#111828) [libc] Fix compilation of new trig functions (llvm#111987) [NFC] Rename `Intrinsic::getDeclaration` to `getOrInsertDeclaration` (llvm#111752) [NFC][CodingStandard] Add additional example for if-else brace rule (llvm#111733) CodeGen: Remove redundant REQUIRES registered-target from tests (llvm#111982) ...
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/9650 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/10003 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/92/builds/7899 Here is the relevant piece of the build log for the reference |
|
@jurahul |
|
Yes I purposely did not change it as it just seems to test mangling and
using this function as an example. I can change it if it makes sense.
…On Fri, Oct 11, 2024 at 9:10 AM Daniel Chen ***@***.***> wrote:
@jurahul <https://github.com/jurahul>
It seems this PR is missing a test file
libcxxabi/test/test_demangle.pass.cpp.
It has a line of
{"_ZN4llvm9Intrinsic14getDeclarationEPNS_6ModuleENS0_2IDEPPKNS_4TypeEj", "llvm::Intrinsic::getDeclaration(llvm::Module*, llvm::Intrinsic::ID, llvm::Type const**, unsigned int)"},
—
Reply to this email directly, view it on GitHub
<#111752 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APRMUB57ADKHWWNICWBAKFTZ27Z6RAVCNFSM6AAAAABPVHZ3QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBXG4ZDAOBXGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I see. I don't have strong preference although it would be nice to use some function that exists. |
|
|
|
Makes sense, thanks.
…On Fri, Oct 11, 2024 at 9:49 AM Nikita Popov ***@***.***> wrote:
test_demangle should definitely *not* be updated when renaming functions.
—
Reply to this email directly, view it on GitHub
<#111752 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APRMUB3GAU722IHUURF5FWTZ276QPAVCNFSM6AAAAABPVHZ3QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBXG44DGNRXGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Could you please explain a bit more since this is new to me? Thanks in advance! |
The use of LLVM symbols in this file is incidental -- it's just a data set of mangled C++ symbol names. The file should only change if the mangling implementation changes. |
OK. I see. Yeah, makes senses now as it reduces maintenance cost. Thanks for the explanation. |
|
for such a wide-reaching change, it would have been nice to add the new function in one commit, update users in other commits, then remove the old function in one last commit. makes it easier to revert the removal of the old function, and makes it easier for out of tree users to to migrate |
|
You’re right. Though I am hoping to add a new function with that name and a
different behavior. But I can see how this might be problematic for out of
tree backends as well as that code will compile fine but might fail at
runtime with the new function. I can call the function “findDeclaration”
instead of “getDeclaration” and then do a rename later down the road
(following the recipe you suggested)
…On Fri, Oct 11, 2024 at 2:10 PM Arthur Eubanks ***@***.***> wrote:
for such a wide-reaching change, it would have been nice to add the new
function in one commit, update users in other commits, then remove the old
function in one last commit. makes it easier to revert the removal of the
old function, and makes it easier for out of tree users to to migrate
—
Reply to this email directly, view it on GitHub
<#111752 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APRMUB7TBNWKFUDORGE7CU3Z3A5ETAVCNFSM6AAAAABPVHZ3QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYGEZDIMJXG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
TBH I am wondering if we should revert this rename. I don't think it's a good idea to reuse getDeclaration with substantially different semantics, and if we're not reusing it, then there's not much point to rename... Instead of having getOrInsertDeclaration + getDeclaration with a meaning inversion, we can have getDeclaration and getDeclarationIfExists without the inversion. |
|
That’s a possibility. I was trying to keep the naming consistent with
similar functions in the Module class. But if folks feel its too much
churn and are ok with the inconsistent naming convention, I am fine with
reverting it and going with the naming scheme proposed above.
…On Fri, Oct 11, 2024 at 2:51 PM Nikita Popov ***@***.***> wrote:
TBH I am wondering if we should revert this rename. I don't think it's a
good idea to reuse getDeclaration with substantially different semantics,
and if we're not reusing it, then there's not much point to rename...
Instead of having getOrInsertDeclaration + getDeclaration with a meaning
inversion, we can have getDeclaration and getDeclarationIfExists without
the inversion.
—
Reply to this email directly, view it on GitHub
<#111752 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APRMUB7YY3H7PL3DQAD3MOTZ3BB57AVCNFSM6AAAAABPVHZ3QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYGE3DIMRVGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Btw, the findDeclaration was a temporary state and the plan is to switch to
getDeclaration eventually. The findDeclaration was for staging the rename.
…On Fri, Oct 11, 2024 at 2:55 PM Rahul Joshi ***@***.***> wrote:
That’s a possibility. I was trying to keep the naming consistent with
similar functions in the Module class. But if folks feel its too much
churn and are ok with the inconsistent naming convention, I am fine with
reverting it and going with the naming scheme proposed above.
On Fri, Oct 11, 2024 at 2:51 PM Nikita Popov ***@***.***>
wrote:
> TBH I am wondering if we should revert this rename. I don't think it's a
> good idea to reuse getDeclaration with substantially different semantics,
> and if we're not reusing it, then there's not much point to rename...
>
> Instead of having getOrInsertDeclaration + getDeclaration with a meaning
> inversion, we can have getDeclaration and getDeclarationIfExists without
> the inversion.
>
> —
> Reply to this email directly, view it on GitHub
> <#111752 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/APRMUB7YY3H7PL3DQAD3MOTZ3BB57AVCNFSM6AAAAABPVHZ3QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYGE3DIMRVGU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
I'd like to revert this change, or just add |
|
I will add getDeclaration back and mark it deprecated. What’s the timeframe
for deprecation thought? A couple of weeks? Because I do want to add back
the getDeclaration with different semantics. Unless we go with @nikic’s
recommendation of getDeclarationIfExists. I am going to wait for @nikic’s
response before any more changes here.
…On Sat, Oct 12, 2024 at 6:53 PM Yingwei Zheng ***@***.***> wrote:
TBH I am wondering if we should revert this rename. I don't think it's a
good idea to reuse getDeclaration with substantially different semantics,
and if we're not reusing it, then there's not much point to rename...
Instead of having getOrInsertDeclaration + getDeclaration with a meaning
inversion, we can have getDeclaration and getDeclarationIfExists without
the inversion.
I'd like to revert this change, or just add Intrinsic::getDeclaration
back and mark it as deprecated. I have encountered a build error on Alive2.
—
Reply to this email directly, view it on GitHub
<#111752 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APRMUB5DATS3NVLAPVXRNITZ3HHANAVCNFSM6AAAAABPVHZ3QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYG44DANRYGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
To reuse the name with different semantics, I'd put removal and reuse in different releases, so it takes about half a year. (Deprecation itself can be very short-lived.) |
|
Thanks. So it seems we have 2 ways forwards:
1. Add back getDeclaration, marked deprecated, and then delete it in say
2-3 weeks, and then reintroduce it with different semantics 6 months down
the road. Meanwhile, add findDeclaration as an alias, start using it, and
then 6 months down the road, we will have getDeclaration as well, so
another 6 months to finally remove findDeclaration. I find that
surprisingly prolonged process for C++ API that LLVM has not stability
guarantees even on a day to day basis.
2. Revert the change, and go back to your earlier proposal of
getDeclaration and getDeclarationIfExists, and live with lack of naming
consistency between Module:: and Intrinsic:: members that do similar things.
Does that sound right?
…On Sun, Oct 13, 2024 at 5:02 AM Nikita Popov ***@***.***> wrote:
To reuse the name with different semantics, I'd put removal and reuse in
different releases, so it takes about half a year. (Deprecation itself can
be very short-lived.)
—
Reply to this email directly, view it on GitHub
<#111752 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APRMUB7YRNGO2MYJETG3DXLZ3JOL7AVCNFSM6AAAAABPVHZ3QGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYHE2TANZWHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
We have downstream code that used |
Yes, that is the proposed plan, but not yet signed off on by folks. The temporary step is so that other out-of-tree code that uses getDeclaration had ample time window to fail building and moving to getOrInsertDeclaration before the new getDeclaration with new semantics is added. |
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
…lvm#111752) Rename the function to reflect its correct behavior and to be consistent with `Module::getOrInsertFunction`. This is also in preparation of adding a new `Intrinsic::getDeclaration` that will have behavior similar to `Module::getFunction` (i.e, just lookup, no creation).
bricknerb
pushed a commit
to bricknerb/llvm-project
that referenced
this pull request
Oct 17, 2024
…lvm#111752) Rename the function to reflect its correct behavior and to be consistent with `Module::getOrInsertFunction`. This is also in preparation of adding a new `Intrinsic::getDeclaration` that will have behavior similar to `Module::getFunction` (i.e, just lookup, no creation).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rename the function to reflect its correct behavior and to be consistent with
Module::getOrInsertFunction. This is also in preparation of adding a newIntrinsic::getDeclarationthat will have behavior similar toModule::getFunction(i.e, just lookup, no creation).