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

rustfmt tests/mir-opt #125912

Merged
merged 2 commits into from
Jun 3, 2024
Merged

Conversation

nnethercote
Copy link
Contributor

Continuing the work started in #125759. Details in individual commit log messages.

r? @oli-obk

The `mir!` macro has multiple parts:
- An optional return type annotation.
- A sequence of zero or more local declarations.
- A mandatory starting anonymous basic block, which is brace-delimited.
- A sequence of zero of more additional named basic blocks.

Some `mir!` invocations use braces with a "block" style, like so:
```
mir! {
    let _unit: ();
    {
	let non_copy = S(42);
	let ptr = std::ptr::addr_of_mut!(non_copy);
	// Inside `callee`, the first argument and `*ptr` are basically
	// aliasing places!
	Call(_unit = callee(Move(*ptr), ptr), ReturnTo(after_call), UnwindContinue())
    }
    after_call = {
	Return()
    }
}
```
Some invocations use parens with a "block" style, like so:
```
mir!(
    let x: [i32; 2];
    let one: i32;
    {
	x = [42, 43];
	one = 1;
	x = [one, 2];
	RET = Move(x);
	Return()
    }
)
```
And some invocations uses parens with a "tighter" style, like so:
```
mir!({
    SetDiscriminant(*b, 0);
    Return()
})
```
This last style is generally used for cases where just the mandatory
starting basic block is present. Its braces are placed next to the
parens.

This commit changes all `mir!` invocations to use braces with a "block"
style. Why?

- Consistency is good.

- The contents of the invocation is a block of code, so it's odd to use
  parens. They are more normally used for function-like macros.

- Most importantly, the next commit will enable rustfmt for
  `tests/mir-opt/`. rustfmt is more aggressive about formatting macros
  that use parens than macros that use braces. Without this commit's
  changes, rustfmt would break a couple of `mir!` macro invocations that
  use braces within `tests/mir-opt` by inserting an extraneous comma.
  E.g.:
  ```
  mir!(type RET = (i32, bool);, { // extraneous comma after ';'
      RET.0 = 1;
      RET.1 = true;
      Return()
  })
  ```
  Switching those `mir!` invocations to use braces avoids that problem,
  resulting in this, which is nicer to read as well as being valid
  syntax:
  ```
  mir! {
      type RET = (i32, bool);
      {
	  RET.0 = 1;
	  RET.1 = true;
	  Return()
      }
  }
  ```
@rustbot rustbot added A-meta Area: Issues about the rust-lang/rust repository. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

The Miri subtree was changed

cc @rust-lang/miri

The only non-obvious changes:
- `building/storage_live_dead_in_statics.rs` has a `#[rustfmt::skip]`
  attribute to avoid reformating a table of data.
- Two `.mir` files have slight changes involving line numbers.
- In `unusual_item_types.rs` an `EMIT_MIR` annotation is moved to
  outside a function, which is the usual spot, because `tidy` complains
  if such a comment is indented.

The commit also tweaks the comments in `rustfmt.toml`.
@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2024

@bors r+ p=1 bitrotty

@bors
Copy link
Contributor

bors commented Jun 3, 2024

📌 Commit c9c80d2 has been approved by oli-obk

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 Jun 3, 2024
@bors
Copy link
Contributor

bors commented Jun 3, 2024

⌛ Testing commit c9c80d2 with merge 8768db9...

@bors
Copy link
Contributor

bors commented Jun 3, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 8768db9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2024
@bors bors merged commit 8768db9 into rust-lang:master Jun 3, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 3, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8768db9): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was 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)
-0.6% [-1.3%, -0.2%] 5
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 0.4%, secondary 3.9%)

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)
2.0% [2.0%, 2.1%] 2
Regressions ❌
(secondary)
3.9% [3.1%, 5.0%] 4
Improvements ✅
(primary)
-2.9% [-2.9%, -2.9%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [-2.9%, 2.1%] 3

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: 669.197s -> 667.912s (-0.19%)
Artifact size: 318.98 MiB -> 318.98 MiB (0.00%)

@nnethercote nnethercote deleted the rustfmt-tests-mir-opt branch June 3, 2024 22:58
@nnethercote nnethercote mentioned this pull request Jun 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
rustfmt more tests

This finishes the formatting of tests begun in rust-lang#125759 and continued in rust-lang#125912.

r? `@lqd`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues about the rust-lang/rust repository. 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. T-libs Relevant to the library 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