Add Allocation to SMIR by ouz-a · Pull Request #114466 · rust-lang/rust

ouz-a · 2023-08-04T15:08:23Z

As it's discussed here this is an attempt to cover Constants for smir in a different way compared to #114342

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

r? @oli-obk

rustbot · 2023-08-04T15:08:31Z

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

oli-obk · 2023-08-04T15:36:50Z

compiler/rustc_middle/src/mir/interpret/allocation/provenance_map.rs

we can ignore this, this will always be empty except for miri

oli-obk · 2023-08-04T15:43:02Z

compiler/rustc_middle/src/mir/interpret/allocation/init_mask.rs

keep this private. Instead add an iterator over u64 that allows getting the blocks out directly (and just produces the same value over and over in case it was lazy). If you need to box those iterators, that's fine.

We can just store a Vec<u64> in SMIR, we don't need to use this optimization for now.

keep this private.

if I make it private I would have to remove Stable implementation for it. Can't implement trait for private type. (can I?)

you can't, but you can add a function returning the iterator I described to InitMask

keep this private. Instead add an iterator over u64 that allows getting the blocks out directly (and just produces the same value over and over in case it was lazy). If you need to box those iterators, that's fine.

but don't we lose information if it's InitMaskBlocks::Lazy{true} or InitMaskBlocks::Lazy(false), how would someone know if it's lazy or not, since I can't match private value. Maybe something like this ? So people who use smir could identify it it's by checking repeating 0's or 1's

vec![1;10].iter() // lazy ? vec![0;10].iter() // not lazy ?

Lazy is not information anyone cares about. It is just an internal representation detail. it's a optimization to avoid allocating a vec if all entries are true or all entries are false

Ah I get it now, I didn't know it wasn't useful for smir

RalfJung · 2023-08-04T16:18:56Z

compiler/rustc_middle/src/mir/interpret/allocation.rs

This was all quite deliberately sealed away. Do we really have to make this public? :(

Not even the interpreter or codegen need that kind of access. I don't think anything can legitimately require it.

We're gonna make it private before this PR lands. We're not exposing the internals, but adding helper methods that avoid having to expose them.

You don't need to add any helper methods. The existing methods are sufficient for the interpreter, codegen, and MiniRust.

Yes, but these do random access, we want to duplicate the information. Iterating over all bytes is... not great.

No they don't. They build a full model of the allocation contents in MiniRust / LLVM. I really can't see how you would need more APIs than what is required to build a fully accurate model of Rust semantics aka MiniRust.

Here is how MiniRust does it

Yea, I assumed that was too expensive for big allocations, but we can optimize when we see that being an issue

Made them private, I think in future PR we should def get inspiration from MiniRust.

RalfJung · 2023-08-04T16:21:26Z

compiler/rustc_smir/src/stable_mir/ty.rs

This should be AllocId. How is that exposed to smir?

Similar to how we have a table for DefIds, we should add something like it for AllocIds (instead of exposing the AllocIds internals). Since we don't actually need this in this PR yet, we can make Prov just be Opaque and stick a stringified version of the AllocId in it.

I made it Opaque

RalfJung · 2023-08-04T16:22:12Z

compiler/rustc_smir/src/stable_mir/ty.rs

This will not be very useful without doc comments explaining what this is all about.

I added comments from rustc, I don't know specifics of what smir user would want to know from this. I think it should suffice for now? If not I would love to learn more and add more information/documentation.

oli-obk · 2023-08-05T18:33:11Z

compiler/rustc_middle/src/mir/interpret/allocation/init_mask.rs

this is wrong, it's not zero sized, but repeating state for the length of the entire bytes buffer.

But, as Ralf mentioned, we can avoid this entirely by encoding bytes as Vec<Option<u8>> and using https://github.com/RalfJung/minirust/blob/196da9640a4cd608405ad40110d29930827a99a5/tooling/minimize/src/constant.rs#L132-L145 to generate that Vec. Then we don't need an init_mask field at all.

oli-obk · 2023-08-05T18:33:58Z

compiler/rustc_smir/src/rustc_smir/mod.rs

Suggested change

ptrs.push((size.bytes_usize(), opaque(&prov.0.get())));

ptrs.push((size.bytes_usize(), opaque(prov)));

when we render it opaquely, just render the full thing instead of its internal information

oli-obk · 2023-08-06T11:06:25Z

compiler/rustc_smir/src/rustc_smir/mod.rs

This and get_blocks is now dead code

oli-obk · 2023-08-06T11:06:47Z

compiler/rustc_smir/src/stable_mir/ty.rs

THis isn't needed anymore either

oli-obk · 2023-08-06T11:07:13Z

Please squash the commits after all comments have been addressed

RalfJung · 2023-08-06T11:45:54Z

compiler/rustc_middle/src/mir/interpret/allocation.rs

Looks like some spurious remainders, these are now just unnecessary changes.

compiler/rustc_smir/src/rustc_smir/mod.rs

oli-obk · 2023-08-06T13:54:01Z

@bors r+ rollup

bors · 2023-08-06T13:54:03Z

📌 Commit b9a539e has been approved by oli-obk

It is now in the queue for this repository.

Add Allocation to SMIR As it's discussed [here ](https://rust-lang.zulipchat.com/#narrow/stream/320896-project-stable-mir/topic/Representing.20Constants.20in.20smir)this is an attempt to cover Constants for smir in a different way compared to rust-lang#114342 cc rust-lang/project-stable-mir#15 r? `@oli-obk`

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#114466 (Add Allocation to SMIR) - rust-lang#114505 (Add documentation to has_deref) - rust-lang#114519 (use offset_of! to calculate dirent64 field offsets) - rust-lang#114537 (Migrate GUI colors test to original CSS color format) - rust-lang#114539 (linkchecker: Remove unneeded FIXME about intra-doc links) Failed merges: - rust-lang#114485 (Add trait decls to SMIR) r? `@ghost` `@rustbot` modify labels: rollup

Convert Const to Allocation in smir Continuation of previous pr rust-lang#114466 cc rust-lang/project-stable-mir#15 r? `@oli-obk`

Convert Const to Allocation in smir Continuation of previous pr rust-lang/rust#114466 cc rust-lang/project-stable-mir#15 r? `@oli-obk`

rustbot assigned oli-obk Aug 4, 2023

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 4, 2023

oli-obk requested changes Aug 4, 2023

View reviewed changes

RalfJung reviewed Aug 4, 2023

View reviewed changes

oli-obk reviewed Aug 5, 2023

View reviewed changes

oli-obk reviewed Aug 6, 2023

View reviewed changes

compiler/rustc_smir/src/rustc_smir/mod.rs Outdated

Comment on lines 1072 to 1075

Copy link

Contributor

oli-obk Aug 6, 2023

Choose a reason for hiding this comment

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

This and get_blocks is now dead code

oli-obk reviewed Aug 6, 2023

View reviewed changes

compiler/rustc_smir/src/stable_mir/ty.rs Outdated

Comment on lines 260 to 269

Copy link

Contributor

oli-obk Aug 6, 2023

Choose a reason for hiding this comment

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

THis isn't needed anymore either

ouz-a force-pushed the smir_allocation branch 2 times, most recently from c2c3c54 to fc63e74 Compare August 6, 2023 11:11

RalfJung reviewed Aug 6, 2023

View reviewed changes

compiler/rustc_smir/src/rustc_smir/mod.rs Outdated Show resolved Hide resolved

Add alocation to smir

b9a539e

ouz-a force-pushed the smir_allocation branch from cb2abb3 to b9a539e Compare August 6, 2023 12:06

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 6, 2023

matthiaskrgr mentioned this pull request Aug 6, 2023

Rollup of 2 pull requests #114547

Closed

matthiaskrgr mentioned this pull request Aug 6, 2023

Rollup of 5 pull requests #114553

Merged

bors merged commit 92c0421 into rust-lang:master Aug 6, 2023

rustbot added this to the 1.73.0 milestone Aug 6, 2023

ouz-a mentioned this pull request Aug 7, 2023

Convert Const to Allocation in smir #114587

Merged

	ptrs.push((size.bytes_usize(), opaque(&prov.0.get())));
	ptrs.push((size.bytes_usize(), opaque(prov)));

Uh oh!

Conversation

ouz-a commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouz-a Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Aug 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oli-obk commented Aug 6, 2023

Uh oh!

bors commented Aug 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ouz-a commented Aug 4, 2023 •

edited

Loading

ouz-a Aug 4, 2023 •

edited

Loading

RalfJung Aug 4, 2023 •

edited

Loading