[NFC] Rename Intrinsic::getDeclaration to getOrInsertDeclaration#111752
Merged
Conversation
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).
6cab160 to
b4e9093
Compare
|
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 |
Contributor
|
@jurahul |
Contributor
Author
|
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:
***@***.***>
|
Contributor
|
I see. I don't have strong preference although it would be nice to use some function that exists. |
Contributor
|
|
Contributor
Author
|
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:
***@***.***>
|
Contributor
Could you please explain a bit more since this is new to me? Thanks in advance! |
Contributor
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. |
Contributor
OK. I see. Yeah, makes senses now as it reduces maintenance cost. Thanks for the explanation. |
Contributor
|
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 |
Contributor
Author
|
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:
***@***.***>
|
Contributor
|
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. |
Contributor
Author
|
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:
***@***.***>
|
Contributor
Author
|
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:
> ***@***.***>
>
|
Member
I'd like to revert this change, or just add |
Contributor
Author
|
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:
***@***.***>
|
Contributor
|
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.) |
Contributor
Author
|
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:
***@***.***>
|
Contributor
|
We have downstream code that used |
Contributor
Author
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).
Ninja3047
added a commit
to lifting-bits/remill
that referenced
this pull request
Dec 16, 2025
`llvm::Instrinsic::getDeclaration` will be removed soon, deprecation started in llvm 21 llvm/llvm-project#111752 llvm/llvm-project#112242
kyle-elliott-tob
pushed a commit
to lifting-bits/remill
that referenced
this pull request
Dec 16, 2025
`llvm::Instrinsic::getDeclaration` will be removed soon, deprecation started in llvm 21 llvm/llvm-project#111752 llvm/llvm-project#112242
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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).