Introduce ConstValue and use it instead of miri's Value for constant values #50249
Conversation
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 |
|
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 |
src/librustc/middle/const_val.rs
Outdated
| Some(b) | ||
| pub fn to_bits<'gcx>(&self, tcx: TyCtxt<'_, 'gcx, '_>, ty: Ty<'gcx>) -> Option<u128> { | ||
| let primval = match self { | ||
| ConstVal::Value(ref alloc) => { |
There was a problem hiding this comment.
you should be able to just call the to_bits method of Allocation here
src/librustc/mir/interpret/mod.rs
Outdated
| if self.bytes.len() as u64 != size { | ||
| return None; | ||
| } | ||
| // This mirrors the logic of rustc_mir::interpret::memory::Memory::read_primval |
There was a problem hiding this comment.
can Memory::read_primval just forward to this function?
There was a problem hiding this comment.
That question also applies to the other functions here. Can we move functionality from memory here by forwarding from the memory calls?
There was a problem hiding this comment.
can Memory::read_primval just forward to this function?
This doesn't not read from an offset, so no. The other way around could work though.
I want to split Allocation into two types though, one for constant which is interned and read-only and one for Miri to use, so I don't think we should do this refactoring.
There was a problem hiding this comment.
So the deduplication of code will happen with the split into two types?
There was a problem hiding this comment.
No, it just won't be possible to deduplicate it.
There was a problem hiding this comment.
Can't you create a function that takes an offset, and have this function forward to it by filling out the offset with 0?
There was a problem hiding this comment.
We might be able to use Miri methods here if we move the rest of the compiler away from using Value and PrimVal.
There was a problem hiding this comment.
Hmmm I don't understand this argument. Can't miri call this method or the hypothetical one with offsets?
There was a problem hiding this comment.
Miri has callbacks into the machine for locks on the memory. That makes it tricky to factor it out.
There was a problem hiding this comment.
This code is now gone.
| @@ -393,6 +403,15 @@ macro_rules! implement_ty_decoder { | |||
| decode_const(self) | |||
| } | |||
| } | |||
|
|
|||
| impl<$($typaram),*> SpecializedDecoder<&'tcx $crate::mir::interpret::Allocation> | |||
There was a problem hiding this comment.
Doesn't this conflict with the derive on Allocation? IIRC this produces runtime issues if done incorrectly. Have a look at what AllocId does
There was a problem hiding this comment.
I don't think so. This decodes a pointer to an Allocation, while the derive is for Allocation itself.
src/librustc/ty/sty.rs
Outdated
| } | ||
|
|
||
| pub fn to_bits<'gcx>(&self, tcx: TyCtxt<'_, 'gcx, '_>, ty: Ty<'gcx>) -> Option<u128> { | ||
| if self.ty != ty { |
There was a problem hiding this comment.
Can this be an assertion? In what cases to we actually return None due to a type mismatch?
There was a problem hiding this comment.
This just tries to convert a value to an u128 if it is of the correct type ty. It is used as an replacement for matching in many cases. An example is the to_usize method.
| @@ -381,28 +377,24 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
|
|
|||
| // Helper to get a `-1` value of the appropriate type | |||
| fn neg_1_literal(&mut self, span: Span, ty: Ty<'tcx>) -> Operand<'tcx> { | |||
| let ty = self.hir.tcx().lift_to_global(&ty).unwrap(); | |||
There was a problem hiding this comment.
I'm not too happy with all the lift_to_global calls. Can we make the lifetime annotations on from_bits and friends work out so we don't need that anymore?
There was a problem hiding this comment.
We need global types here since we must compute their layout (which requires Ty<'gcx>).
There was a problem hiding this comment.
The lift_to_global calls have been removed.
| kind: box PatternKind::Constant { | ||
| value: tcx.mk_const( | ||
| ty::Const::from_bits(tcx, | ||
| *b as u128, |
|
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 #50097) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -115,16 +115,21 @@ pub struct ValTy<'tcx> { | |||
| pub value: Value, | |||
| pub ty: Ty<'tcx>, | |||
| } | |||
|
|
|||
| /* | |||
| impl<'tcx> ValTy<'tcx> { | |||
There was a problem hiding this comment.
Remove this since it seems to be unused?
| @@ -15,12 +15,12 @@ | |||
| use rustc::hir::def::Def; | |||
| use rustc::mir::{Constant, Literal, Location, Place, Mir, Operand, Rvalue, Local}; | |||
| use rustc::mir::{NullOp, StatementKind, Statement, BasicBlock, LocalKind}; | |||
| use rustc::mir::{TerminatorKind, ClearCrossCrate, SourceInfo, BinOp, ProjectionElem}; | |||
| use rustc::mir::{TerminatorKind, ClearCrossCrate, SourceInfo, BinOp/*, ProjectionElem*/}; | |||
| @@ -60,7 +60,7 @@ impl MirPass for ConstProp { | |||
| } | |||
| } | |||
|
|
|||
| type Const<'tcx> = (Value, ty::Ty<'tcx>, Span); | |||
| type Const<'tcx> = (&'tcx Allocation, ty::Ty<'tcx>, Span); | |||
|
|
|||
There was a problem hiding this comment.
I think this needs to stay Value. Or where was it necessary to move to allocation?
There was a problem hiding this comment.
Yeah, I think const_prop should just create one Miri context, then it can create as many Values as it likes and it will all be freed when it's done.
| @@ -334,7 +343,8 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> { | |||
| } | |||
| Value::ByVal(val) | |||
| }; | |||
| Some((val, place_ty, span)) | |||
|
|
|||
| Some((const_value_to_alloc(self.tcx, val, place_ty), place_ty, span)) | |||
There was a problem hiding this comment.
His interns an intermediate/temp value. Won't that slowly fill up memory without really using it
|
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 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 |
|
@bors try |
|
⌛ Trying commit cd5230f5c5183f3af61b895b599502da867b9511 with merge 750791559018389ae7bae476053c1a162d40ecdf... |
|
I've changed this to add a pub enum ConstValue<'tcx> {
// Used only for types with layout::abi::Scalar ABI and ZSTs which use PrimVal::Undef
ByVal(PrimVal),
// Used only for types with layout::abi::ScalarPair
ByValPair(PrimVal, PrimVal),
// Used only for the remaining cases
ByRef(&'tcx Allocation),
}
|
Zoxc
changed the title
| @@ -12,17 +12,11 @@ | |||
|
|
|||
| #![crate_type = "lib"] | |||
|
|
|||
| // CHECK: @VAR1 = constant <{ [4 x i8] }> <{ [4 x i8] c"\01\00\00\00" }>, section ".test_one" | |||
| // CHECK: @VAR1 = constant i32 1, section ".test_one" | |||
There was a problem hiding this comment.
@eddyb The types of static are changed for some values. I assume all uses of statics use bitcasts anyway?
There was a problem hiding this comment.
This seems wrong, why would a static go through anything other than an Allocation?
There was a problem hiding this comment.
Here is the code which converts constants into LLVM values: https://github.com/rust-lang/rust/pull/50249/files#diff-823b4422fff3f3018e53dfb6dc0a1fb3R84
It does not seem useful to convert these back into an Allocation.
There was a problem hiding this comment.
statics should never be anything other than Allocation, so this seems wrong. Evaluating the initializer of a static should not be done the same way as evaluating a const. Only the former should produce a value, whereas a static should already have an allocation, and the evaluation should "fill it in"
Hang on, I got a bit turned around. The value in this case is the initializer, so ByRef indicates not the "allocation of the static" but the "contents" - it just happens to use &Allocation, which has become confusing.
In which case, if no information is obtained from that Allocation other than the bytes and the relocations, it seems fine (I just checked and this is indeed the case).
Can you inline the body of global_initializer into const_value_to_llvm and rename the latter to global_initializer? At least that way it can't be easily confused with the case where ByRef actually requires indirection.
There was a problem hiding this comment.
global_initializer is still used elsewhere. I've renamed it to const_alloc_to_llvm.
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum I'd like a perf run here |
Zoxc
changed the title
|
Perf queued. |
src/librustc/ich/impls_ty.rs
Outdated
| @@ -384,6 +384,30 @@ for ::middle::const_val::ConstVal<'gcx> { | |||
| } | |||
| } | |||
|
|
|||
| impl<'a, 'gcx> HashStable<StableHashingContext<'a>> | |||
| for ::middle::const_val::ConstValue<'gcx> { | |||
There was a problem hiding this comment.
Please do not add more things to that module. It should be removed already. Everything constant belongs in miri.
There was a problem hiding this comment.
It almost seems like you should be able to make miri's Value generic over the allocation information in the ByRef case.
There was a problem hiding this comment.
I agree. Also it would probably suffice to return the Allocation directly instead of an interned one.
There was a problem hiding this comment.
Please do not add more things to that module. It should be removed already. Everything constant belongs in miri.
Do you want to move ty::Const, ConstVal and ConstValue to a new module librustc\mir\interpret\const.rs?
It almost seems like you should be able to make miri's Value generic over the allocation information in the ByRef case.
I don't think there's anything to be gained by doing that. I'd rather just get rid of miri's Value type. Representing values by pointers to values in some cases and just values in other cases is confusing.
Also it would probably suffice to return the Allocation directly instead of an interned one.
I don't know what you mean here, but ConstValue can't store an Allocation directly, since it constants are in the dropless interner. Interning here also makes type folding cheaper.
There was a problem hiding this comment.
@Zoxc ConstValue should be right next to the existing miri Value, while ConstVal should be ConstKind right next to the definition of ty::Const.
There was a problem hiding this comment.
I've moved ConstValue next to Value. I don't want to move the other types in this PR, it is large enough already.
|
☔ The latest upstream changes (presumably #50198) made this pull request unmergeable. Please resolve the merge conflicts. |
Zoxc
force-pushed
the
allocation-const
branch
2 times, most recently
from
db253de
to
3f4cb4d
Compare
|
☔ The latest upstream changes (presumably #50395) made this pull request unmergeable. Please resolve the merge conflicts. |
Zoxc
force-pushed
the
allocation-const
branch
3 times, most recently
from
774a5a7
to
84244fc
Compare
|
@bors r+ let's not make this PR any bigger. There's already a followup PR and FIXMEs are in place where applicable. |
|
📌 Commit 0aa92ac has been approved by |
bors
added
S-waiting-on-bors
Introduce ConstValue and use it instead of miri's Value for constant values r? @oli-obk
|
☀️ Test successful - status-appveyor, status-travis |
Tested on commit rust-lang/rust@c705877. Direct link to PR: <rust-lang/rust#50249> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
r? @oli-obk