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

Add Rvalue::ShallowInitBox to MIR #460

Closed
1 of 3 tasks
nbdd0121 opened this issue Sep 4, 2021 · 3 comments
Closed
1 of 3 tasks

Add Rvalue::ShallowInitBox to MIR #460

nbdd0121 opened this issue Sep 4, 2021 · 3 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@nbdd0121
Copy link

nbdd0121 commented Sep 4, 2021

Proposal

Introduce a new Rvalue variant, Rvalue::ShallowInitBox. It accepts an operand and a type, and will convert the operand to a shallowly-initialized box. This design is considered the better approach than a Box terminator after the Zulip dicussion.

In today's the MIR, box expr is translated to:

let _1: Box<ty>;
_1 = Box(ty);
(*_1) = expr;

however this does not allow unwind. Unwinding is necessary for rust-lang/rfcs#2116's oom=panic support (tracked in rust-lang/rust#43596).

Under this proposal, the originally Rvalue::NullaryOp(NullOp::Box, ty) is split into two operations, first a call to exchange_malloc(size_of::<ty>(), align_of::<ty>()), then converts it into a shallow-initialized box:

let _1: *mut u8;
let _2: Box<ty>;
bb0: {
	_1 = exchange_malloc(SizeOf(ty), AlignOf(ty)) -> [return: bb1, unwind: bb2];
}
bb1: {
	_2 = ShallowInitBox(move _1, ty);
	(*_2) = expr;
}

with the extra unwinding edge provided by the exchange_malloc call.

The operational behaviour of ShallowInitBox is simply transmuting *mut u8 to Box<T>. The only thing special is that dataflow analysis will treat the content of the box as uninitialized.

An alternative is to remove box_syntax and Box nullary op from MIR at all; however, an attempt in rust-lang/rust#87781 shows very significant regression when doing so (perf run). Here is an godbolt snippet shows a case where box still needs special treatment in the MIR, and thus box_syntax is still needed at least as an implementation detail of the library.

Another alternative is the original proposal, to add a TerminatorKind::Box. However adding a new terminator requires much more changes than the current proposal.

The original proposal

Introduce a new terminator, TerminatorKind::Box to the MIR of form:

_1 = Box(ty) -> [return: bb1, unwind: bb2];

it'll be roughly equivalent to

_1 = Box(ty) // Nullary op Box here
goto -> bb1

in today's MIR, but has an additional unwind edge.

Box needs to be a special terminator rather than a function call, because a function call will deep-initialize the return place while Box will only shallow-initialize its return place.

The overall plan is:

  • Introduce TerminatorKind::Box
  • Benchmark performance difference from using Box terminator vs Box nullary op
  • If performance is reasonable, switch box_syntax to use Box terminator instead of Box nullary op
  • Remove Box as a nullary op.

Mentors or Reviewers

If you have a reviewer or mentor in mind for this work, mention then
here. You can put your own name here if you are planning to mentor the
work.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@nbdd0121 nbdd0121 added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Sep 4, 2021
@rustbot
Copy link
Collaborator

rustbot commented Sep 4, 2021

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Sep 4, 2021
@jonas-schievink
Copy link
Contributor

Seems like the correct way to move this forward.

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Sep 4, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 9, 2021
@nbdd0121 nbdd0121 changed the title Add TerminatorKind::Box to MIR Add Rvalue::ShallowInitBox to MIR Sep 12, 2021
@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Sep 16, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 25, 2021
Introduce `Rvalue::ShallowInitBox`

Polished version of rust-lang#88700.

Implements MCP rust-lang/compiler-team#460, and should allow rust-lang#43596 to go forward.

In short, creating an empty box is split from a nullary-op `NullOp::Box` into two steps, first a call to `exchange_malloc`, then a `Rvalue::ShallowInitBox` which transmutes `*mut u8` to a shallow-initialized `Box<T>`. This allows the `exchange_malloc` call to unwind. Details can be found in the MCP.

`NullOp::Box` is not yet removed, purely to make reverting easier in case anything goes wrong as the result of this PR. If revert is needed a reversion of "Use Rvalue::ShallowInitBox for box expression" commit followed by a test bless should be sufficient.

Experiments in rust-lang#88700 showed a very slight compile-time perf regression due to (supposedly) slightly more time spent in LLVM. We could omit unwind edge generation (in non-`oom=panic` case) in box expression MIR construction to restore perf; but I don't think it's necessary since runtime perf isn't affected and perf difference is rather small.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Sep 28, 2021
Introduce `Rvalue::ShallowInitBox`

Polished version of #88700.

Implements MCP rust-lang/compiler-team#460, and should allow #43596 to go forward.

In short, creating an empty box is split from a nullary-op `NullOp::Box` into two steps, first a call to `exchange_malloc`, then a `Rvalue::ShallowInitBox` which transmutes `*mut u8` to a shallow-initialized `Box<T>`. This allows the `exchange_malloc` call to unwind. Details can be found in the MCP.

`NullOp::Box` is not yet removed, purely to make reverting easier in case anything goes wrong as the result of this PR. If revert is needed a reversion of "Use Rvalue::ShallowInitBox for box expression" commit followed by a test bless should be sufficient.

Experiments in #88700 showed a very slight compile-time perf regression due to (supposedly) slightly more time spent in LLVM. We could omit unwind edge generation (in non-`oom=panic` case) in box expression MIR construction to restore perf; but I don't think it's necessary since runtime perf isn't affected and perf difference is rather small.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 3, 2022
Remove `NullOp::Box`

Follow up of rust-lang#89030 and MCP rust-lang/compiler-team#460.

~1 month later nothing seems to be broken, apart from a small regression that rust-lang#89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely.

r? `@jonas-schievink`
`@rustbot` label T-compiler
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jan 13, 2022
Remove `NullOp::Box`

Follow up of #89030 and MCP rust-lang/compiler-team#460.

~1 month later nothing seems to be broken, apart from a small regression that #89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely.

r? `@jonas-schievink`
`@rustbot` label T-compiler
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Jan 18, 2022
Remove `NullOp::Box`

Follow up of #89030 and MCP rust-lang/compiler-team#460.

~1 month later nothing seems to be broken, apart from a small regression that #89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely.

r? `@jonas-schievink`
`@rustbot` label T-compiler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants