Remove some Regions from HAIR
#56638
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@bors try - want to check perf |
|
⌛ Trying commit 1bfa367e19efaa5f670378c482e5c03652932c31 with merge 7c4c3b8da31eead5a65508346fe8352b7ef85023... |
matthewjasper
force-pushed
the
remove-ref-region
branch
from
1bfa367 to
74d3462
Compare
|
@bors try |
Remove `Region` from `mir::Rvalue::Ref` This was only used for borrow checking, but we will create a fresh `RegionVid` for each borrow anyway, so there's no need for us to store it in the MIR. Also removes `Region`s from various places where they're no longer needed. Uses `ReErased` for any regions that need to be created in MIR generation. r? @nikomatsakis
This comment has been minimized.
This comment has been minimized.
|
☀️ Test successful - status-travis |
|
@rust-lang/infra Please can I have a perf run? |
|
@rust-timer build 34d39ce |
|
Success: Queued 34d39ce with parent 9772d02, comparison URL. |
|
Finished benchmarking try commit 34d39ce |
|
Hmm, results aren't great. This seems to have affected incremental for the worse. |
|
Those benchmarks are high-variance -- I would expect that any delta seen is just noise. If this is not intended as a performance optimization, then the performance aspect can be ignored; it has not changed. |
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Not sure what I think yet -- see below. I'd like to ponder it and/or discuss the motivation a bit =)
src/librustc/mir/tcx.rs
Outdated
| Rvalue::Ref(bk, ref place) => { | ||
| let place_ty = place.ty(local_decls, tcx).to_ty(tcx); | ||
| tcx.mk_ref(reg, | ||
| tcx.mk_ref(tcx.types.re_erased, |
There was a problem hiding this comment.
Hmm, I'm not sure how I feel about this. It seems to add some non-trivial complexity that the result of rvalue.ty cannot be used in the type-checker and kind of may introduce erased regions, even when all regions that appear in the MIR have been "renumbered".
What's the motivation exactly for removing this from the MIR? (As opposed to (eventually) encoding a 'erased here?)
There was a problem hiding this comment.
The reason for this was that it seemed a bit wasteful to have Regions that are almost always erased in MIR (which was my original intention).
| | Rvalue::Discriminant(..) => {} | ||
| } | ||
|
|
||
| rvalue.ty(mir, tcx) |
There was a problem hiding this comment.
...this is the case that bothers me. Now we have to know that sometimes we can use this, sometimes we can't...
|
Sorry for leaving this for so long. I appreciate the motivation here, but it doesn't quite feel worth it to me. Soon enough, all regions will be erased anyway, so we're just saving a word. (Maybe we can even make the MIR parametric over the region type at some point.) Curious to hear what @pnkfelix or other @rust-lang/wg-compiler-nll folks think. |
Use `ReErased` for any regions that need to be created in RValue::Ref in MIR generation.
matthewjasper
force-pushed
the
remove-ref-region
branch
from
74d3462 to
4f3c469
Compare
matthewjasper
changed the title
Region from mir::Rvalue::RefRegions from HAIR
|
|
|
@bors r+ |
|
📌 Commit 4f3c469 has been approved by |
bors
added
S-waiting-on-bors
Remove some `Region`s from HAIR Use `ReErased` for any regions that need to be created in RValue::Ref in MIR generation. We will change them to all to `ReVar` before borrow checking anyway. r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
7 participants
Use
ReErasedfor any regions that need to be created in RValue::Refin MIR generation. We will change them to all to
ReVarbefore borrowchecking anyway.
r? @nikomatsakis