Add Allocation to SMIR #114466

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

+    pub fn bytes(&self) -> &Option<Box<SortedMap<Size, Prov>>> {
+        &self.bytes
+    }


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

@@ -185,7 +193,7 @@ impl InitMask {
 // Note: for performance reasons when interning, some of the fields can be partially
 // hashed. (see the `Hash` impl below for more details), so the impl is not derived.
 #[derive(Clone, Debug, Eq, PartialEq, HashStable)]
-struct InitMaskMaterialized {
+pub struct InitMaskMaterialized {


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

-mod init_mask;
-mod provenance_map;
+pub mod init_mask;
+pub mod provenance_map;


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

+
+pub type Bytes = Vec<u8>;
+pub type Size = usize;
+pub type Prov = usize;


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

+#[derive(Clone, Debug)]
+pub struct ProvenanceMap {
+    pub ptrs: Vec<(Size, Prov)>,
+}


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

@@ -64,6 +64,13 @@ impl InitMask {
        }
    }

+    pub fn get_blocks(&self) -> Box<std::slice::Iter<'_, u64>> {
+        match &self.blocks {
+            InitMaskBlocks::Lazy { state: _ } => Box::new([].iter()),


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

+            provenance: {
+                let mut ptrs = Vec::new();
+                for (size, prov) in self.provenance().ptrs().iter() {
+                    ptrs.push((size.bytes_usize(), opaque(&prov.0.get())));


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

+        let mut blocks = Vec::new();
+        for i in self.init_mask().get_blocks() {
+            blocks.push(*i);
+        }


This and get_blocks is now dead code

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

compiler/rustc_smir/src/stable_mir/ty.rs

+#[derive(Clone, Debug)]
+pub enum InitMaskBlocks {
+    Materialized(InitMaskMaterialized),
+}
+
+#[derive(Clone, Debug)]
+pub struct InitMask {
+    pub blocks: InitMaskBlocks,
+    pub len: Size,
+}


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

 use super::{
    read_target_uint, write_target_uint, AllocId, InterpError, InterpResult, Pointer, Provenance,
    ResourceExhaustionInfo, Scalar, ScalarSizeMismatch, UndefinedBehaviorInfo, UninitBytesAccess,
    UnsupportedOpInfo,
 };
 use crate::ty;
 use init_mask::*;
-use provenance_map::*;


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

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 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
11 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Allocation to SMIR #114466

Add Allocation to SMIR #114466

ouz-a commented Aug 4, 2023 •

edited

Loading

rustbot commented Aug 4, 2023

oli-obk Aug 4, 2023

oli-obk Aug 4, 2023

oli-obk Aug 4, 2023

ouz-a Aug 4, 2023 •

edited

Loading

oli-obk Aug 4, 2023

ouz-a Aug 4, 2023

oli-obk Aug 4, 2023

ouz-a Aug 4, 2023

RalfJung Aug 4, 2023 •

edited

Loading

oli-obk Aug 4, 2023

RalfJung Aug 4, 2023

oli-obk Aug 4, 2023

RalfJung Aug 4, 2023

RalfJung Aug 4, 2023

oli-obk Aug 4, 2023

ouz-a Aug 4, 2023

RalfJung Aug 4, 2023

oli-obk Aug 4, 2023

ouz-a Aug 4, 2023

RalfJung Aug 4, 2023

ouz-a Aug 4, 2023

oli-obk Aug 5, 2023

oli-obk Aug 5, 2023

oli-obk Aug 6, 2023

oli-obk Aug 6, 2023

oli-obk commented Aug 6, 2023

RalfJung Aug 6, 2023

oli-obk commented Aug 6, 2023

bors commented Aug 6, 2023

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

Add Allocation to SMIR #114466

Add Allocation to SMIR #114466

Conversation

ouz-a commented Aug 4, 2023 • edited Loading

rustbot commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouz-a Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Aug 6, 2023

Choose a reason for hiding this comment

oli-obk commented Aug 6, 2023

bors commented Aug 6, 2023

ouz-a commented Aug 4, 2023 •

edited

Loading

ouz-a Aug 4, 2023 •

edited

Loading

RalfJung Aug 4, 2023 •

edited

Loading