Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

llvm: replace some deprecated functions #130389

Merged
merged 5 commits into from
Sep 24, 2024
Merged

llvm: replace some deprecated functions #130389

merged 5 commits into from
Sep 24, 2024

Conversation

Luv-Ray
Copy link
Contributor

@Luv-Ray Luv-Ray commented Sep 15, 2024 •
edited by nikic
Loading

LLVMMDStringInContext and LLVMMDNodeInContext are deprecated, replace them with LLVMMDStringInContext2 and LLVMMDNodeInContext2.
Also replace Value with Metadata in some function signatures for better consistency.

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 15, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@klensy klensy mentioned this pull request Sep 18, 2024
Closed
@bors
Copy link
Contributor

bors commented Sep 18, 2024

☔ The latest upstream changes (presumably #130519) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
Copy link
Contributor

I unintentionally duplicated this effort in #130503. My PR looked extremley similar to this one, although I didn't add Metadata to BackendTypes. Anyway, looks good to me.

r? @nikic

@rustbot rustbot assigned nikic and unassigned wesleywiser Sep 18, 2024
@Luv-Ray Luv-Ray force-pushed the LLVMMDNodeInContext2 branch from 475d65c to b7c5656 Compare September 19, 2024 01:39
nikic
nikic reviewed Sep 19, 2024
View reviewed changes
compiler/rustc_codegen_llvm/src/builder.rs Outdated
);
let md = llvm::LLVMMDNodeInContext2(self.cx.llcx, v.as_ptr(), v.len());
let md = llvm::LLVMMetadataAsValue(&self.llcx, md);
llvm::LLVMSetMetadata(load, llvm::MD_range as c_uint, md);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a set_metadata method that does the LLVMMetadataAsValue + LLVMSetMetadata dance?

LLVM should really allow setting the metadata without first wrapping it in a value and then unwrapping again, but we can at least hide this...

nikic
nikic reviewed Sep 19, 2024
View reviewed changes
compiler/rustc_codegen_llvm/src/context.rs Outdated
@@ -586,6 +586,14 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
llvm::LLVMSetSection(g, c"llvm.metadata".as_ptr());
}
}

/// A wrapper for [`llvm::LLVMSetMetadata`], but it takes `Metadata` as a parameter instead of `Value`.
pub(crate) fn set_metadata<'a>(&self, val: &'a Value, kind_id: c_uint, md: &'a Metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accept MetadataType here and move the c_uint cast into the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

@nikic
Copy link
Contributor

nikic commented Sep 19, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 19, 2024

📌 Commit 6da2d6e has been approved by nikic

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2024
@bors
Copy link
Contributor

bors commented Sep 19, 2024

⌛ Testing commit 6da2d6e with merge dede102...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2024
@bors
Auto merge of rust-lang#130389 - Luv-Ray:LLVMMDNodeInContext2, r=nikic
dede102 
llvm: replace some deprecated functions

`LLVMMDStringInContext` and `LLVMMDNodeInContext` are deprecated, replace them with `LLVMMDStringInContext2` and `LLVMMDNodeInContext2`.
Also replace `Value` with `Metadata` in some function signatures for better consistency.
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 19, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 19, 2024
@Luv-Ray
Copy link
Contributor Author

Luv-Ray commented Sep 19, 2024

I can't reproduce the error with ./x.py --stage 1 test tests/codegen/issues/issue-111508-vec-tryinto-array.rs on my local machine...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2024
@bors
Auto merge of rust-lang#130389 - Luv-Ray:LLVMMDNodeInContext2, r=<try>
563c5ff 
llvm: replace some deprecated functions

`LLVMMDStringInContext` and `LLVMMDNodeInContext` are deprecated, replace them with `LLVMMDStringInContext2` and `LLVMMDNodeInContext2`.
Also replace `Value` with `Metadata` in some function signatures for better consistency.

try-job: x86_64-gnu-llvm-19
@nikic
Copy link
Contributor

nikic commented Sep 20, 2024

Ah crap, I didn't realize that bors retry also retires try builds. I'll just let it run to avoid confusing bors further.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 20, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2024
@nikic
Copy link
Contributor

nikic commented Sep 20, 2024

Huh, and now it failed again. So probably something is legitimately wrong with this patch that makes things break non-deterministically -- but I'm not able to spot it.

This was referenced Sep 21, 2024
Closed
Merged
@nikic
Copy link
Contributor

nikic commented Sep 21, 2024

Okay, this test has been failing on some other PRs as well, I filed #130656.

@nikic
Copy link
Contributor

nikic commented Sep 23, 2024

Problematic test has been removed, try again...

@bors r+

@bors
Copy link
Contributor

bors commented Sep 23, 2024

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Sep 23, 2024

📌 Commit 6da2d6e has been approved by nikic

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 23, 2024
@Luv-Ray
Copy link
Contributor Author

Luv-Ray commented Sep 23, 2024

seems that it needs to run retry

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 23, 2024
@workingjubilee
Rollup merge of rust-lang#130389 - Luv-Ray:LLVMMDNodeInContext2, r=nikic
cda7e35 
llvm: replace some deprecated functions

`LLVMMDStringInContext` and `LLVMMDNodeInContext` are deprecated, replace them with `LLVMMDStringInContext2` and `LLVMMDNodeInContext2`.
Also replace `Value` with `Metadata` in some function signatures for better consistency.
@workingjubilee workingjubilee mentioned this pull request Sep 23, 2024
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee
Copy link
Member

@bors r=nikic

@bors
Copy link
Contributor

bors commented Sep 24, 2024

📌 Commit d7ebf9e has been approved by nikic

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 24, 2024

⌛ Testing commit d7ebf9e with merge 4cbfcf1...

@bors
Copy link
Contributor

bors commented Sep 24, 2024

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 4cbfcf1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2024
@bors bors merged commit 4cbfcf1 into rust-lang:master Sep 24, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 24, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4cbfcf1): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary -3.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.0% [-3.0%, -3.0%] 1
All ❌✅ (primary) - - 0

Cycles

Results (primary 1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.8% [1.8%, 1.8%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.8% [1.8%, 1.8%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 770.84s -> 769.56s (-0.17%)
Artifact size: 340.88 MiB -> 340.89 MiB (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@nikic nikic nikic left review comments

Assignees

@nikic nikic

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
10 participants
@Luv-Ray @rustbot @rust-log-analyzer @bors @nnethercote @nikic @workingjubilee @rust-timer @bjorn3 @wesleywiser