-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Added MIR constant propagation of Scalars into function call arguments #71697
Conversation
This is a continuation of #71522 |
CC @oli-obk <3 |
The Edit: Azure CI passed, so it was indeed just some network issue on the servers. Did a forced push to restart the CI testing. |
@wesleywiser does Github ping you when I re-request a review? (I ask so that I know to tag you or not for the next PRs in which you review me) Anyway... I think this is done! I have written down the full rationale. Lemme know what you think :) |
omg I'm an idiot. Brb. Formatting (this always happens to me whenever I write documentation 😅 ) |
- Documented rationale of current solution - Polished documentation
Fixed! |
The following code would appear to be incomplete, because | ||
the function `Operand::place()` returns `None` if the | ||
`Operand` is of the variant `Operand::Constant`. In this | ||
context however, that variant will never appear. This is why: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 42
is a const in the MIR's function call arguments in
fn main() {
foo(42);
}
fn foo(i: i32) {}
But that's already "propagated" so it's fine and we don't need to care about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to realize what you meant. But indeed! Those are already "propagated": https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9bfca57ccf6c252f7e601ed58beb7895
Should I add something to the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think the comment should be adjusted. It's not that an Operand::Constant
doesn't appear, it's that when it does, it's already constant and we don't need to propagate anything.
Yeah, I get pinged I was just afk today. This looks good to me but I'll do one last review tomorrow! |
Okay, that's good to know :3 |
Looking great! @bors r+ rollup (functional changes are gated under |
📌 Commit d0dea9f has been approved by |
…wesleywiser Added MIR constant propagation of Scalars into function call arguments Now for the function call arguments! Caveats: 1. It's only being enabled at `mir-opt-2` or higher, because currently codegen gives performance regressions with this optimization. 2. Only propagates Scalars. Tuples and references (references are `Indirect`, right??) are not being propagated into as of this PR. 3. Maybe more tests would be nice? 4. I need (shamefully) to ask @wesleywiser to write in his words (or explain to me, and then I can write it down) why we want to ignore propagation into `ScalarPairs` and `Indirect` arguments. r? @wesleywiser
Rollup of 5 pull requests Successful merges: - rust-lang#71038 (forbid `dyn Trait` in patterns) - rust-lang#71697 (Added MIR constant propagation of Scalars into function call arguments) - rust-lang#71773 (doc: misc rustdoc things) - rust-lang#71810 (Do not try to find binop method on RHS `TyErr`) - rust-lang#71877 (Use f64 in f64 examples) Failed merges: r? @ghost
Now for the function call arguments!
Caveats:
mir-opt-2
or higher, because currently codegen gives performance regressions with this optimization.Indirect
, right??) are not being propagated into as of this PR.ScalarPairs
andIndirect
arguments.r? @wesleywiser