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

Conversation

HodanPlodky
Copy link
Collaborator

@HodanPlodky HodanPlodky commented Jul 25, 2024

What I did

Fix for issues #4162 and #4070

How I did it

Removed the phi nodes which were not in use after SCCP pass. This could happen when the SCCP would replace jnz by jmp instruction and by this removing need for phi instruction (this is the case in #4162). More over the when merging the basic block in the SimplifyCFGPass there could be problem when renaming the labels in phi nodes since it went only into the first cfg_out basic block (this is the case in #4070)

How to verify it

So far both of the POC from compile correctly

Commit message

This commit reduces `phi` nodes which, after SCCP, refer to a
basic block which is unreachable.  This could happen when the
SCCP would replace a `jnz` by a `jmp` instruction, resulting in a
`phi` instruction which refers to a non-existent block, or a `phi`
instruction in a basic block which only has one predecessor.  This
commit reduces such `phi` instructions into `store` instructions.

@charles-cooper
Copy link
Member

shouldn't we fix up the phi nodes during sccp directly?

@HodanPlodky HodanPlodky changed the title Fix/simplify cfg phi rename fix/simplify cfg phi rename Aug 15, 2024
@HodanPlodky HodanPlodky changed the title fix/simplify cfg phi rename fix[venom]: simplify cfg phi rename Aug 15, 2024
@HodanPlodky HodanPlodky marked this pull request as ready for review August 16, 2024 15:18
@@ -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

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 70.45455% with 13 lines in your changes missing coverage. Please review.

Project coverage is 88.22%. Comparing base (03095ce) to head (a867595).

Files with missing lines Patch % Lines
vyper/venom/passes/sccp/sccp.py 70.58% 9 Missing and 1 partial ⚠️
vyper/venom/passes/simplify_cfg.py 70.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4181      +/-   ##
==========================================
- Coverage   91.35%   88.22%   -3.14%     
==========================================
  Files         109      109              
  Lines       15641    15672      +31     
  Branches     3443     3454      +11     
==========================================
- Hits        14289    13826     -463     
- Misses        920     1314     +394     
- Partials      432      532     +100     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 366 to 367
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

@@ -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
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

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):

@@ -329,6 +338,40 @@ def _replace_constants(self, inst: IRInstruction):
if isinstance(lat, IRLiteral):
inst.operands[i] = lat

def _fix_phi_nodes(self):
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()?

assert len(operands) == 1
assert isinstance(operands[0], IRVariable)
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?

self._fix_phi_bb_r(next_bb, visited)

def _fix_phi_node_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?

@charles-cooper
Copy link
Member

i pushed some cleanup here: HodanPlodky#2. it feels like a kludge though, since it seems like we really should be fixing up these phi nodes inside of _handle_SSA_work_item.

charles-cooper and others added 2 commits September 18, 2024 11:48
simplify the cleanup - transforming phis to store instead of removing them
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

pushed some stuff, lgtm now. @harkal mind taking a look?

Copy link
Collaborator

@harkal harkal left a comment

Choose a reason for hiding this comment

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

lgtm

@charles-cooper charles-cooper changed the title fix[venom]: simplify cfg phi rename fix[venom]: fix invalid phis after SCCP Sep 26, 2024
@charles-cooper charles-cooper merged commit e2f6001 into vyperlang:master Sep 27, 2024
155 checks passed
charles-cooper added a commit to charles-cooper/vyper that referenced this pull request Oct 5, 2024
This commit reduces `phi` nodes which, after SCCP, refer to a
basic block which is unreachable.  This could happen when the
SCCP would replace a `jnz` by a `jmp` instruction, resulting in a
`phi` instruction which refers to a non-existent block, or a `phi`
instruction in a basic block which only has one predecessor.  This
commit reduces such `phi` instructions into `store` instructions.

---------

Co-authored-by: Harry Kalogirou <[email protected]>
Co-authored-by: Charles Cooper <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants