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

Hide comments about constants in MIR dumps if they bring no additional information #74508

Closed
oli-obk opened this issue Jul 19, 2020 · 9 comments
Labels
A-mir-opt Area: MIR optimizations C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. 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 Jul 19, 2020

cc @lcnr @eddyb

We sometimes have super verbose things in MIR like

         assert(const true, "index out of bounds: the len is {} but the index is {}", const 4_usize, const 2_usize) -> bb1; // scope 0 at $DIR/array_index.rs:5:18: 5:33
                                          // ty::Const
                                          // + ty: bool
                                          // + val: Value(Scalar(0x01))
                                          // mir::Constant
                                          // + span: $DIR/array_index.rs:5:18: 5:33
                                          // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
                                          // ty::Const
                                          // + ty: usize
                                          // + val: Value(Scalar(0x0000000000000004))
                                          // mir::Constant
                                          // + span: $DIR/array_index.rs:5:18: 5:33
                                          // + literal: Const { ty: usize, val: Value(Scalar(0x0000000000000004)) }
                                          // ty::Const
                                          // + ty: usize
                                          // + val: Value(Scalar(0x0000000000000002))
                                          // mir::Constant
                                          // + span: $DIR/array_index.rs:5:18: 5:33
                                          // + literal: Const { ty: usize, val: Value(Scalar(0x0000000000000002)) }

These constants give no useful additional information, as everything is already contained in the mir terminator. I think we could start out with just not printing these statements if the type is an integer, bool or char, which should cover most use cases. A more complex analysis could later look at constant in more detail and decide whether the verbose printing could be helpful.

@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-mir-opt Area: MIR optimizations labels Jul 19, 2020
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 19, 2020
@mglagla
Copy link
Contributor

mglagla commented Jul 21, 2020

I'd like to try this.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 21, 2020

ooh, sorry, evanrichter (idk their github handle) is working on it already since yesterday.

@evanrichter
Copy link
Contributor

@mglagla we can work together if you want, as I'm new to rustc dev. would you like to claim by saying @rustbot claim?

@mglagla
Copy link
Contributor

mglagla commented Jul 21, 2020

@evanrichter No, it's alright, I merely wanted to test if I could find the relevant part of code to edit and I did. I only would make a pull request if nobody worked on this already.

@evanrichter
Copy link
Contributor

@mglagla sounds like you are ahead of me and I am busy this week. If the issue is still available next week I'll claim it

@xldenis
Copy link
Contributor

xldenis commented Jul 26, 2020

@evanrichter do think you'll have time to work on this?

@evanrichter
Copy link
Contributor

evanrichter commented Jul 26, 2020

@xldenis not until Wednesday at least

@alasher
Copy link
Contributor

alasher commented Aug 15, 2020

Hey guys, sorry to snipe this - there hadn't been any activity in a few weeks so I had a go at fixing this.

@oli-obk I've implemented your suggestion in the linked PR. One note though is that your post above did not specify if we should suppress only ty::Const messages, only mir::Constant messages, or both. My change removes them both for the types you listed, but I could see why we'd want to keep mir::Constant messages. I'm not familiar enough with the project to know what people would prefer, I'm happy to change it to what you think is best.

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 18, 2020
Suppress verbose MIR comments for trivial types

Addresses rust-lang#74508

This is my first contribution to the Rust project! Please let me know if anything needs revising, I'm happy to make changes.
@alasher
Copy link
Contributor

alasher commented Aug 27, 2020

My commit landed just over a week ago, we should be able to close this issue.

@oli-obk oli-obk closed this as completed Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants