Implement Valtree to ConstValue conversion #95976

b-naber · 2022-04-12T13:23:55Z

Once we start to use ValTrees in the type system we will need to be able to convert them into ConstValue instances, which we want to continue to use after MIR construction.

r? @oli-obk

cc @RalfJung

rust-highfive · 2022-04-12T13:23:57Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

b-naber · 2022-04-12T13:40:42Z

Will resolve the conflicts later. CI is irrelevant to this PR, since we don't yet use any of the functionality that is introduced here.

RalfJung · 2022-04-12T18:33:47Z

Hm, the conversion is lossy so we have to be careful about when we do the round-trip.

But I also cannot think of a good way to avoid having the back-conversion. :/ That is some unfortunate extra complexity...

All non-trivial valtrees will have been constructed from a constant. So maybe we could remember that constant somewhere and just use that for the backconversion? But that sounds like a potentially bad side-channel circumventing query purity...

RalfJung · 2022-04-12T18:36:00Z

I am surprised by ConstValue as the conversion target though. Isn't the plan to entirely get rid of ConstValue? Those should all become valtrees.

b-naber · 2022-04-12T18:43:28Z

I am surprised by ConstValue as the conversion target though. Isn't the plan to entirely get rid of ConstValue? Those should all become valtrees.

@RalfJung We were debating this here.

RalfJung · 2022-04-13T02:34:06Z

That doesn't really mention the point that ConstValue is supposed to be removed. We currently already have too many types that all sound almost the same (Const, ConstS, ConstKind, ConstValue) -- valtrees shouldn't make that list any longer.^^

@lcnr wrote

would have to check whether we would then have to reimplement the conversion for each backend

However, I think if valtrees can be converted into a ConstAlloc, then that's enough for all backends? Most backends will probably want to have a shortcut for certain small valtrees (like, of integer type), but I don't think we want to keep the ConstValue type around.

oli-obk · 2022-04-13T05:34:24Z

For now, moving to just ConstAlloc instead of ConstValue is a too big change to be in one step. We can either dynamically forbid it in MIR and convert everywhere to ConstAlloc as a test, or we can do this PR and add valtrees and then explore that possibility afterwards. I'm slightly in favour of the latter, as the amount of ConstValue is smaller aver valtrees, making such a change easier to review

lcnr · 2022-04-13T06:05:10Z

can miri also directly work with ConstAlloc? That conversion is necessary because we have to go from ty::Const to mir::ConstantKind for ctfe

b-naber · 2022-04-13T07:18:13Z

Wouldn't there be problems in the query system if we would convert ValTrees into ConstAllocs (if I remember correctly I suggested keeping the AllocIds around and re-using them later, but @lcnr mentioned that this would be problematic with regards to the query system)? Is the idea to keep the AllocIds stored somewhere and then map a ValTree to an AllocId? Otherwise if we would create a new Allocation for a ValTree in the conversion, afaict that would basically amount to most of what this PR is doing.

RalfJung · 2022-04-13T15:21:18Z

ConstAlloc should be trivial to use for Miri, much easier than any other representation.

But if this is just a stepping stone, and the plan is still to remove ConstValue later, and most of the work done here will be relevant even after ConstValue removal, then I am happy. :)

lcnr · 2022-04-13T19:17:58Z

yes, I still don't think we should cache the origin for valtrees so that we don't have to rebuild them later, that's prone to cause unstable query results, leading to ICE or unsoudness

b-naber · 2022-04-21T13:57:00Z

@RalfJung Since Oli is on vacation, can you maybe review this, please? This is only code that is const-eval related.

oli-obk · 2022-04-21T14:02:41Z

CI is failing... maybe we should teach CI to keep compiling on lint errors but fail only at the end

RalfJung · 2022-04-21T14:15:05Z

Sorry, I am traveling for the next 2.5 weeks and won't have much Rust time at all.

compiler/rustc_const_eval/src/interpret/operand.rs

compiler/rustc_const_eval/src/interpret/place.rs

compiler/rustc_const_eval/src/const_eval/valtrees.rs

…nversion

compiler/rustc_const_eval/src/interpret/place.rs

compiler/rustc_const_eval/src/interpret/operand.rs

compiler/rustc_const_eval/src/interpret/mod.rs

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

compiler/rustc_const_eval/src/const_eval/valtrees.rs

b-naber · 2022-04-26T16:45:15Z

@oli-obk Some tests involving statics still fail, this is due to const_to_valtree currently being called in the wrong place. I'm currently implementing the functionality for transitioning to using Valtrees in the type system (which will be part of a future PR though) and we won't encounter those errors then, since const_to_valtree will be called in the correct places. I'd suggest to run the test set including the call behind the debug_assertions cfg guard here once and then remove the call again.

compiler/rustc_const_eval/src/const_eval/valtrees.rs

compiler/rustc_middle/src/ty/util.rs

b-naber · 2022-04-27T14:26:54Z

Addressed the review and added another recursive call for unsized types that was necessary to correctly handle nested custom DSTs.

I agree with your point that ideally we should only have on function for this, but I will try to fix this in a future PR.

Also haven't included the depth limit on constants here yet, since this is called in eval_to_const_value and we get constants with more than 1000 nodes.

b-naber · 2022-04-27T14:53:05Z

Test suite results were as expected. Took the call of valtree_to_const_value out again.

…ested unsized types correctly

oli-obk · 2022-04-28T08:54:11Z

While I was hoping we could leave it in, let's merge this and address the issue with statics in a separate PR

@bors r+

bors · 2022-04-28T08:54:13Z

📌 Commit ef5f072 has been approved by oli-obk

bors · 2022-04-28T13:18:30Z

⌛ Testing commit ef5f072 with merge b2c2a32...

bors · 2022-04-28T15:47:40Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing b2c2a32 to master...

rust-timer · 2022-04-28T17:04:30Z

Finished benchmarking commit (b2c2a32): comparison url.

Summary:

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 🎉 relevant improvements found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	0	1	3	1
mean²	N/A	N/A	-0.4%	-0.3%	-0.4%
max	N/A	N/A	-0.4%	-0.4%	-0.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

RalfJung · 2022-05-09T10:42:34Z

compiler/rustc_const_eval/src/interpret/place.rs

@@ -115,6 +115,12 @@ impl<'tcx, Tag: Provenance> std::ops::Deref for MPlaceTy<'tcx, Tag> {
    }
 }

+impl<'tcx, Tag: Provenance> std::ops::DerefMut for MPlaceTy<'tcx, Tag> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.mplace


Not having mutable access to these fields was a deliberate decision. Why is this needed?

I think this is just leftover from previous refactorings. I missed that one :/

this is currently made use of here: https://github.com/b-naber/rust/blob/ef5f07256cfa4e1f43a18acb45e0d8108f8471a4/compiler/rustc_const_eval/src/const_eval/valtrees.rs#L227

I think it'd be better to construct a new MPlaceTy than change the existing one.

RalfJung · 2022-05-09T10:49:21Z

compiler/rustc_const_eval/src/const_eval/valtrees.rs

+    if !ty.is_sized(ecx.tcx, ty::ParamEnv::empty()) {
+        // We need to create `Allocation`s for custom DSTs
+
+        let (unsized_inner_ty, num_elems) = get_info_on_unsized_field(ty, valtree, tcx);


If we only need all this to get the size, then maybe it'd make sense to alternatively have a type for a (ValTree, Option<Metadata>)? That's often what our reference types end up looking like, so it probably makes sense here, too.

OTOH it is true that the valtree should already contain all the info we need, it's just a bit awkward to get it. Do we plan to ever have valtrees for dyn Trait?

Do we plan to ever have valtrees for dyn Trait?

Not at present.

If we only need all this to get the size, then maybe it'd make sense to alternatively have a type for a (ValTree, Option<Metadata>)?

you mean an additional variant on ValTree? This is probably going in the same bucket as the string representation optimization that we'll look into. Until that happens (if ever) we could just add more convenience helpers

RalfJung · 2022-05-09T10:50:33Z

compiler/rustc_const_eval/src/const_eval/valtrees.rs

+            intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &place).unwrap();
+
+            let const_val = match ty.kind() {
+                ty::Ref(_, _, _) => {


Given these two match that special-case Ref, it might make sense to just make this a separate arm in the top-level match?

😆 this was intentionally requested by me.

The three calls above and the opt_to_const were shared and the difference between the arms wasn't very obvious to me, with this change the difference is very obvious.

The first match is actually unnecessary, we can just unconditionally call create_pointee_place (though it should probably be renamed)

Fair.^^ I wouldn't even expect a lot of commonalities between these arms so having them into one seems a bit forced to me. 🤷

RalfJung · 2022-05-09T10:51:51Z

compiler/rustc_const_eval/src/const_eval/valtrees.rs

+
+// FIXME Needs a better/correct name
+#[instrument(skip(ecx), level = "debug")]
+fn fill_place_recursively<'tcx>(


This is basically the destination passing style version of valtree_to_const_value. So what about something like valtree_into_mplace?

I don't like that this duplicates logic from valtree_to_const_value

FWIW I am not surprised that something like this is required. It should not have a lot of duplication though; looks like that got improved since the comment has been written.

RalfJung · 2022-05-09T10:52:42Z

compiler/rustc_const_eval/src/interpret/place.rs

@@ -487,7 +494,8 @@ where
    }

    /// Project into an mplace
-    pub(super) fn mplace_projection(
+    #[instrument(skip(self), level = "debug")]
+    pub(crate) fn mplace_projection(


Looks like this is not even used after all, and could hence remain private?

rust-highfive assigned oli-obk Apr 12, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2022

b-naber added 2 commits April 21, 2022 10:59

add some helper methods to ScalarInt

2bc59c7

add helper methods on ValTree

eaf8cda

b-naber force-pushed the valtree-constval-conversion branch from b66ed4d to ee31aaf Compare April 21, 2022 13:50

implement valtree -> constvalue conversion

1157dc7

b-naber force-pushed the valtree-constval-conversion branch from ee31aaf to 1157dc7 Compare April 21, 2022 13:53

This comment has been minimized.

Sign in to view

oli-obk requested changes Apr 21, 2022

View reviewed changes

implement (as of now still unused) query for valtree -> constvalue co…

28af967

…nversion

RalfJung reviewed Apr 21, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/place.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 21, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/operand.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 21, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 21, 2022

View reviewed changes

compiler/rustc_middle/src/mir/interpret/pointer.rs Outdated Show resolved Hide resolved

b-naber commented Apr 21, 2022

View reviewed changes

compiler/rustc_const_eval/src/const_eval/valtrees.rs Outdated Show resolved Hide resolved

account for custom DSTs in valtree -> constvalue conversion

bfefb4d

b-naber force-pushed the valtree-constval-conversion branch from 8829cf2 to 37856d4 Compare April 26, 2022 16:41

This comment has been minimized.

Sign in to view

oli-obk reviewed Apr 26, 2022

View reviewed changes

compiler/rustc_const_eval/src/const_eval/valtrees.rs Outdated Show resolved Hide resolved

oli-obk reviewed Apr 26, 2022

View reviewed changes

compiler/rustc_middle/src/ty/util.rs Outdated Show resolved Hide resolved

b-naber force-pushed the valtree-constval-conversion branch from afba722 to 5faf24f Compare April 27, 2022 14:25

This comment has been minimized.

Sign in to view

b-naber force-pushed the valtree-constval-conversion branch from 5faf24f to 1198729 Compare April 27, 2022 14:51

This comment has been minimized.

Sign in to view

combine all unsized types and add another recursive call to process n…

ef5f072

…ested unsized types correctly

b-naber force-pushed the valtree-constval-conversion branch from 1198729 to ef5f072 Compare April 27, 2022 14:58

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 Apr 28, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 28, 2022

bors merged commit b2c2a32 into rust-lang:master Apr 28, 2022

rustbot added this to the 1.62.0 milestone Apr 28, 2022

bors mentioned this pull request Apr 28, 2022

Fix inaccurate function name in rustc_const_eval docs #95281

Merged

RalfJung reviewed May 9, 2022

View reviewed changes

Implement Valtree to ConstValue conversion #95976

Implement Valtree to ConstValue conversion #95976

Conversation

b-naber commented Apr 12, 2022

rust-highfive commented Apr 12, 2022

b-naber commented Apr 12, 2022

RalfJung commented Apr 12, 2022

RalfJung commented Apr 12, 2022

b-naber commented Apr 12, 2022

RalfJung commented Apr 13, 2022 • edited Loading

oli-obk commented Apr 13, 2022 • edited Loading

lcnr commented Apr 13, 2022

b-naber commented Apr 13, 2022 • edited Loading

RalfJung commented Apr 13, 2022

lcnr commented Apr 13, 2022

b-naber commented Apr 21, 2022 • edited Loading

oli-obk commented Apr 21, 2022

This comment has been minimized.

RalfJung commented Apr 21, 2022

b-naber commented Apr 26, 2022

This comment has been minimized.

b-naber commented Apr 27, 2022 • edited Loading

This comment has been minimized.

b-naber commented Apr 27, 2022

This comment has been minimized.

oli-obk commented Apr 28, 2022

bors commented Apr 28, 2022

bors commented Apr 28, 2022

bors commented Apr 28, 2022

rust-timer commented Apr 28, 2022

Footnotes

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

RalfJung commented Apr 13, 2022 •

edited

Loading

oli-obk commented Apr 13, 2022 •

edited

Loading

b-naber commented Apr 13, 2022 •

edited

Loading

b-naber commented Apr 21, 2022 •

edited

Loading

b-naber commented Apr 27, 2022 •

edited

Loading