[SSA] Fix bug in to_ssa.py #111
Conversation
Fixes Issue sampsyo#108. The optimization in prune_phis() is not sound. phi nodes that are "partially undefined" cannot be removed. It is only illegal to read from the result if the second to last executed label corresponds to an undefined argument. `to_ssa.py` generated incorrect code for the two tests added, before the fix. In most cases, these phi nodes aren't read from, and will be removed by DCE. However, in the case where the phi nodes are read, a correct optimization would be: 1. Detect partially undefined phi nodes whose value is always used. Let `undefined_labels` be the set of labels whose argument is undefined. 2. Leverage the undefined behavior to say that `undefined_labels` are in fact not predecessors to the basic block. `preds = preds - undefined_labels`. Though, I'm not sure how useful that optimization would be in practice.
|
You could also find phi nodes that have only one non-undefined argument and replace it with an |
You'd need to merge PR #112 to allow that optimization. You can decide if you think it is a good idea or not. |
| else: | ||
| # The variable is not defined on this path | ||
| phi_args[s][p].append((block, "__undefined")) |
There was a problem hiding this comment.
This isn't 100% necessary, as brili supports phi nodes with incomplete sets of labels. If the label isn't present it treats the variable as undefined. However, the bril spec says that "It is an error to use a phi instruction when [..] the instruction does not contain an entry for the second-most-recently-executed label".
Let me know what you would prefer.
There was a problem hiding this comment.
This is perfect! I can definitely see it going either way, but I like that this version of the pass, at least, maintains the invariant that phi-nodes reflect the in-degree of the CFG node they are attached to (as they do in LLVM, for example).
| else: | ||
| # The variable is not defined on this path | ||
| phi_args[s][p].append((block, "__undefined")) |
There was a problem hiding this comment.
This is perfect! I can definitely see it going either way, but I like that this version of the pass, at least, maintains the invariant that phi-nodes reflect the in-degree of the CFG node they are attached to (as they do in LLVM, for example).
Fixes Issue #108.
The optimization in prune_phis() is not sound. phi nodes that are
"partially undefined" cannot be removed. It is only illegal to read
from the result if the second to last executed label corresponds to
an undefined argument.
to_ssa.pygenerated incorrect code for thetwo tests added, before the fix.
In most cases, these phi nodes aren't read from, and will be removed
by DCE. However, in the case where the phi nodes are read, a correct
optimization would be:
Let
undefined_labelsbe the set of labels whose argument isundefined.
undefined_labelsare in fact not predecessors to the basic block.
preds = preds - undefined_labels.Though, I'm not sure how useful that optimization would be in
practice.