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.

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

Reuse LLVMConstInBoundsGEP2 #113523

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

workingjubilee
Copy link
Contributor

We have had LLVM 14 as our minimum for a bit now.

We have had LLVM 14 as our minimum for a bit now.
@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@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 Jul 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2023

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@workingjubilee
Copy link
Contributor Author

This binds against an already-present definition, I don't believe bumping the LLVM CI stamp is appropriate, if I understand things correctly?

@@ -1616,17 +1616,6 @@ extern "C" void LLVMRustSetLinkage(LLVMValueRef V,
LLVMSetLinkage(V, fromRust(RustLinkage));
}

// FIXME: replace with LLVMConstInBoundsGEP2 when bumped minimal version to llvm-14
Copy link
Contributor

Choose a reason for hiding this comment

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

At time when i placed this comment i didn't realized that current minimum llvm was 14. Is there easy to find places where it noted?

Copy link
Member

Choose a reason for hiding this comment

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

The easiest thing to see is the PR job that runs on the minimum, currently x86_64-gnu-llvm-14.

We also mention updates in RELEASES.md, and the actual enforcement is here:

rust/src/bootstrap/llvm.rs

Lines 519 to 533 in fd68a6d

fn check_llvm_version(builder: &Builder<'_>, llvm_config: &Path) {
if builder.config.dry_run() {
return;
}
let mut cmd = Command::new(llvm_config);
let version = output(cmd.arg("--version"));
let mut parts = version.split('.').take(2).filter_map(|s| s.parse::<u32>().ok());
if let (Some(major), Some(_minor)) = (parts.next(), parts.next()) {
if major >= 14 {
return;
}
}
panic!("\n\nbad LLVM version: {}, need >=14.0\n\n", version)
}

@cuviper
Copy link
Member

cuviper commented Jul 10, 2023

@bors r+

This binds against an already-present definition, I don't believe bumping the LLVM CI stamp is appropriate, if I understand things correctly?

Yeah, the bot is too aggressive, set for anything in compiler/rustc_llvm/llvm-wrapper which is really after the fact.

@bors
Copy link
Contributor

bors commented Jul 10, 2023

📌 Commit 0726c78 has been approved by cuviper

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 Jul 10, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 10, 2023
…s-gep2, r=cuviper

Reuse LLVMConstInBoundsGEP2

We have had LLVM 14 as our minimum for a bit now.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 10, 2023
…s-gep2, r=cuviper

Reuse LLVMConstInBoundsGEP2

We have had LLVM 14 as our minimum for a bit now.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 11, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#113386 (style-guide: Expand example of combinable expressions to include arrays)
 - rust-lang#113523 (Reuse LLVMConstInBoundsGEP2)
 - rust-lang#113528 (Dynamically size sigaltstk in rustc)
 - rust-lang#113543 (Remove `rustc_llvm` from llvm-stamp nags)
 - rust-lang#113548 (Update books)
 - rust-lang#113551 (bootstrap: Don't print "Skipping" twice)
 - rust-lang#113556 (Don't use serde-derive in the rls shim)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 92a1018 into rust-lang:master Jul 11, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 11, 2023
@workingjubilee workingjubilee deleted the reuse-const-inbounds-gep2 branch July 31, 2023 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants