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

Convert Const to Allocation in smir #114587

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Aug 7, 2023

Continuation of previous pr #114466

cc rust-lang/project-stable-mir#15

r? @oli-obk

@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 Aug 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 7, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

.align;
Allocation::new_empty_allocation(align.abi)
}
ConstValue::Slice { data, start: _, end: _ } => data.0.stable(tables),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wrong. the allocation refers to what the slice points to, not to the slice pointer itself. maybe just make the entire thing opaque?

Allocation::new_empty_allocation(align.abi)
}
ConstValue::Slice { data, start: _, end: _ } => data.0.stable(tables),
ConstValue::ByRef { alloc, offset: _ } => alloc.0.stable(tables),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you lost the offset. You only want the bytes starting at offset

let size = scalar.size();
let align = tables
.tcx
.layout_of(rustc_middle::ty::ParamEnv::empty().and(const_kind.ty()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.layout_of(rustc_middle::ty::ParamEnv::empty().and(const_kind.ty()))
.layout_of(rustc_middle::ty::ParamEnv::reveal_all().and(const_kind.ty()))

@@ -326,7 +326,7 @@ impl<Prov: Provenance, Bytes: AllocBytes> Allocation<Prov, (), Bytes> {
}

/// Try to create an Allocation of `size` bytes, panics if there is not enough memory
/// available to the compiler to do so.
/// available to the compiler to do so. Also used for creating dummy `Allocation`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a dummy allocation?

The comment change is confusing me.

Copy link
Contributor Author

@ouz-a ouz-a Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should documented as

can be used for creating allocations out of Scalar values,

because from the current doc I couldn't understand I could do that I had to ask oli about it. It's very hard to understand what functions do or when to use them when you are foreign some part of the compiler.

edit:tl;dr: my intention is to make the doc more beginner friendly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you are asking for is a complete tutorial on the data model of the CTFE interpreter and its memory. That's reasonable, this should ideally be documented somewhere, but fn uninit is not the right place for that.

There's nothing specific about uninit that makes it more or less suited for "dummy" allocations so I think this comment change should be removed. Also note that "converting a Scalar to an Allocation" is an extremely uncommon usecase, so it might not be a good idea to gear the documentation towards that.

You could say something general like, "To obtain an allocation filled with specific data, first call this function and then call write_scalar to fill in the right data."

Copy link
Contributor Author

@ouz-a ouz-a Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now on second thought, you are right maybe someone could explain these topics in depth in rustc dev guide

@bors
Copy link
Contributor

bors commented Aug 7, 2023

☔ The latest upstream changes (presumably #114585) made this pull request unmergeable. Please resolve the merge conflicts.

}
ConstValue::ByRef { alloc, offset } => {
let size = alloc.0.size();
let bytes = alloc.0.get_bytes_unchecked(alloc_range(offset, offset + size));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for a 10 element slice at offset 4 this is now going to take elements 4..14, which don't exist.

Also, I just remembered that you may want to use the type size, instead of everything from offset to the end.

@rust-log-analyzer

This comment has been minimized.

.layout_of(rustc_middle::ty::ParamEnv::reveal_all().and(const_kind.ty()))
.unwrap()
.size;
let bytes = alloc.0.get_bytes_unchecked(alloc_range(offset, ty_size));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loses the initialization information and the relocation information. It may be easier to pull out the logic of the stable method for Allocation to be able to call it with arbitrary allocation ranges (and have the stable method always take the entire range). For the initialization information, nothing would change, your existing logic would work if the ranges in the for loop are adjusted. For relocations you'd need to remap the offsets and remove all things that are out of range.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 9, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 9, 2023

📌 Commit 8f1ea57 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 Aug 9, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Aug 9, 2023

@bors rollup

@ouz-a ouz-a mentioned this pull request Aug 9, 2023
@spastorino
Copy link
Member

spastorino commented Aug 9, 2023

#114599 is already approved and was rolled up too.

This PR won't merge cleanly in a rollup with #114599.

@bors rollup-

I can do a rollup again once #114599 is merged in a rollup or if a rollup is not created before this one is in the top of the queue, this one is going to be merged first :).

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 9, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#110435 (rustdoc-json: Add test for field ordering.)
 - rust-lang#111891 (feat: `riscv-interrupt-{m,s}` calling conventions)
 - rust-lang#114377 (test_get_dbpath_for_term(): handle non-utf8 paths (fix FIXME))
 - rust-lang#114469 (Detect method not found on arbitrary self type with different mutability)
 - rust-lang#114587 (Convert Const to Allocation in smir)
 - rust-lang#114670 (Don't use `type_of` to determine if item has intrinsic shim)

Failed merges:

 - rust-lang#114599 (Add impl trait declarations to SMIR)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bbc1109 into rust-lang:master Aug 10, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 10, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 14, 2023
Make Const more useful in smir

Since rust-lang#114587 is merged, we can make use of what we built and make Const more useful by making it not `Opaque`

r? `@spastorino`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 25, 2023
Add stable for Constant in smir

Previously rust-lang#114587 we covered much of the groundwork needed to cover Const in smir, so there is no reason keep `Constant` as String.

r? `@spastorino`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 26, 2023
Add stable for Constant in smir

Previously rust-lang/rust#114587 we covered much of the groundwork needed to cover Const in smir, so there is no reason keep `Constant` as String.

r? `@spastorino`
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Sep 6, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#110435 (rustdoc-json: Add test for field ordering.)
 - rust-lang#111891 (feat: `riscv-interrupt-{m,s}` calling conventions)
 - rust-lang#114377 (test_get_dbpath_for_term(): handle non-utf8 paths (fix FIXME))
 - rust-lang#114469 (Detect method not found on arbitrary self type with different mutability)
 - rust-lang#114587 (Convert Const to Allocation in smir)
 - rust-lang#114670 (Don't use `type_of` to determine if item has intrinsic shim)

Failed merges:

 - rust-lang#114599 (Add impl trait declarations to SMIR)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants