Skip to content

Concrete regions can show up in mir borrowck if the originated from there - #88533

Merged
bors merged 1 commit into
rust-lang:masterfrom
oli-obk:tait_🧊
Sep 1, 2021

Hidden character warning

The head ref may contain hidden characters: "tait_\ud83e\uddca"
Merged

Concrete regions can show up in mir borrowck if the originated from there#88533
bors merged 1 commit into
rust-lang:masterfrom
oli-obk:tait_🧊

Conversation

@oli-obk

Copy link
Copy Markdown
Contributor

We used to not encounter them here, because we took regions from typeck's opaque type resolution by renumbering them. We don't do that anymore. Instead mir borrock does all the logic, and it can handle concrete regions just fine, as long as it created them itself.

fixes#83190 which was introduced by #87287

r? @spastorino

…here.
We used to not encounter them here, because we took regions from typeck's opaque type resolution by renumbering them. We don't do that anymore.
@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2021
@oli-obkoli-obk added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Aug 31, 2021
@spastorino

Copy link
Copy Markdown
Member

It would be nice to add a regression test for this particular thing. We could reduce the code from #83190, the bioyino-metric create, see #83190 (comment).

@rustbot ping cleanup-crew

To try to reduce that example and come up with a regression test.

Anyway, we're in a rush to merge this and backport to beta so ...

@bors r+ rollup=never

@bors

bors commented Aug 31, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 43738c6 has been approved by spastorino

@rustbot

Copy link
Copy Markdown
Collaborator

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2021
@spastorino

Copy link
Copy Markdown
Member

This fixes bioyino-metric and warp already works on nightly.
cc @Mark-Simulacrum

@jackh726

Copy link
Copy Markdown
Member

@bors p=1
Want to make sure this gets in soon for backport

@bors

bors commented Aug 31, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 43738c6 with merge a395610...

@bors

bors commented Sep 1, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: spastorino
Pushing a395610 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Sep 1, 2021
@bors
bors merged commit a395610 into rust-lang:masterSep 1, 2021
@rustbotrustbot added this to the 1.56.0 milestone Sep 1, 2021
@Mark-SimulacrumMark-Simulacrum modified the milestones: 1.56.0, 1.55.0Sep 4, 2021
@Mark-SimulacrumMark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 4, 2021
@Mark-SimulacrumMark-Simulacrum mentioned this pull request Sep 4, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2021
…ulacrum
[beta] backports
This PR backports:
* Concrete regions can show up in mir borrowck if the originated from there rust-lang#88533 (fixesrust-lang#83190)
* Fix loading large rlibs rust-lang#88506 (fixesrust-lang#88351)
* Display associated types of implementors rust-lang#88490 (fixesrust-lang#86631)
* Tracking issue for UNSUPPORTED_CALLING_CONVENTIONS rust-lang#88397
r? `@Mark-Simulacrum`
@pnkfelix

Copy link
Copy Markdown
Contributor

During weekly performance triage, this was identified as causing a very small regression in instruction counts (up to 1.0% on incr-patched: add static arr item builds of coercions). As I noted in the triage report, this is a small regression to coercions and that may be noise. However, there are many others that are over 0.4% regression to instruction counts. The combination of those two factors led me to think that we may want to take a second look at the effects of this PR.

@pnkfelixpnkfelix added the perf-regression Performance regression. label Sep 8, 2021
@oli-obk
oli-obk deleted the tait_🧊 branch September 9, 2021 10:11
@oli-obk

Copy link
Copy Markdown
ContributorAuthor

Since these examples compiled before this PR, and this PR solely removes a delay_span_bug, the only way perf can be affected is by llvm optimizing rustc differently. There can be no change in behaviour for existing code that successfully compiled

@Mark-Simulacrum

Copy link
Copy Markdown
Member

The new significance algorithm essentially concurs that this was not after all a performance regression. If we do include very small changes, then there's a couple regressions, but those are pretty likely to be unrelated to this PR -- it looks like the majority of the new instructions come from llvm::PMTopLevelManager::findAnalysisPass and llvm::AAResultsWrapperPass::runOnFunction, which seem obviously irrelevant for this change.

@Mark-SimulacrumMark-Simulacrum removed the perf-regression Performance regression. label Oct 3, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beta-acceptedAccepted for backporting to the compiler in the beta channel.merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ICE: unexpected concrete region in borrowck: ReEarlyBound(0, 'a)

8 participants

@oli-obk@spastorino@bors@rustbot@jackh726@pnkfelix@Mark-Simulacrum@rust-highfive