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

Remove some only- clauses from mir-opt tests #122645

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

saethlin
Copy link
Member

Derived from #122295

Many of these tests were originally codegen tests, and MIR is more trivially portable than LLVM IR. We simply don't need to restrict the platform in most cases.

r? Nadrieril

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2024
@Nadrieril
Copy link
Member

nice!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 17, 2024

📌 Commit 6828f0c has been approved by Nadrieril

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 Mar 17, 2024
@Nadrieril
Copy link
Member

Nadrieril commented Mar 17, 2024

ah wait, this could fail in rollup since bors tries more targets than PR CI does

@bors rollup=maybe

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 17, 2024
…=Nadrieril

Remove some only- clauses from mir-opt tests

Derived from rust-lang#122295

Many of these tests were originally codegen tests, and MIR is more trivially portable than LLVM IR. We simply don't need to restrict the platform in most cases.

r? Nadrieril
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 17, 2024
…=Nadrieril

Remove some only- clauses from mir-opt tests

Derived from rust-lang#122295

Many of these tests were originally codegen tests, and MIR is more trivially portable than LLVM IR. We simply don't need to restrict the platform in most cases.

r? Nadrieril
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#121652 (Detect when move of !Copy value occurs within loop and should likely not be cloned)
 - rust-lang#122639 (Fix typos)
 - rust-lang#122645 (Remove some only- clauses from mir-opt tests)
 - rust-lang#122654 (interpret/memory: explain why we use == on bool)
 - rust-lang#122656 (simplify_cfg: rename some passes so that they make more sense)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors rollup=iffy since queue is basically empty right now

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2024
…adrieril

Remove some only- clauses from mir-opt tests

Derived from rust-lang#122295

Many of these tests were originally codegen tests, and MIR is more trivially portable than LLVM IR. We simply don't need to restrict the platform in most cases.

r? Nadrieril
@bors
Copy link
Contributor

bors commented Mar 18, 2024

⌛ Testing commit 6828f0c with merge 19acca7...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 18, 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 Mar 18, 2024
@Nadrieril
Copy link
Member

Well, that explains it :D

@Nadrieril
Copy link
Member

Nadrieril commented Mar 18, 2024

The targets list for --bless is currently somewhat arbitrary. We could choose only targets with asm support? (and change that one test to run under these targets)

@saethlin
Copy link
Member Author

The bless list already only contains targets with asm support; the test failed because we tried to run a test that needs assembly support on a target that doesn't have it.

I've made the change that I think makes the most sense here, but I'm going to run the test-various Docker image locally before I re-apply your approval.

@saethlin
Copy link
Member Author

@bors r=Nadrieril

@bors
Copy link
Contributor

bors commented Mar 18, 2024

📌 Commit 68f284f has been approved by Nadrieril

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 Mar 18, 2024
@Nadrieril
Copy link
Member

Nice, TIL about needs-asm-support

@saethlin
Copy link
Member Author

Me too. I found it by grepping through all the other compiletest directives used in the mir-opt tests.

@workingjubilee
Copy link
Member

cute.

// Note thet we intentionally still put the needs- prefix here to make the file show up when
// grepping for a directive name, even though we could technically strip that.
let needs = &[
Need {
name: "needs-asm-support",
condition: config.has_asm_support(),
ignore_reason: "ignored on targets without inline assembly support",
},
Need {
name: "needs-sanitizer-support",
condition: cache.sanitizer_support,
ignore_reason: "ignored on targets without sanitizers support",
},

@bors
Copy link
Contributor

bors commented Mar 19, 2024

⌛ Testing commit 68f284f with merge 91b87c4...

@bors
Copy link
Contributor

bors commented Mar 19, 2024

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing 91b87c4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 19, 2024
@bors bors merged commit 91b87c4 into rust-lang:master Mar 19, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 19, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (91b87c4): 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

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)
-4.1% [-4.6%, -3.7%] 2
Improvements ✅
(secondary)
-2.4% [-3.5%, -0.9%] 4
All ❌✅ (primary) -4.1% [-4.6%, -3.7%] 2

Cycles

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

Binary size

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

Bootstrap: 668.321s -> 669.426s (0.17%)
Artifact size: 312.76 MiB -> 312.79 MiB (0.01%)

@saethlin saethlin deleted the portable-mir-opt-tests branch May 3, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants