revert mir inlining policy for beta-1.64

[pnkfelix]

revert mir inlining policy for beta-1.64

Fix #101004

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rust-highfive]

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

[rust-highfive]

⚠️ Warning ⚠️

• Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

[pnkfelix]

I had to locally disable src/test/codegen/mem-replace-direct-memcpy.rs to get my test suite to pass when trying this locally, regardless of whether I was testing stage 1 or stage 2. I assume this is because of #99619

[compiler-errors]

@pnkfelix you might need to disable and/or bless mem-replace-direct-memcpy because it looks like it's angry on CI too.

r=me when CI is green though.

[pnkfelix]

@pnkfelix you might need to disable and/or bless mem-replace-direct-memcpy because it looks like it's angry on CI too.

r=me when CI is green though.

Yeah, I'm going to throw away that test on beta. It is just trying to ensure that the stdlib has no inefficient intermediate calls starting from core::ptr::read

[eddyb]

Yeah, I'm going to throw away that test on beta.

You can just revert the MIR inlining PR. It changes the test just like many other tests because it gets inlined now, I don't see how all the other changed tests could pass.

^{(Oh, wait, I see, some of them just got -Zinline-mir=no, which didn't require changing the test itself, only the ones where -Zinline-mir=no wasn't sufficient, because the optimizations had happened in libcore itself, are the issue)}

Still, I would try using git revert or at least copying the old version of the test (pre-MIR-inlining), if for no other reason than to have an extra layer of confirming it's actually getting back to the previous state.

[pnkfelix]

@eddyb I'll do that as a follow-up activity, for now I just want to get this in. (note that we're not making this change on nightly, just on beta, so I would argue that there is very little risk of this performance regression leaking back in.. but still, I can put a little time into it next week.)

[pnkfelix]

@bors r=compiler-errors

[bors]

📌 Commit 2741dd8 has been approved by compiler-errors

It is now in the queue for this repository.

[eddyb]

so I would argue that there is very little risk of this performance regression leaking back in

IIRC it's not even a performance regression, I think Rust-GPU needed to avoid some dynamism in terms of memcpy lengths (and we pick our nightlies so no need to worry about that).

The main thing I wanted to express is that if the goal is to "disable MIR inlining" and the only test that appears to observe that difference is this one, it should be reverted so that it can act as a "canary" for MIR inlining being disabled.

(I would even prefer if it wasn't this test, but it seems that most codegen tests just disable MIR optimizations now, which makes some sense but it's suboptimal for actually testing, well, MIR inlining, or other MIR optimizations)

[eddyb]

@bors r- (more testing was brought up in Zulip PM, taking it out of the queue for now)

[eddyb]

At the very least it would be good to include this reduction as a test:

• ICE when compiling nalgebra 0.25.4 in Release mode #100550 (comment)

Oh and maybe some of this will pass on beta? (But I'm not sure what the status is)

• Add tests for 99866 and 99852 #100998

[pnkfelix]

Yes I plan to at least add the test from #100550, and perhaps also try to adapt the one from #100476

[pnkfelix]

@eddyb wrote:

Oh and maybe some of this will pass on beta? (But I'm not sure what the status is)

• Add tests for 99866 and 99852 #100998

As far as I can tell, the examples in that ticket are not relevant to (or at least are not exposed by) the change in MIR inlining policy...

[pnkfelix]

@bors r=compiler-errors

[bors]

📌 Commit 2f96703 has been approved by compiler-errors

It is now in the queue for this repository.

[compiler-errors]

@bors rollup=never

[wesleywiser]

@bors p=1

Resolves P-critical issue in beta.

[bors]

⌛ Testing commit 2f96703 with merge 1ec9b66...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 1ec9b66 to beta...