Added "copy" to Debug fmt for copy operands #122551

RayMuir · 2024-03-15T14:37:37Z

In MIR's debug mode (--emit mir) the printing for Operands is slightly inconsistent.

The RValues - values on the right side of an Assign - are usually printed with their Operand when they are Places.

Example:
_2 = move _3

But for arguments, the operand is omitted.

_2 = _1

I propose a change be made, to display the place with the operand.

_2 = copy _1

Move and copy have different semantics, meaning this difference is important and helpful to the user. It also adds consistency to the pretty printing.

-- EDIT --

Consider this example Rust program and its MIR output with the updated pretty printer.

This was generated with the arguments --emit mir --crate-type lib -Zmir-opt-level=0 (Otherwise, it's optimised away since it's a junk program).

fn main(foo: i32) {
    let v = 10;

    if v == 20 {
        foo;
    }
    else {
        v;
    }
}

// WARNING: This output format is intended for human consumers only
// and is subject to change without notice. Knock yourself out.
fn main(_1: i32) -> () {
    debug foo => _1;
    let mut _0: ();
    let _2: i32;
    let mut _3: bool;
    let mut _4: i32;
    let _5: i32;
    let _6: i32;
    scope 1 {
        debug v => _2;
    }

    bb0: {
        StorageLive(_2);
        _2 = const 10_i32;
        StorageLive(_3);
        StorageLive(_4);
        _4 = copy _2;
        _3 = Eq(move _4, const 20_i32);
        switchInt(move _3) -> [0: bb2, otherwise: bb1];
    }

    bb1: {
        StorageDead(_4);
        StorageLive(_5);
        _5 = copy _1;
        StorageDead(_5);
        _0 = const ();
        goto -> bb3;
    }

    bb2: {
        StorageDead(_4);
        StorageLive(_6);
        _6 = copy _2;
        StorageDead(_6);
        _0 = const ();
        goto -> bb3;
    }

    bb3: {
        StorageDead(_3);
        StorageDead(_2);
        return;
    }
}

In this example program, we can see that when we move a place, it is preceded by "move". e.g. _3 = Eq(move _4, const 20_i32);. However, when we copy a place such as _5 = _1;, it is not preceded by the operand in the original printout. I propose to change the print to include the copy _5 = copy _1 as in this example.

Regarding the arguments part. When I originally submitted this PR, I was under the impression this only affected the print for arguments to a function, but actually, it affects anything that uses a copy. This is preferable anyway with regard to consistency. The PR is about making copy explicit.

rustbot · 2024-03-15T14:37:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

pnkfelix · 2024-03-15T16:39:49Z

I'm not sure this adds any actual value; the people who are reading the MIR output probably are already well aware of the semantics here?

But maybe making the pretty-printing line up has value. I certainly have valued assembly languages that chose the same number of letters in their opcodes (or at least a small upper bound to their lengh) for this exact reason.

Who are the people who would care most about whether or not to make this change? The people in @rust-lang/wg-mir-opt ?

pnkfelix · 2024-03-15T16:40:50Z

I'm going to reassign this to @oli-obk for now because I think they are in a better position to make a call about the value of a change like this.

r? @oli-obk

compiler-errors · 2024-03-15T20:26:22Z

@pnkfelix:

I'm not sure this adds any actual value; the people who are reading the MIR output probably are already well aware of the semantics here?

I think this adds value.

MIR is read by both MIR experts and non-expert users, and unless a non-experts reads the rustc pretty-printing code, or they reason by elimination, or someone teaches them that non-marked operands are always copy, I don't see how one transitions from "not knowing this" to "knowing this".

If anything, not marking the move operands and always marking copy operands seems more obvious, but why do we care about brevity here?

oli-obk · 2024-03-18T09:22:27Z

I'm moderately in favor of this. I think even tho it's obvious to me I'dve benefited from it when the difference was especially important.

let's leave this open for a week to let other @rust-lang/wg-mir-opt folk chime in. If there are no explanations of why we should keep the status quo or do something else entirely in that week, let's bless everything and land it.

DianQK · 2024-03-18T10:00:00Z

LGTM. But do we also need to update custom_mir?

apiraino · 2024-06-20T13:45:58Z

@RayMuir cc for this comment. Apart from that are we good for merging this?

thanks

@rustbot author

scottmcm · 2024-06-20T15:42:36Z

I'm a fan of this. Emphasizing the place-vs-operand distinction always would be nice (there's no way to see in the MIR that Len(_1) is on a place), and even for code where I know exactly what's happening, seeing (for example) ptr::read/write giving *_1 = move _2 vs _2 = copy *_1 is important enough that having both copy and move clearly visible sounds worth any extra costs from it being overall longer.

(Maybe do the test updates in a separate commit, though, so we can ignore it in the blame history?)

RayMuir · 2024-07-11T08:57:17Z

LGTM. But do we also need to update custom_mir?

That may need looking into as I don't know off the top of my head.

Is there anything more I'm required to do before this is accepted (this is my first pull request).

apiraino · 2024-07-11T09:20:27Z

cc @DianQK @RalfJung thanks :-)

@rustbot review

RalfJung · 2024-07-11T09:23:00Z

I am confused by the PR description.

But for arguments, the operand is omitted.

What "arguments"? Do you mean function arguments? But then the rest of the text seems to apply to all uses of Operand, not just (function) arguments. And the example does not involve an argument as far as I can see. What do "arguments" have to do with copy vs move anyway?

So not sure what is actually being proposed here. It seems to just be about making copy explicit rather than implicit, but the description seems to talk about something completely different.

RalfJung · 2024-07-11T09:25:01Z

It would be good if you could run ./x.py test mir-opt --bless and add that to the PR, so that one can have an idea for what effect this PR will have.

DianQK · 2024-07-11T09:52:49Z

LGTM. But do we also need to update custom_mir?

That may need looking into as I don't know off the top of my head.

Is there anything more I'm required to do before this is accepted (this is my first pull request).

For me, just bless all the test cases. We don't often use custom_mir. It's not very important.

RayMuir · 2024-08-13T15:58:19Z

I am confused by the PR description.

But for arguments, the operand is omitted.

What "arguments"? Do you mean function arguments? But then the rest of the text seems to apply to all uses of Operand, not just (function) arguments. And the example does not involve an argument as far as I can see. What do "arguments" have to do with copy vs move anyway?

So not sure what is actually being proposed here. It seems to just be about making copy explicit rather than implicit, but the description seems to talk about something completely different.

Apologies, hopefully this makes the PR a little clearer. Consider this example Rust program and its MIR output with the updated pretty printer.

This was generated with the arguments --emit mir --crate-type lib -Zmir-opt-level=0 (Otherwise it's optimised away since it's a junk program).

fn main(foo: i32) {
    let v = 10;

    if v == 20 {
        foo;
    }
    else {
        v;
    }
}

// WARNING: This output format is intended for human consumers only
// and is subject to change without notice. Knock yourself out.
fn main(_1: i32) -> () {
    debug foo => _1;
    let mut _0: ();
    let _2: i32;
    let mut _3: bool;
    let mut _4: i32;
    let _5: i32;
    let _6: i32;
    scope 1 {
        debug v => _2;
    }

    bb0: {
        StorageLive(_2);
        _2 = const 10_i32;
        StorageLive(_3);
        StorageLive(_4);
        _4 = copy _2;
        _3 = Eq(move _4, const 20_i32);
        switchInt(move _3) -> [0: bb2, otherwise: bb1];
    }

    bb1: {
        StorageDead(_4);
        StorageLive(_5);
        _5 = copy _1;
        StorageDead(_5);
        _0 = const ();
        goto -> bb3;
    }

    bb2: {
        StorageDead(_4);
        StorageLive(_6);
        _6 = copy _2;
        StorageDead(_6);
        _0 = const ();
        goto -> bb3;
    }

    bb3: {
        StorageDead(_3);
        StorageDead(_2);
        return;
    }
}

In this example program, we can see that when we move a place, it is preceded by "move". e.g. _3 = Eq(move _4, const 20_i32);. However, when we copy a place such as _5 = _1;, it is not preceded by the operand in the original printout. I propose to change the print to include the copy _5 = copy _1 as in this example.

Regarding the arguments part. When I originally submitted this PR, I was under the impression this only affected the print for arguments to a function, but actually, it affects anything that uses a copy. This is preferable anyway with regard to consistency. The PR is about making copy explicit.

Hope this clears the PR up.

RayMuir · 2024-08-13T15:59:14Z

It would be good if you could run ./x.py test mir-opt --bless and add that to the PR, so that one can have an idea for what effect this PR will have.

I ran the test and have attached it as the output was quite long.
out.txt

RalfJung · 2024-08-13T16:30:36Z

The interesting part of a --bless run is not the output, it's what it does to the files. :) Please git commit tests -m "bless tests" after the command finishes, and push that to this branch -- then the PR will contain a diff of how your PR changes MIR printing.

RalfJung · 2024-08-13T16:32:00Z

Regarding the arguments part. When I originally submitted this PR, I was under the impression this only affected the print for arguments to a function, but actually, it affects anything that uses a copy. This is preferable anyway with regard to consistency. The PR is about making copy explicit.

That helps, thanks. :) Please update the PR description so that it reflects your current understanding and the current status of the PR.

oli-obk · 2024-08-17T08:30:48Z

r? mir-opt

saethlin · 2024-08-18T23:44:23Z

I approve of the contents of the commits, though I'd expect some conversation with the first-time contributor before taking over their PR.

RayMuir · 2024-08-19T15:17:33Z

I approve of the contents of the commits, though I'd expect some conversation with the first-time contributor before taking over their PR.

Thanks for the detailed explanation of the process above, it was very helpful. It's certainly a bit more advanced than I'm used to, but I have some colleagues who are a bit more experienced with Git and its processes, which is lucky. I think from the PR's history that someone has done what was asked? If so, then great. Otherwise, let me know and I'll do the bit that was originally asked. Thanks for the patience!

saethlin · 2024-08-19T15:22:15Z

If so, then great.

So shall it be!

@bors r+ rollup=never p=1

bors · 2024-08-19T15:22:17Z

📌 Commit b9cffa7 has been approved by saethlin

It is now in the queue for this repository.

bors · 2024-08-19T16:31:28Z

⌛ Testing commit b9cffa7 with merge b7e4e42...

Added "copy" to Debug fmt for copy operands In MIR's debug mode (--emit mir) the printing for Operands is slightly inconsistent. The RValues - values on the right side of an Assign - are usually printed with their Operand when they are Places. Example: _2 = move _3 But for arguments, the operand is omitted. _2 = _1 I propose a change be made, to display the place with the operand. _2 = copy _1 Move and copy have different semantics, meaning this difference is important and helpful to the user. It also adds consistency to the pretty printing. -- EDIT -- Consider this example Rust program and its MIR output with the **updated pretty printer.** This was generated with the arguments --emit mir --crate-type lib -Zmir-opt-level=0 (Otherwise, it's optimised away since it's a junk program). ```rust fn main(foo: i32) { let v = 10; if v == 20 { foo; } else { v; } } ``` ```MIR // WARNING: This output format is intended for human consumers only // and is subject to change without notice. Knock yourself out. fn main(_1: i32) -> () { debug foo => _1; let mut _0: (); let _2: i32; let mut _3: bool; let mut _4: i32; let _5: i32; let _6: i32; scope 1 { debug v => _2; } bb0: { StorageLive(_2); _2 = const 10_i32; StorageLive(_3); StorageLive(_4); _4 = copy _2; _3 = Eq(move _4, const 20_i32); switchInt(move _3) -> [0: bb2, otherwise: bb1]; } bb1: { StorageDead(_4); StorageLive(_5); _5 = copy _1; StorageDead(_5); _0 = const (); goto -> bb3; } bb2: { StorageDead(_4); StorageLive(_6); _6 = copy _2; StorageDead(_6); _0 = const (); goto -> bb3; } bb3: { StorageDead(_3); StorageDead(_2); return; } } ``` In this example program, we can see that when we move a place, it is preceded by "move". e.g. ``` _3 = Eq(move _4, const 20_i32);```. However, when we copy a place such as ```_5 = _1;```, it is not preceded by the operand in the original printout. I propose to change the print to include the copy ```_5 = copy _1``` as in this example. Regarding the arguments part. When I originally submitted this PR, I was under the impression this only affected the print for arguments to a function, but actually, it affects anything that uses a copy. This is preferable anyway with regard to consistency. The PR is about making ```copy``` explicit.

rust-log-analyzer · 2024-08-19T18:13:15Z

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] miri test:false 4.509
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Mon, Aug 19, 2024  6:12:58 PM
  network time: Mon, 19 Aug 2024 18:12:59 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

bors · 2024-08-19T18:13:54Z

💔 Test failed - checks-actions

saethlin · 2024-08-19T18:19:20Z

@bors retry

bors · 2024-08-19T23:10:50Z

⌛ Testing commit b9cffa7 with merge 79611d9...

bors · 2024-08-20T01:39:36Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 79611d9 to master...

rust-timer · 2024-08-20T02:54:50Z

Finished benchmarking commit (79611d9): 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)

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

Cycles

Results (secondary 2.1%)

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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 751.143s -> 749.461s (-0.22%)
Artifact size: 338.64 MiB -> 338.73 MiB (0.03%)

rustbot assigned pnkfelix Mar 15, 2024

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 Mar 15, 2024

This comment has been minimized.

Sign in to view

rustbot assigned oli-obk and unassigned pnkfelix Mar 15, 2024

rustbot 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 Jun 20, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 11, 2024

apiraino 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 Jul 18, 2024

Exclude the copy blessing from git blame

b9cffa7

scottmcm force-pushed the copy_fmt branch from ebbccc6 to b9cffa7 Compare August 18, 2024 23:18

rustbot added the A-meta Area: Issues & PRs about the rust-lang/rust repository itself label Aug 18, 2024

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 Aug 19, 2024

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 Aug 19, 2024

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 Aug 19, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 20, 2024

bors merged commit 79611d9 into rust-lang:master Aug 20, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added "copy" to Debug fmt for copy operands #122551

Added "copy" to Debug fmt for copy operands #122551

RayMuir commented Mar 15, 2024 •

edited

Loading

rustbot commented Mar 15, 2024

This comment has been minimized.

pnkfelix commented Mar 15, 2024

pnkfelix commented Mar 15, 2024

compiler-errors commented Mar 15, 2024

oli-obk commented Mar 18, 2024

DianQK commented Mar 18, 2024

apiraino commented Jun 20, 2024

scottmcm commented Jun 20, 2024 •

edited

Loading

RayMuir commented Jul 11, 2024 •

edited

Loading

apiraino commented Jul 11, 2024

RalfJung commented Jul 11, 2024 •

edited

Loading

RalfJung commented Jul 11, 2024

DianQK commented Jul 11, 2024

RayMuir commented Aug 13, 2024

RayMuir commented Aug 13, 2024

RalfJung commented Aug 13, 2024

RalfJung commented Aug 13, 2024

oli-obk commented Aug 17, 2024

saethlin commented Aug 18, 2024

RayMuir commented Aug 19, 2024

saethlin commented Aug 19, 2024

bors commented Aug 19, 2024

bors commented Aug 19, 2024

rust-log-analyzer commented Aug 19, 2024

bors commented Aug 19, 2024

saethlin commented Aug 19, 2024

bors commented Aug 19, 2024

bors commented Aug 20, 2024

rust-timer commented Aug 20, 2024

Added "copy" to Debug fmt for copy operands #122551

Added "copy" to Debug fmt for copy operands #122551

Conversation

RayMuir commented Mar 15, 2024 • edited Loading

rustbot commented Mar 15, 2024

This comment has been minimized.

pnkfelix commented Mar 15, 2024

pnkfelix commented Mar 15, 2024

compiler-errors commented Mar 15, 2024

oli-obk commented Mar 18, 2024

DianQK commented Mar 18, 2024

apiraino commented Jun 20, 2024

scottmcm commented Jun 20, 2024 • edited Loading

RayMuir commented Jul 11, 2024 • edited Loading

apiraino commented Jul 11, 2024

RalfJung commented Jul 11, 2024 • edited Loading

RalfJung commented Jul 11, 2024

DianQK commented Jul 11, 2024

RayMuir commented Aug 13, 2024

RayMuir commented Aug 13, 2024

RalfJung commented Aug 13, 2024

RalfJung commented Aug 13, 2024

oli-obk commented Aug 17, 2024

saethlin commented Aug 18, 2024

RayMuir commented Aug 19, 2024

saethlin commented Aug 19, 2024

bors commented Aug 19, 2024

bors commented Aug 19, 2024

rust-log-analyzer commented Aug 19, 2024

bors commented Aug 19, 2024

saethlin commented Aug 19, 2024

bors commented Aug 19, 2024

bors commented Aug 20, 2024

rust-timer commented Aug 20, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

RayMuir commented Mar 15, 2024 •

edited

Loading

scottmcm commented Jun 20, 2024 •

edited

Loading

RayMuir commented Jul 11, 2024 •

edited

Loading

RalfJung commented Jul 11, 2024 •

edited

Loading