Skip to content
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

fix[venom]: fix invalid phis after SCCP #4181

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion vyper/venom/passes/sccp/sccp.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ def run_pass(self):

# self._propagate_variables()

self.analyses_cache.invalidate_analysis(CFGAnalysis)
if self.cfg_dirty:
self.analyses_cache.force_analysis(CFGAnalysis)
self._fix_phi_nodes()
else:
self.analyses_cache.invalidate_analysis(CFGAnalysis)

def _calculate_sccp(self, entry: IRBasicBlock):
"""
Expand Down Expand Up @@ -329,6 +333,28 @@ def _replace_constants(self, inst: IRInstruction):
if isinstance(lat, IRLiteral):
inst.operands[i] = lat

def _fix_phi_nodes(self):
charles-cooper marked this conversation as resolved.
Show resolved Hide resolved
for bb in self.function.get_basic_blocks():
for inst in bb.instructions.copy():
if inst.opcode == "phi":
self._fix_phi_node_inst(inst)

def _fix_phi_node_inst(self, phi_inst: IRInstruction):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _fix_phi_node_inst(self, phi_inst: IRInstruction):
def _fix_phi_inst(self, phi_inst: IRInstruction):

if len(phi_inst.parent.cfg_in) != 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when can this happen? shouldn't we handle this case?

return

src_bb: IRBasicBlock = phi_inst.parent.cfg_in.first()
assert isinstance(src_bb, IRBasicBlock)

from_src_bb = filter(lambda x: x[0] == src_bb.label, phi_inst.phi_operands)
operands = list(map(lambda x: x[1], from_src_bb))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use a list comprehension here

Suggested change
from_src_bb = filter(lambda x: x[0] == src_bb.label, phi_inst.phi_operands)
operands = list(map(lambda x: x[1], from_src_bb))
operands = [op for label, op in phi_inst.phi_operands if label == src_bb.label]


assert len(operands) == 1
assert isinstance(operands[0], IRVariable)
assert phi_inst.output is not None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert phi_inst.output is not None
assert phi_inst.output is not None

phi_inst.output.value = operands[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why modify the instruction if it gets removed on the next line?

phi_inst.parent.remove_instruction(phi_inst)
charles-cooper marked this conversation as resolved.
Show resolved Hide resolved


def _meet(x: LatticeItem, y: LatticeItem) -> LatticeItem:
if x == LatticeEnum.TOP:
Expand Down
25 changes: 11 additions & 14 deletions vyper/venom/passes/simplify_cfg.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,21 @@ class SimplifyCFGPass(IRPass):
def _merge_blocks(self, a: IRBasicBlock, b: IRBasicBlock):
a.instructions.pop()
for inst in b.instructions:
assert inst.opcode != "phi", "Not implemented yet"
if inst.opcode == "phi":
a.instructions.insert(0, inst)
else:
inst.parent = a
a.instructions.append(inst)
assert inst.opcode != "phi", f"Instruction should never be phi {inst}"
inst.parent = a
a.instructions.append(inst)

# Update CFG
a.cfg_out = b.cfg_out
if len(b.cfg_out) > 0:
next_bb = b.cfg_out.first()
next_bb.remove_cfg_in(b)
next_bb.add_cfg_in(a)

for inst in next_bb.instructions:
if inst.opcode != "phi":
break
inst.operands[inst.operands.index(b.label)] = a.label
for next_bb in b.cfg_out:
next_bb.remove_cfg_in(b)
next_bb.add_cfg_in(a)

for inst in next_bb.instructions:
if inst.opcode != "phi":
break
inst.operands[inst.operands.index(b.label)] = a.label

self.function.remove_basic_block(b)

Expand Down
Loading