Address more comments from #226. #253
Conversation
| @@ -409,6 +409,7 @@ impl TraceCompiler { | |||
| self.stack_builder.alloc(ty.size(), ty.align()) | |||
| } | |||
|
|
|||
| /// Implements the StorageLive statement, assigning a Location the local. | |||
There was a problem hiding this comment.
I'm not sure how to parse this sentence.
There was a problem hiding this comment.
How about:
Called when a local variable becomes live. The register allocator is invoked to make storage space for the local.
There was a problem hiding this comment.
Which part is the problem? This basically tells the compiler a variable is live and assigns it a register or location on the stack. How about: Implements a local going live, assigning it to a register or, if no registers are free, to a location on the stack.
There was a problem hiding this comment.
@vext01 I think, in general, more active descriptions are clearer.
@Lukasd Words like "implements" feel a bit like padding. I'm also not sure what "a local going live" exactly means.
Taking those thoughts together, would something like "Assign a Location to a StorageLive (i.e. finding it a register or space on the stack)." be accurate? Maybe too terse?
There was a problem hiding this comment.
StorageLive is a MIR statement, so saying we assign a location to it, doesn't sound right. We are assigning a location to the local that MIR has decided is live from that point onwards until we see a StorageDead for it.
There was a problem hiding this comment.
It means that we don't strictly have to allocate a local until its first use, and could in fact remove the function in question altogether.
It might result in better allocation results overall. Consider the following:
live($1)
if (...) {
$1 = call foo(...)
call blah($1)
} else {
// don't use $1
}
If we trace the else branch, then right now, then the trace would allocate space for, but never use $1. In other words, it could potentially block another variable from using a register.
If we allocate on first use, we wouldn't allocate $1 at all (in this trace), leaving it free for other locals.
But, I don't know if MIR would generate code like this...
Another question I then have is, if we don't allocate eagerly, what is the purpose of StorageLive in our bytecode?
There was a problem hiding this comment.
Lukas and I raced, but agreed.
So perhaps StorageDead is useful for #243. But then again, we could just consider the first use of a variable as its birth?
There was a problem hiding this comment.
I have no idea so I'm relying on you two to work this out :)
There was a problem hiding this comment.
live($1) if (...) { $1 = call foo(...) call blah($1) } else { // don't use $1 }
I don't know why MIR wouldn't put the live($1) in the same block where it's assigned and used, but you never know. Although a quick test on Rust Playground seems to confirm this suspicion (which again doesn't mean MIR always does this).
|
Ready for re-review. |
|
Go ahead. |
|
Squashed. |
|
bors r+ |
|
Build succeeded: |
A quick PR addressing comments from the audit (#226), so I don't loose track of what I've fixed and what not.