-
Notifications
You must be signed in to change notification settings - Fork 230
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
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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.
Awesome; thank you so much for sorting this out!! Really nice work.
else: | ||
# The variable is not defined on this path | ||
phi_args[s][p].append((block, "__undefined")) |
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.
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.py
generated 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_labels
be the set of labels whose argument isundefined.
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.