-
Notifications
You must be signed in to change notification settings - Fork 13.3k
trans: generalize immediate temporaries to all MIR locals. #34189
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
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
let index = mir.local_index(&mir::Lvalue::ReturnPointer).unwrap(); | ||
let op = match self.locals[index] { | ||
LocalRef::Operand(Some(op)) => op, | ||
LocalRef::Operand(None) => bug!("use of return before def"), |
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.
I feel like at this point we’re starting to rely on weird invariants like ”block with return = x
statement must have smaller number (i.e. translated earlier) than block with return
terminator”, aren’t we?
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.
We've always relied on that, and it's why we use RPO.
I want the Also, to me it feels like the |
@nagisa They're only used in trans, but that's just because that's the place I needed them in. |
I figured out that this generalisation is potentially useful for certain MIR passes (cough dataflow cough), but I’d still like |
@bors r=nagisa |
📌 Commit 8b50516 has been approved by |
⌛ Testing commit 8b50516 with merge f0be110... |
💔 Test failed - auto-linux-64-opt-mir |
@bors r=nagisa |
📌 Commit cb033cb has been approved by |
⌛ Testing commit cb033cb with merge 47d75cc... |
💔 Test failed - auto-linux-64-opt-mir |
@bors r=nagisa |
📌 Commit 7279af8 has been approved by |
trans: generalize immediate temporaries to all MIR locals. Added `Mir::local_index` which gives you an unified index for `Arg`, `Var`, `Temp` and `ReturnPointer`. Also available is `Mir::count_locals` which returns the total number of the above locals. This simplifies a lot of the code which can treat all of the local lvalues in the same manner. If we had `-> impl Iterator`, I could have added a bunch of useful `Ty` or `Lvalue` iterators for all locals. We could of course manually write such iterators as they are needed. The only place which currently takes advantage of unified locals is trans' alloca elision. Currently it's not as good as it could be, due to our usage of `llvm.dbg.declare` in debug mode. But passing some arguments and variables as immediates has some effect on release-mode `libsyntax`: Old trans: ``` time: 11.500; rss: 710MB translation time: 0.002; rss: 710MB assert dep graph time: 0.000; rss: 710MB serialize dep graph time: 4.410; rss: 628MB llvm function passes [0] time: 84.485; rss: 633MB llvm module passes [0] time: 23.898; rss: 634MB codegen passes [0] time: 0.002; rss: 634MB codegen passes [0] time: 113.408; rss: 634MB LLVM passes ``` `-Z orbit`, previously: ``` time: 12.588; rss: 723MB translation time: 0.002; rss: 723MB assert dep graph time: 0.000; rss: 723MB serialize dep graph time: 4.597; rss: 642MB llvm function passes [0] time: 77.347; rss: 646MB llvm module passes [0] time: 24.703; rss: 648MB codegen passes [0] time: 0.002; rss: 615MB codegen passes [0] time: 107.233; rss: 615MB LLVM passes ``` `-Z orbit`, after this PR: ``` time: 13.820; rss: 672MB translation time: 0.002; rss: 672MB assert dep graph time: 0.000; rss: 672MB serialize dep graph time: 3.969; rss: 591MB llvm function passes [0] time: 72.294; rss: 595MB llvm module passes [0] time: 24.610; rss: 597MB codegen passes [0] time: 0.002; rss: 597MB codegen passes [0] time: 101.439; rss: 597MB LLVM passes ```
Added
Mir::local_index
which gives you an unified index forArg
,Var
,Temp
andReturnPointer
.Also available is
Mir::count_locals
which returns the total number of the above locals.This simplifies a lot of the code which can treat all of the local lvalues in the same manner.
If we had
-> impl Iterator
, I could have added a bunch of usefulTy
orLvalue
iterators for all locals.We could of course manually write such iterators as they are needed.
The only place which currently takes advantage of unified locals is trans' alloca elision.
Currently it's not as good as it could be, due to our usage of
llvm.dbg.declare
in debug mode.But passing some arguments and variables as immediates has some effect on release-mode
libsyntax
:Old trans:
-Z orbit
, previously:-Z orbit
, after this PR: