Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert #103880 "Use non-ascribed type as field's type in mir" #105905

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

lqd
Copy link
Member

@lqd lqd commented Dec 19, 2022

This PR prepares a revert for #103880 to fix #105809, #105881, #105886 and others (like the duplicates of the first one), in case an actual fix can't get done today.

I've also added the MCVE from #105809. There is no MCVE for the #105881 and #105886 ICEs yet however, so there are no tests for them here, although we'll need one before relanding the original changes.

Were this PR to land, it would also reopen #96514 as it was fixed by the original PR.

Opening as draft to allow time for a possible fix.

r? @jackh726

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 19, 2022
@jackh726
Copy link
Member

@bors p=1 rollup=never

r=me in 6-8 hours if no fix has been put up

// Non-regression test ICE from issue #105809 and duplicates.

// build-pass: the ICE is during codegen
// compile-flags: --edition 2018
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add -Zmir-opt-level=1 here? It was reported in one of the issues that this only ICE'd with that option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can, but IIRC 1 is the default, and this test does ICE as is on master.

So I take it we'll land the revert and won't have time to do a fix ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking into it right now, but am not sure whether I can fix it in the next couple of hours. If I don't open a PR in the next two hours we can revert it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about it too much, it's perfectly fine, we can land the revert to buy us time to look into it without pressure.

I've now added -Zmir-opt-level=1 to the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting somewhat closer to figuring out what is going wrong. Can you give me 30 more minutes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah but of course! #105881 and #105886 don't have MCVEs yet so hopefully these 3 issues all have the same underlying issue.

I've marked this PR as ready, and if you want it to land, either you or @jackh726 can tell bors that whenever you need. The time frame jack refers to here #105905 (comment) would be 2AM my time.

Copy link
Contributor

@b-naber b-naber Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We add an additional OpaqueCast here because ty != pattern.ty. This then later ICEs because we cannot get the layout for that type when projecting that in codegen. I don't understand why getting the layout fails though. Will try to work more on this in the next few days.

Thanks for opening the revert PR @lqd.

@lqd lqd marked this pull request as ready for review December 19, 2022 19:40
@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2022

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

@jackh726
Copy link
Member

@bors delegate=b-naber

@b-naber From the comment thread, not sure if you're stuck. Feel free to r=me at any point. It's very easy to revert and fix later, so no stress.

@bors
Copy link
Contributor

bors commented Dec 19, 2022

✌️ @b-naber can now approve this pull request

@b-naber
Copy link
Contributor

b-naber commented Dec 19, 2022

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Dec 19, 2022

📌 Commit 5457db9 has been approved by jackh726

It is now in the queue for this repository.

@bors bors 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 Dec 19, 2022
@lqd lqd mentioned this pull request Dec 19, 2022
@lqd
Copy link
Member Author

lqd commented Dec 19, 2022

Bumping the priority to 8, this is a P-critical fix that should land before the #105918 rollup at p=7.

@bors p=8

@bors
Copy link
Contributor

bors commented Dec 19, 2022

⌛ Testing commit 5457db9 with merge 696563e...

@bors
Copy link
Contributor

bors commented Dec 20, 2022

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 696563e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 20, 2022
@bors bors merged commit 696563e into rust-lang:master Dec 20, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 20, 2022
@bors bors mentioned this pull request Dec 20, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (696563e): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.7% [-1.3%, -0.3%] 5
Improvements ✅
(secondary)
-1.7% [-3.5%, -0.4%] 14
All ❌✅ (primary) -0.7% [-1.3%, -0.3%] 5

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.0% [3.0%, 3.0%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.0% [3.0%, 3.0%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.5% [-2.5%, -2.5%] 1
All ❌✅ (primary) - - 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile error 'failed to get layout for'
6 participants