Use mir constant in thir instead of ty::Const #94255
Conversation
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
| @@ -200,14 +200,17 @@ impl<'tcx> AbstractConst<'tcx> { | |||
| Ok(inner.map(|inner| AbstractConst { inner, substs: uv.substs })) | |||
| } | |||
|
|
|||
| pub fn from_const( | |||
| pub fn from_constant( | |||
There was a problem hiding this comment.
ConstantKind::Value is for non type system stuff isnt it? all of the stuff in this file is to do with type level constants so it seems kinda sus to be taking a ConstantKind here instead of a Const. I'm not all that familiar with mir::Constant so I may be off here 🤔
There was a problem hiding this comment.
Yes ConstantKind::Value isn't meant to be seen by the type system, but I don't think this is a problem here since we basically discard those here and only care about unevaluated constants. I think this is unlikely to change given the purpose of AbstractConst.
There was a problem hiding this comment.
we don't want this, should never need an AbstractConst for mir constants. These are only used to unify constants in the type system
Dylan-DPC
added
S-blocked
There was a problem hiding this comment.
will get to the rest of this PR next week. We originally intended to also use valtrees in pattern, as they should simplify exhaustiveness checking.
Currently trying to figure out what exactly our plan should be for that and more generally how to deal with structural equality going forward
compiler/rustc_middle/src/mir/mod.rs
Outdated
| } | ||
|
|
||
| #[instrument(skip(tcx), level = "debug")] | ||
| pub fn try_eval_lit_or_param( |
There was a problem hiding this comment.
that shouldn't be needed, you can always keep mir constants as unevaluated. We maybe have to change it so that mir constants are always eagerly normalized, but we shouldn't have to special case them here
There was a problem hiding this comment.
Mir constants do not need eager normalization per se, but it can be helpful for perf if mir opts can trust that they don't even need to attempt to eval them, because all evaluable ones have been evaluated
There was a problem hiding this comment.
can you remove this function and instead eagerly call tcx.const_eval_resolve on the DefId with its identity substs before building the mir constants?
Yea. My plan was "evaluate to valtree and if that fails, keep the constant opaque". I'm not sure we can fall back to "always be opaque" without a breaking change. And keeping around the deref/field infrastructure for mir constants is a cost I'd rather avoid. |
| /// String literals. Strings are not quite the same as `&[u8]` so we treat them separately. | ||
| Str(ty::Const<'tcx>), | ||
| Str(mir::ConstantKind<'tcx>), |
There was a problem hiding this comment.
as we structurally match strings, we should use ty::Const<'tcx> here
There was a problem hiding this comment.
or tbh, probably we should use Str(Valtree) 😅 not sure what's the best way to do this
There was a problem hiding this comment.
&'tcx [u8] would be the least ambiguous, but may require some conversions or interning later. Right now you can get it right from the Allocation
| @@ -200,14 +200,17 @@ impl<'tcx> AbstractConst<'tcx> { | |||
| Ok(inner.map(|inner| AbstractConst { inner, substs: uv.substs })) | |||
| } | |||
|
|
|||
| pub fn from_const( | |||
| pub fn from_constant( | |||
There was a problem hiding this comment.
we don't want this, should never need an AbstractConst for mir constants. These are only used to unify constants in the type system
b-naber
force-pushed
the
use-mir-constant-in-thir
branch
from
ae1bc06
to
2eab684
Compare
This comment has been minimized.
This comment has been minimized.
b-naber
force-pushed
the
use-mir-constant-in-thir
branch
from
2eab684
to
16929c3
Compare
This comment has been minimized.
This comment has been minimized.
b-naber
force-pushed
the
use-mir-constant-in-thir
branch
2 times, most recently
from
c1169e5
to
647211a
Compare
This comment has been minimized.
This comment has been minimized.
b-naber
force-pushed
the
use-mir-constant-in-thir
branch
from
647211a
to
6fff021
Compare
This comment has been minimized.
This comment has been minimized.
compiler/rustc_middle/src/mir/mod.rs
Outdated
| } | ||
|
|
||
| #[instrument(skip(tcx), level = "debug")] | ||
| pub fn try_eval_lit_or_param( |
There was a problem hiding this comment.
can you remove this function and instead eagerly call tcx.const_eval_resolve on the DefId with its identity substs before building the mir constants?
|
☔ The latest upstream changes (presumably #94081) made this pull request unmergeable. Please resolve the merge conflicts. |
b-naber
force-pushed
the
use-mir-constant-in-thir
branch
from
6fff021
to
cd31117
Compare
b-naber
force-pushed
the
use-mir-constant-in-thir
branch
from
cd31117
to
14e3d03
Compare
This comment has been minimized.
This comment has been minimized.
bors
added
S-waiting-on-bors
|
⌛ Testing commit 3be987e with merge 2562bc2236f1ef40d35cd8c776c080524c0f4717... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry msvc fulldeps test failure: I/O error: operation failed to complete synchronously |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 3be987e with merge 52abb2fa4028f997c9d3887e089674e9dfda1902... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry no output |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (e3c43e6): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Footnotes |
|
Visiting for weekly performance triage. This caused a 0.2% regression on the primary benchmarks webrender-2022 check (full) and cargo-0.60.0 check (incr-full). An earlier perf run, on I think the same commit series as what was eventually merged, showed no primary regressions at all. A number of secondary benchmarks regressed, but they are all stress test microbenchmarks, and should not block landing work of this nature. So, I'm chalking the minor amount of regression that was observed up to noise. @rustbot label +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
…li-obk Use mir constant in thir instead of ty::Const This is blocked on rust-lang#94059 (does include its changes, the first two commits in this PR correspond to those changes) and rust-lang#93800 being reinstated (which had to be reverted). Mainly opening since `@lcnr` offered to give some feedback and maybe also for a perf-run (if necessary). This currently contains a lot of duplication since some of the logic of `ty::Const` had to be copied to `mir::ConstantKind`, but with the introduction of valtrees a lot of that functionality will disappear from `ty::Const`. Only the last commit contains changes that need to be reviewed here. Did leave some `FIXME` comments regarding future implementation decisions and some things that might be incorrectly implemented. r? `@oli-obk`
This is blocked on #94059 (does include its changes, the first two commits in this PR correspond to those changes) and #93800 being reinstated (which had to be reverted). Mainly opening since @lcnr offered to give some feedback and maybe also for a perf-run (if necessary).
This currently contains a lot of duplication since some of the logic of
ty::Consthad to be copied tomir::ConstantKind, but with the introduction of valtrees a lot of that functionality will disappear fromty::Const.Only the last commit contains changes that need to be reviewed here. Did leave some
FIXMEcomments regarding future implementation decisions and some things that might be incorrectly implemented.r? @oli-obk