Get rid of ConstValue::ScalarPair
#55392
Conversation
|
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
This can potentially have adverse effects on performance @bors try |
|
⌛ Trying commit 8ee3e22d801ed63af310d87ba0bbb0b1913c2c1d with merge a08ddc905730cc7c3a758585abcdd8f0608cdfff... |
|
@rust-timer build a08ddc905730cc7c3a758585abcdd8f0608cdfff |
|
Success: Queued a08ddc905730cc7c3a758585abcdd8f0608cdfff with parent 694cf75, comparison URL. |
|
r? @RalfJung |
|
☀️ Test successful - status-travis |
|
Finished benchmarking try commit a08ddc905730cc7c3a758585abcdd8f0608cdfff |
|
Small gains for tuple-stress and ucd, big losses for coercions. Kinda expected. This is a very naive impl: even though str and slices are now ByRef, if they are used in another constant an "optimization" kicks in and converts it back to Value::ScalarPair only to then write it back into memory. |
|
Seems this got out of sync with its base PR? I wanted to look at oli-obk/rust@self_managing_allocations...oli-obk:scalar_single to see what is actually new, but that looks like it still contains #55293. Could you rebase? |
|
Ping from triage. @oli-obk checking for a status update if you have time. thanks! |
oli-obk
added
the
S-blocked
|
This PR is blocked on #55293 (which in turn is blocked, too) |
Mark-Simulacrum
removed
the
S-waiting-on-review
oli-obk
force-pushed
the
scalar_single
branch
from
8ee3e22
to
53506a7
Compare
oli-obk
added
S-waiting-on-review
oli-obk
force-pushed
the
scalar_single
branch
from
53506a7
to
92279b3
Compare
|
@RalfJung do you think it would be reasonable to not read a Kindof adding a |
|
I don't know what you mean -- by either of your two paragraphs.^^
|
|
So... scalar pairs are let _1 = "foo"; // read `ScalarPair` from `ByRef` constant
let _2 = "bar"; // read `ScalarPair` from `ByRef` constant
let _3 = [_1, _2]; // write `ScalarPair` to `ByRef` localI want to skip the |
oli-obk
force-pushed
the
scalar_single
branch
from
92279b3
to
4b367ba
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #56340) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@oli-obk I still don't get it. Are you suggesting we change something in the miri engine? That seems unnecessary, the issue here is just about how to communicate the results of computation to the rest of the compiler. But if you do, could you spell out which miri engine types/operations you intent to change how? The way I see it, we have |
I agree, but what I'm suggesting is to do this the first time we actually try to read an immediate from that local and not do any work before that. I have (since writing #55392 (comment)) implemented a scheme that skips even that step. Instead of creating a This is basically a step all the way back to As a next step I am considering doing the same for |
Yes that's exactly what I was saying? When you need to do work on something, you convert it to an immediate form. The entire rest of the codebase doesn't even have no know that this form exists.
I am increasingly confused. The goal, I thought, was to remove variants from
But what about subslices...? You lost me entirely, I'm afraid. |
No, you want to convert it the moment you read from the global to a local. I want to convert it the first time someone wants to read an immediate from that local. So where we currently move from But as I said, I have moved to a different scheme.
That's not gonna work out of performance reasons. I tried, and each time the perf results were quite clear that we e.g. don't want to have to read array lengths from
Currently a non-issue, so I left it out of the implementation. |
Wait, why are we talking about locals? Inside miri, everything already works.
Okay. So now we are talking about another scheme I don't understand either. Hooray :D
There is no reason that Instead of guessing in Another invariant we fail to encode in the type is that
What does this mean? |
that... makes so much more sense than any of the crazyness I have come up with and failed to make nice.
Encoding fat pointers in constants by using |
Ah I see. Where does that happen though? And can we do something similar to what I proposed for array lengths there? |
I don't think so, every constant in MIR can possibly be a @eddyb suggested once to do some constant folding during |
|
Those constants are only ever accessed for codegen, right? The only difference between what we do now and what we would do then is we would access the EDIT: Okay, there's also |
|
But until we have figured that out, here's how I imagine things could look: enum LazyConstValue<'tcx, T> {
Unevaluated(DefId, &'tcx Substs<'tcx>),
Evaluated(T),
}
enum ConstValue<'tcx> {
Scalar(Scalar),
ScalarPair(Scalar, Scalar),
Indirect(AllocId, &'tcx Allocation, Size),
}
Does that make sense? |
Yes, this is a 1:1 copy of what I prototyped on the train this morning :D |
|
@RalfJung While that's nicer to work with, it's a bit sad, because it means |
|
We have two ways that I can come up with off the top of my head to have our cake and eat it, too:
|
based on #55293 (only the last commit is part of this PR)