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

mir::Constant::span is a footgun for mir optimizations #77608

Open
oli-obk opened this issue Oct 6, 2020 · 8 comments
Open

mir::Constant::span is a footgun for mir optimizations #77608

oli-obk opened this issue Oct 6, 2020 · 8 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Oct 6, 2020

The field is part of the PartialEq impl, and thus two constants that are equal except in their span will compare inequal. This causes problems in #77551 (comment) and #77549 (comment) and possibly in other places.

The "trivial" way to resolve this is to manually implement PartialEq and make it skip the Span field.
Downside: now we may cause weird diagnostics after such an optimization has been applied, because if one of the constants is eliminated, all diagnostics are reported on the other diagnostic's site. We'd need a multispan and properly combine it. That will be hard to ensure to happen if we just silently change PartialEq

cc @rust-lang/wg-mir-opt

@oli-obk oli-obk added A-diagnostics Area: Messages for errors, warnings, and lints A-mir-opt Area: MIR optimizations labels Oct 6, 2020
@tmiasko
Copy link
Contributor

tmiasko commented Nov 3, 2020

The issue also applies to implicit const () at the end of block. Replacing it with an empty tuple aggregate in a MIR builder triggers a few extra optimizations in mir-opt test suite.

@oli-obk oli-obk added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Nov 3, 2020
@camelid
Copy link
Member

camelid commented Nov 15, 2020

So are we going to use the "trivial" fix – adding a custom impl of PartialEq that ignores span – or some kind of more complex one – I think you mentioned a "multispan"?

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 16, 2020

Ideally we'd remove the PartialEq impl and leave a comment that it can't have that impl, but I'm not sure what the fallout of that is.

@camelid
Copy link
Member

camelid commented Nov 16, 2020

Perhaps we could add a method on Constant that checks equality while ignoring span?

@camelid camelid added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Nov 16, 2020
@simonvandel
Copy link
Contributor

Only adding a new method that ignores span equality does not remove the footgun though. Other code would still use the PartialEq which still compares unintuitively because of the span.

@camelid
Copy link
Member

camelid commented Nov 16, 2020

I meant that we should remove PartialEq as Oli suggested and then add a new method that explicitly ignores span.

@camelid
Copy link
Member

camelid commented Nov 16, 2020

@spastorino Is it okay if I work on this a bit?

@spastorino
Copy link
Member

spastorino commented Jun 8, 2022

If someone wants to work on this, feel free to reuse my code. I don't remember how useful it is anymore since it has more than 1 year :). Anyway, feel free to contact me on a Zulip topic.

@spastorino spastorino removed their assignment Aug 30, 2022
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants