Refactor i64 lowering to use RAII temp vars#1177
Conversation
| } | ||
|
|
||
| TempVar& operator=(TempVar&& rhs) { | ||
| assert(!rhs.moved); |
There was a problem hiding this comment.
What happens if:
TempVar a(1, pass);
TempVar b(2, pass);
TempVar c(3, pass);
a = b;
b = c;?
(having only looked at this struct in this PR)
There was a problem hiding this comment.
This code doesn't compile because the assignment operator has been deleted. However, if you used std::move to use move assignment, you're totally right that this is completely broken. It leaks 1 and 3 and frees 2. I'll fix this when I have time, probably tomorrow or Friday.
There was a problem hiding this comment.
Are you sure? This seems to work OK: https://godbolt.org/g/zFjpFw
There was a problem hiding this comment.
It would compile fine but it would leak the 1 when a was overwritten with b, since destroying a after that frees the 2 instead of the 1. However, the sample code you linked contains the fixed version that frees the old value before overwriting it in the move assignment operator.
There was a problem hiding this comment.
Ah sorry, didn't realize I was looking at the fixed code. :-)
| TempVar(Index idx, I64ToI32Lowering& pass) : | ||
| idx(idx), pass(pass), moved(false) {} | ||
|
|
||
| TempVar(TempVar&& other) : idx(other), pass(other.pass), moved(false) { |
There was a problem hiding this comment.
i wonder if it's important to handle moving here? is it for correctness, or to save locals? if just to save locals, we can depend on opts to fix that up later, and this logic could be simpler.
There was a problem hiding this comment.
No, being move-only is critical for the correctness of these vars. When an expression is lowered its high bits are stored in a new variable and this variable is stored in a lookup table to be retrieved by the expression's parent. The parent can choose to reuse or free the variable, but until the parent expression is processed, the variable must not be used for anything else.
By only allowing variables to be moved and not copied, we can ensure that a variable cannot be improperly reused. Without the move semantics it would be impossible to use RAII for these variables since they would be freed and potentially reused before the parent expression could consume them.
|
Overall looks good, but Travis is failing because: so that should probably be fixed |
|
Yeah there's all sorts of wonkiness going on with br_tables right now. I'm working on it! |
|
I had to update my strategy for lowering Switch expressions to always use trampoline blocks because the previous strategy relied on being able to store a single variable into multiple locations in a lookup table, which is no longer possible. |
|
@kripken @jgravelle-google any more thoughts on this change? |
|
lgtm, thanks! |
4 participants
No description provided.