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 7 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
45 changes: 44 additions & 1 deletion vyper/venom/passes/sccp/sccp.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,14 @@ class SCCP(IRPass):
work_list: list[WorkListItem]
cfg_dirty: bool
cfg_in_exec: dict[IRBasicBlock, OrderedSet[IRBasicBlock]]
cfg_phi_fix_needed: OrderedSet[IRBasicBlock]

def __init__(self, analyses_cache: IRAnalysesCache, function: IRFunction):
super().__init__(analyses_cache, function)
self.lattice = {}
self.work_list: list[WorkListItem] = []
self.cfg_dirty = False
self.cfg_phi_fix_needed = OrderedSet()

def run_pass(self):
self.fn = self.function
Expand All @@ -74,7 +76,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 @@ -305,6 +311,9 @@ def _replace_constants(self, inst: IRInstruction):
inst.opcode = "jmp"
inst.operands = [target]
self.cfg_dirty = True
for bb in inst.parent.cfg_out:
if bb.label == target:
self.cfg_phi_fix_needed.add(bb)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not self._fix_phi_bb_r(bb, OrderedSet()) here and remove all the need for self.cfg_phi_fix_needed and iterating later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason was not to iterate over some basic block multiple times. Since solutions only detects start from which the occurrence of faulty phi node could be possible but not the end there could be a lot of overlap. In this case since I iterate over possible positions of the faulty phi nodes at the end I visit every possible basic block only once.

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
self.cfg_phi_fix_needed.add(bb)
self.cfg_phi_fix_needed.add(bb)
break


elif inst.opcode in ("assert", "assert_unreachable"):
lat = self._eval_from_lattice(inst.operands[0])
Expand All @@ -329,6 +338,40 @@ 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
visited: OrderedSet[IRBasicBlock] = OrderedSet()
Copy link
Member

Choose a reason for hiding this comment

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

why keep track of visited, instead of just iterating over fn.get_basic_blocks()?

for bb in self.cfg_phi_fix_needed:
self._fix_phi_bb_r(bb, visited)

def _fix_phi_bb_r(self, bb: IRBasicBlock, visited: OrderedSet[IRBasicBlock]):
Fixed Show fixed Hide fixed
if bb in visited:
return

for inst in bb.instructions:
if inst.opcode == "phi":
self._fix_phi_node_inst(inst)

visited.add(bb)

for next_bb in bb.cfg_out:
self._fix_phi_bb_r(next_bb, visited)

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