Skip to content

Reuse LLVMConstInBoundsGEP2#113523

Merged
bors merged 1 commit intorust-lang:masterfrom
workingjubilee:reuse-const-inbounds-gep2
Jul 11, 2023
Merged

Reuse LLVMConstInBoundsGEP2#113523
bors merged 1 commit intorust-lang:masterfrom
workingjubilee:reuse-const-inbounds-gep2

Conversation

@workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Jul 10, 2023

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

@workingjubilee
Reuse LLVMConstInBoundsGEP2
0726c78 
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
Member Author

workingjubilee commented Jul 10, 2023

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

klensy
klensy reviewed Jul 10, 2023
View reviewed changes
compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp
LLVMSetLinkage(V, fromRust(RustLinkage));
}

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

@klensy klensy Jul 10, 2023

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

@cuviper cuviper Jul 10, 2023

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
Collaborator

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
@matthiaskrgr
Rollup merge of rust-lang#113523 - workingjubilee:reuse-const-inbound…
6ed121a 
…s-gep2, r=cuviper

Reuse LLVMConstInBoundsGEP2

We have had LLVM 14 as our minimum for a bit now.
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 10, 2023
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 10, 2023
@matthiaskrgr
Rollup merge of rust-lang#113523 - workingjubilee:reuse-const-inbound…
1abf5d3 
…s-gep2, r=cuviper

Reuse LLVMConstInBoundsGEP2

We have had LLVM 14 as our minimum for a bit now.
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 10, 2023
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 10, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 11, 2023
@bors
Auto merge of rust-lang#113559 - matthiaskrgr:rollup-jrqyctc, r=matth…
fcaf04e 
…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
@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

Reviewers

@cuviper cuviper cuviper left review comments

+1 more reviewer

@klensy klensy klensy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@cuviper cuviper

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

Milestone

1.73.0

Development

Successfully merging this pull request may close these issues.

5 participants

@workingjubilee @rustbot @cuviper @bors @klensy