Skip to content

Short circuit#437

Merged
guipublic merged 8 commits intomasterfrom
gd/short_circuit
Nov 15, 2022
Merged

Short circuit#437
guipublic merged 8 commits intomasterfrom
gd/short_circuit

Conversation

@guipublic
Copy link
Contributor

@guipublic guipublic commented Nov 3, 2022

Description

Summary of changes

This PR fixes an issue related to IF statements where instructions under conditional statement would fail if the condition were true. With the example from the added test case, you can see it can happen that array expressions are out-of-bound in a condition that is not met. In that case, when we know that the expression is failing (at compile time), which can be because of:

  • index out of bound
  • divide by 0
  • false constraint
    We short circuit the block by replacing it with a constraint failing if the block assumption is true.

The PR is now finalised:

  • I could extend the short-circuit when we merge blocks
  • I delete the instructions that were short-circuited in case they are used afterward.
  • The find_join method is fixed, it was working for trees and it is now adapted to the CFG.

Note that divide-by-0 is not triggering a short circuit because it is handled by the constant folding. We should allow it when it is under an assumption and it will be handled as a short-circuit, but this can be done in another PR.

Test additions / changes

Regression test case is added to 9_conditional

@guipublic
Copy link
Contributor Author

I create a draft for now because I want to investigate if block can be further 'short-circuit' during merge block

delete short-circuit instructions in case they are used after
Fix the find_join that worked only for trees
@guipublic guipublic changed the title Gd/short circuit Short circuit Nov 9, 2022
@guipublic guipublic marked this pull request as ready for review November 9, 2022 11:04
@kevaundray
Copy link
Contributor

For future tasks, as noted in your description, please create issues when this gets merged

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

I'd normally like the issue introduced by this PR (Location::dummy) to be fixed before merging it, but it seems this PR has a bugfix that is useful for users to have merged in. I've went ahead and created two issues relating to this PR (#471 and #472) that we can work on later.

@guipublic guipublic merged commit 35497b7 into master Nov 15, 2022
@guipublic guipublic deleted the gd/short_circuit branch November 15, 2022 08:48
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.

4 participants