Stream: t-compiler/wg-prioritization/alerts

Topic: I-prioritize #78192 InstCombine introduces an incorrect use…


triagebot (Oct 21 2020 at 15:46, on Zulip):

@WG-prioritization/alerts issue #78192 has been requested for prioritization.

Procedure

Hameer Abbasi (Oct 21 2020 at 16:03, on Zulip):

Definitely P-critical.

apiraino (Oct 21 2020 at 16:04, on Zulip):

Is this a regression? I see in the linked PR #76683 oli expressed some concerns

apiraino (Oct 21 2020 at 16:06, on Zulip):

right. I don't have a lot of context, but it smells bad. I'd like to understand which Rust release impacts (stable, beta or nightly ... or all of them)

Stu (Oct 21 2020 at 16:16, on Zulip):

I'm not 100 per cent sure, but I think its from stable-to-beta

LeSeulArtichaut (Oct 21 2020 at 16:17, on Zulip):

If it regressed in #76683 which landed a month ago, it’s in beta

Joshua Nelson (Oct 21 2020 at 16:17, on Zulip):

https://github.com/rust-lang/rust/pull/76683 is only present on master

Joshua Nelson (Oct 21 2020 at 16:18, on Zulip):

You can tell by looking at the commit

Joshua Nelson (Oct 21 2020 at 16:18, on Zulip):

It will say the tags

Joshua Nelson (Oct 21 2020 at 16:19, on Zulip):

Oh hmm that doesn't match the milestone though

LeSeulArtichaut (Oct 21 2020 at 16:19, on Zulip):

Are you sure? It’s tagged with milestone 1.48.0

Joshua Nelson (Oct 21 2020 at 16:19, on Zulip):

Wonder what's up with that

LeSeulArtichaut (Oct 21 2020 at 16:19, on Zulip):

:shrug:

LeSeulArtichaut (Oct 21 2020 at 16:19, on Zulip):

It was merged one month ago

LeSeulArtichaut (Oct 21 2020 at 16:19, on Zulip):

I think it’s most likely in beta

Stu (Oct 21 2020 at 16:19, on Zulip):

I tested in the playground and it seems like it is in beta

LeSeulArtichaut (Oct 21 2020 at 16:19, on Zulip):

When did beta last branch? Two weeks ago?

LeSeulArtichaut (Oct 21 2020 at 16:20, on Zulip):

Anyway I guess we should consider the worst case scenario, i.e. the bug is in beta and it is coming for you

LeSeulArtichaut (Oct 21 2020 at 16:21, on Zulip):

Does everyone agree on P-critical?

apiraino (Oct 21 2020 at 16:27, on Zulip):

we now have the shiny new regression-* labels, we can also use them :-)

oliver (Oct 21 2020 at 16:32, on Zulip):

/me is relieved

LeSeulArtichaut (Oct 21 2020 at 16:52, on Zulip):

@oliver Were you worried?

oliver (Oct 21 2020 at 16:53, on Zulip):

wasn't everyone?

LeSeulArtichaut (Oct 21 2020 at 16:54, on Zulip):

Finding this bug in beta is still fine

LeSeulArtichaut (Oct 21 2020 at 16:55, on Zulip):

Finding it in stable would have been very worrying indeed

LeSeulArtichaut (Oct 21 2020 at 16:56, on Zulip):

But finding this in beta means we can disable the mir optimization, which is simple to backport

oliver (Oct 21 2020 at 16:57, on Zulip):

I was replying to the comment about adding a public regressions-* tag

DPC (Oct 21 2020 at 17:30, on Zulip):

@Joshua Nelson milestones will be accurate since 1.48 onwards they are auto-added

Jonas Schievink (Oct 21 2020 at 18:55, on Zulip):

Joshua Nelson said:

Oh hmm that doesn't match the milestone though

The beta channel has no git tags

Santiago Pastorino (Oct 21 2020 at 20:20, on Zulip):

I guess we should process and tag this as P-critical and label it as a regression-from-stable-to-beta, right?

Camelid (Oct 21 2020 at 20:21, on Zulip):

apiraino said:

we now have the shiny new regression-* labels, we can also use them :-)

Were you referring to regression-untriaged?

triagebot (Oct 21 2020 at 20:23, on Zulip):

Issue #78192's prioritization request has been removed.

Santiago Pastorino (Oct 21 2020 at 20:36, on Zulip):

can somebody summarize this issue?

Santiago Pastorino (Oct 21 2020 at 20:37, on Zulip):

I'm trying to go over pending issues

apiraino (Oct 21 2020 at 20:37, on Zulip):

Based on Stu's comment I think that ...

Santiago Pastorino said:

I guess we should process and tag this as P-critical and label it as a regression-from-stable-to-beta, right?

:point_up: this

apiraino (Oct 21 2020 at 20:37, on Zulip):

Santiago Pastorino said:

can somebody summarize this issue?

I'll add a bit of detail

Santiago Pastorino (Oct 21 2020 at 20:37, on Zulip):

:+1:

Santiago Pastorino (Oct 21 2020 at 20:37, on Zulip):

it would be nice to explain what's wrong with doing what the optimization does in the summary

apiraino (Oct 21 2020 at 20:42, on Zulip):

Santiago Pastorino said:

it would be nice to explain what's wrong with doing what the optimization does in the summary

not sure I have enough context for that, but I'll try. You can always improve and fill in the blanks :thumbs_up:

apiraino (Oct 21 2020 at 21:12, on Zulip):

So, there is a PR already merged #78195 to fix this, not sure about the further work needed to adress this issue. Can anyone please add some context for this case? There are two linked PR and an issue but I can't reorder their logic :thinking:

LeSeulArtichaut (Oct 21 2020 at 21:13, on Zulip):

We need to backport the PR

LeSeulArtichaut (Oct 21 2020 at 21:14, on Zulip):

I'm pretty sure we could just tag this as P-medium now

LeSeulArtichaut (Oct 21 2020 at 21:14, on Zulip):

The PR will get discussed in the meeting anyway

LeSeulArtichaut (Oct 21 2020 at 21:14, on Zulip):

The context should go there

Camelid (Oct 21 2020 at 21:19, on Zulip):

We also need a test I think

LeSeulArtichaut (Oct 21 2020 at 21:20, on Zulip):

The issue is not going to be closed anyway, I think

LeSeulArtichaut (Oct 21 2020 at 21:20, on Zulip):

The mir-opt WG will have to fix the pass

LeSeulArtichaut (Oct 21 2020 at 21:20, on Zulip):

I think once the PR disabling the pass is backported, we should just tag the issue as requires-nightly and call it a day :D

Camelid (Oct 21 2020 at 21:22, on Zulip):

And maybe P-medium as you suggested (once the backport has happened)

LeSeulArtichaut (Oct 21 2020 at 21:23, on Zulip):

I think in the past we had assigned P-medium to issues waiting for backports

Camelid (Oct 21 2020 at 21:24, on Zulip):

Well it's better to be safe and make sure it doesn't get lost :)

LeSeulArtichaut (Oct 21 2020 at 21:24, on Zulip):

So what I'd suggest is to change the priority to P-medium now (so we don't "pollute" the agenda)

LeSeulArtichaut (Oct 21 2020 at 21:24, on Zulip):

Camelid said:

Well it's better to be safe and make sure it doesn't get lost :)

We have the backport request

Camelid (Oct 21 2020 at 21:25, on Zulip):

I don't know; my thinking is it's still P-critical on beta until the backport has happened, so we should leave the label as such

LeSeulArtichaut (Oct 21 2020 at 21:25, on Zulip):

On the other hand that's no longer the issue that's important to focus on, but the PR

apiraino (Oct 21 2020 at 21:27, on Zulip):

uh right, but ... uhm ... what is this issue about? If I understand there is a MIR optimization #77369 that causes the memory issue (?)
And what about pr #78147 and #76683?

Camelid (Oct 21 2020 at 21:27, on Zulip):

There's a MIR opt that had unsoundness and thus needed to be temporarily disabled

Camelid (Oct 21 2020 at 21:28, on Zulip):

It was disabled in #78195

oliver (Oct 21 2020 at 21:28, on Zulip):

apiraino said:

what about pr #78147 and #76683?

I don't understand either of these

Camelid (Oct 21 2020 at 21:29, on Zulip):

Same here :)

LeSeulArtichaut (Oct 21 2020 at 21:29, on Zulip):

It's MIR-opt stuff :D

LeSeulArtichaut (Oct 21 2020 at 21:29, on Zulip):

I think they run a validator for each beta to try to catch bugs like these

Camelid (Oct 21 2020 at 21:30, on Zulip):

I know, I'm trying to learn about this stuff for my own compiler :)

Camelid (Oct 21 2020 at 21:30, on Zulip):

You mean with crater?

LeSeulArtichaut (Oct 21 2020 at 21:30, on Zulip):

Camelid said:

You mean with crater?

Unsure

LeSeulArtichaut (Oct 21 2020 at 21:31, on Zulip):

I think I read something or someone talked about it here

LeSeulArtichaut (Oct 21 2020 at 21:31, on Zulip):

IIUC #77369 implemented a new check to the MIR validator

LeSeulArtichaut (Oct 21 2020 at 21:32, on Zulip):

And #78147 too probably

LeSeulArtichaut (Oct 21 2020 at 21:32, on Zulip):

Seems like #78147 was failing because of this bug

LeSeulArtichaut (Oct 21 2020 at 21:33, on Zulip):

#76683 is the PR that introduced the bug

LeSeulArtichaut (Oct 21 2020 at 21:34, on Zulip):

The pass was disabled in #78195

Camelid (Oct 21 2020 at 21:34, on Zulip):

Camelid said:

It was disabled in #78195

^

Camelid (Oct 21 2020 at 21:36, on Zulip):

LeSeulArtichaut said:

Camelid said:

You mean with crater?

Unsure

Asked here.

apiraino (Oct 21 2020 at 21:38, on Zulip):

thank you @LeSeulArtichaut :cake:

Santiago Pastorino (Oct 22 2020 at 00:13, on Zulip):

I think what we have done in the past is leave this P-critical until the backport lands on beta and then we need a test and once that test lands the issue can be just closed

Santiago Pastorino (Oct 22 2020 at 00:13, on Zulip):

or well, maybe we want to have an issue to track the mir opt issue, like to do a real fix instead of disabling it

Camelid (Oct 22 2020 at 00:29, on Zulip):

Sounds good

triagebot (Oct 26 2020 at 21:36, on Zulip):

@WG-prioritization/alerts issue #78192 has been requested for prioritization.

Procedure

Jonas Schievink (Oct 26 2020 at 21:39, on Zulip):

P-critical again

Jonas Schievink (Oct 26 2020 at 21:39, on Zulip):

sigh

Camelid (Oct 26 2020 at 22:19, on Zulip):

I guess this case was not caught when the pass was re-enabled?

triagebot (Oct 26 2020 at 22:21, on Zulip):

Issue #78192's prioritization request has been removed.

LeSeulArtichaut (Oct 27 2020 at 00:22, on Zulip):

Do we want to ping MIR-opt folks for this?

Jonas Schievink (Oct 27 2020 at 10:00, on Zulip):

Opened https://github.com/rust-lang/rust/pull/78434 to disable the opt

apiraino (Oct 28 2020 at 09:57, on Zulip):

Thanks for the patch @Jonas Schievink :)

Last update: Apr 10 2021 at 23:00UTC