CTFE engine refactor #53424
Merged
CTFE engine refactor #53424
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rust-highfive
added
the
S-waiting-on-review
|
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 |
RalfJung
force-pushed
the
miri-refactor
branch
from
3f349b4 to
af45af7
Compare
|
We'll want a perf run. @bors try |
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
|
Hu? I rebased right before I submitted this... |
RalfJung
force-pushed
the
miri-refactor
branch
from
af45af7 to
2d1134d
Compare
|
@bors try |
|
⌛ Trying commit 2d1134d1ee313cd422bb4767d000cb4e727e87e7 with merge b7e59799d3969b8df886349ed1a3c7ae38c1e57a... |
|
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 |
oli-obk
requested changes
Aug 16, 2018
| self.memory.check_align(ptr, ptr_align)?; | ||
|
|
||
| if mplace.layout.size.bytes() == 0 { | ||
| return Ok(Some(Value::Scalar(ScalarMaybeUndef::Scalar(Scalar::Bits { bits: 0, size: 0 })))); |
There was a problem hiding this comment.
use Scalar::zst(). Maybe even Scalar::zst().into()
| // This decides which types we will use the Immediate optimization for, and hence should | ||
| // match what `try_read_value` and `eval_place_to_op` support. | ||
| if layout.is_zst() { | ||
| return Ok(Operand::Immediate(Value::Scalar(ScalarMaybeUndef::Undef))); |
There was a problem hiding this comment.
This feels very odd. We have an undefined ZST?
There was a problem hiding this comment.
Okay I will just use Scalar::zst()
There was a problem hiding this comment.
What made you change your mind?
There was a problem hiding this comment.
Talking with @eddyb :D And also thinking about what this even means... I mean, ZSTs don't really have a value. The read functions short-circuit them before we even hit memory, and they always return Scalar::zst(). So this seems more consistent.
Also, 0 bytes of unintiailized memory are not really undef. They are just empty. As in, if you think of a value as a &[Byte], where Byte is either u8 or Undef -- then a value of size 0 cannot be Undef.
| layout::FieldPlacement::Array { count, .. } => count, | ||
| _ => bug!("Length for non-array layout {:?} requested", self.layout), | ||
| }; | ||
| if let PlaceExtra::Length(len) = self.extra { |
There was a problem hiding this comment.
Why would an array ever have this? or do slices have Array field layout, too?
There was a problem hiding this comment.
I was thinking so... @eddyb should know.^^ What is the layout of [u8]?
I seem to remember it is an "array" of length 0.
| self.layout.ty.hash(state); | ||
| } | ||
| } | ||
| impl<'tcx> PartialEq for MPlaceTy<'tcx> { |
There was a problem hiding this comment.
No, TyLayout has no PartialEq. So we just compare the type.
There was a problem hiding this comment.
You might want to include a few more things in the Eq/Hash, such as an Option<usize> where Some(index) means Variants::Single { index }, while None is for all other Variants.
src/librustc_mir/interpret/place.rs
Outdated
| pub fn write_value( | ||
| &mut self, | ||
| src_val: Value, | ||
| dest : PlaceTy<'tcx>, |
src/test/ui/union-ub-fat-ptr.rs
Outdated
| const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice}; | ||
| //~^ ERROR this constant likely exhibits undefined behavior | ||
| // bad | ||
| const C2: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.slice}; | ||
| //~^ ERROR this constant likely exhibits undefined behavior |
There was a problem hiding this comment.
can you also add a test where a trait object has the correct vtable and pointer but the object is wrong? So create the object unsafely with brokenness and then downcast
There was a problem hiding this comment.
I added these:
// bad data *inside* the trait object
const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl };
// bad data *inside* the slice
const H: &[bool] = &[unsafe { BoolTransmute { val: 3 }.bl }];Does that look like it is testing the right thing?
There was a problem hiding this comment.
You forgot to push, but assuming that bool: Trait it should
There was a problem hiding this comment.
No I just waited for --bless to finish before pushing. ;)
|
☀️ Test successful - status-travis |
|
Okay we don't need a perf run yet... triggered by a warning that some test takes >60s, I benchmarked |
|
Uh. Perf says
However, I also have debug-assertions enabled, so let's see if that makes a difference. |
|
After compiling without debug assertions, it's still 50% slower. |
RalfJung
force-pushed
the
miri-refactor
branch
from
d3b20ad to
7066438
Compare
oli-obk
approved these changes
Aug 17, 2018
|
r=me w/o perf regressions and a crater run |
|
Didn't you suggest we should make a crater run as well? |
|
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 |
|
I guess we do not have to wait with the crater run until I completed the perf work... @bors try |
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
|
Argh.^^ Okay, another rebase... |
RalfJung
force-pushed
the
miri-refactor
branch
from
c4b2a17 to
6cf4d70
Compare
RalfJung
force-pushed
the
miri-refactor
branch
from
4038124 to
4fec615
Compare
|
This now does not update the submodule anymore. The submodule update will come in a separate PR. @bors r+ |
|
📌 Commit 4fec615 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 22, 2018
CTFE engine refactor
* Value gets renamed to `Operand`, so that now `interpret::{Place, Operand}` are the "dynamic" versions of `mir::{Place, Operand}`.
* `Operand` and `Place` share the data for their "stuff is in memory"-base in a new type, `MemPlace`. This also makes it possible to give some more precise types in other areas. Both `Operand` and `MemPlace` have methods available to project into fields (and other kinds of projections) without causing further allocations.
* The type for "a `Scalar` or a `ScalarPair`" is called `Value`, and again used to give some more precise types.
* All of these have versions with an attached layout, so that we can more often drag the layout along instead of recomputing it. This lets us get rid of `PlaceExtra::Downcast`. `MPlaceTy` and `PlaceTy` can only be constructed in place.rs, making sure the layout is handled properly. (The same should eventually be done for `ValTy` and `OpTy`.)
This is used to check, when copying an operand to a place, that the sizes match (which caught a bunch of bugs).
* All the high-level functions to write typed memory take a `Place`, and live in `place.rs`. All the high-level typed functions to read typed memory take an `Operand`, and live in `operands.rs`.
* Remove `cur_frame` and handling of signedess from memory (catching a bug in the float casting code).
* [Only functional change] Enable sanity check to recurse below dyn traits and slices.
r? @oli-obk
Cc @eddyb
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced Aug 22, 2018
bors
added a commit
that referenced
this pull request
Aug 22, 2018
fix array drop glue: properly turn raw ptr into reference Discovered while working on #53424: The generated drop glue uses an assignment `ptr = cur` where `ptr` is a reference and `cur` a raw pointer. This is not well-formed MIR. Do we have MIR sanity checks that run on the drop glue and should have caught this? r? @eddyb
|
This PR is helping fixing #52849. Since that's a beta regression, should this be backported @rust-lang/compiler? |
pietroalbini
added
beta-nominated
|
I think this PR is way too big to be backported. If need be we can disable validation on beta. |
nikomatsakis
removed
the
beta-nominated
|
in the discussion on #52849, it sounds like one of the big performance issues came from hashing the memory state, which was then something that the discussion talked about removing. Was hashing the memory state actually removed (I cannot tell from skimming the titles of the commit series) on this PR (#53424)? And if it was the case, can that part of the diff be backported on its own? |
|
No it has not been removed here, but it will be removed in #52626. And yes, just removing it from the hash is trivial to backport. Do you want me to do that? |
|
Yes and no. The majority of that issue is fully intentional -- we walk the resulting value computed for every constant; in benchmarks that consist only of a single constant, that has a huge impact. Two unintentional changes that are also part of the same issue (hence the rather messy overall situation) are:
|
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Operand, so that nowinterpret::{Place, Operand}are the "dynamic" versions ofmir::{Place, Operand}.OperandandPlaceshare the data for their "stuff is in memory"-base in a new type,MemPlace. This also makes it possible to give some more precise types in other areas. BothOperandandMemPlacehave methods available to project into fields (and other kinds of projections) without causing further allocations.Scalaror aScalarPair" is calledValue, and again used to give some more precise types.PlaceExtra::Downcast.MPlaceTyandPlaceTycan only be constructed in place.rs, making sure the layout is handled properly. (The same should eventually be done forValTyandOpTy.)This is used to check, when copying an operand to a place, that the sizes match (which caught a bunch of bugs).
Place, and live inplace.rs. All the high-level typed functions to read typed memory take anOperand, and live inoperands.rs.cur_frameand handling of signedess from memory (catching a bug in the float casting code).r? @oli-obk
Cc @eddyb