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

effects: relax recursion detection for effects analysis #45993

Merged
merged 2 commits into from
Jul 12, 2022

Conversation

aviatesk
Copy link
Member

In a similar spirit to #40561, we can relax the recursion detection to
guarantee :terminates effect and allow the effects analysis to not
taint :terminates effect when there are no cycles in MethodInstances
in a call graph.

fix #45781

@aviatesk aviatesk requested review from vtjnash and Keno July 11, 2022 11:41
@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

effects = Effects(effects; terminates=ALWAYS_FALSE)
# Some sort of recursion was detected.
if edge !== nothing && !edgelimited && !is_edge_recursed(edge, sv)
# no `MethodInstance` cycles -- don't taint :terminate
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it may only consider whether there was a cycle that will impact the computation of the result type. If we note that the result is discarded, there won't be an edge or edgecycle here at all (note line 512 above).

IIUC, the correct fix for that may be to poison the effects at line 512 however?

Copy link
Member Author

@aviatesk aviatesk Jul 12, 2022

Choose a reason for hiding this comment

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

If we discard the result, we propagate the unknown effects Effects(), whose :terminate effect is also tainted, and it will eventually poison the rest of the call graph.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

sgtm

Base automatically changed from avi/effects to master July 12, 2022 03:54
In a similar spirit to #40561, we can relax the recursion detection to
guarantee `:terminates` effect and allow the effects analysis to not
taint `:terminates` effect when there are no cycles in `MethodInstance`s
in a call graph.

fix #45781
@aviatesk aviatesk merged commit 166f64e into master Jul 12, 2022
@aviatesk aviatesk deleted the avi/recursion-effects branch July 12, 2022 09:09
aviatesk added a commit that referenced this pull request Jul 13, 2022
* ~~Add (the equivalent of) `@assume_effects :terminates_globally` to
  `unwrap_composed`. Although it could be inferred as `Const`, without
  the annotation, it was not elided for too complex inputs, resulting in
  unnecessary runtime overhead.~~
  EDIT: now #45993 is merged and this part isn't included.
* Reverse recursion order in `call_composed`. This prevents potentially
  changing argument types being piped through the recursion, making
  inference bail out. With the reversed order, only the tuple of
  remaining functions is changing during recursion and is becoming
  strictly simpler, letting inference succeed.

Co-authored-by: Shuhei Kadowaki <aviatesk@gmail.com>
aviatesk added a commit that referenced this pull request Jul 20, 2022
effects: relax recursion detection for effects analysis
@aviatesk aviatesk mentioned this pull request Jul 20, 2022
32 tasks
aviatesk added a commit that referenced this pull request Jul 21, 2022
#45993 fixed this bug.
@aviatesk aviatesk mentioned this pull request Jul 21, 2022
aviatesk added a commit that referenced this pull request Jul 22, 2022
#45993 fixed this bug.
aviatesk added a commit that referenced this pull request Jul 23, 2022
#45993 fixed this bug, let's make
the math effect-tests actually fail
aviatesk added a commit that referenced this pull request Jul 26, 2022
effects: relax recursion detection for effects analysis
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
* ~~Add (the equivalent of) `@assume_effects :terminates_globally` to
  `unwrap_composed`. Although it could be inferred as `Const`, without
  the annotation, it was not elided for too complex inputs, resulting in
  unnecessary runtime overhead.~~
  EDIT: now JuliaLang#45993 is merged and this part isn't included.
* Reverse recursion order in `call_composed`. This prevents potentially
  changing argument types being piped through the recursion, making
  inference bail out. With the reversed order, only the tuple of
  remaining functions is changing during recursion and is becoming
  strictly simpler, letting inference succeed.

Co-authored-by: Shuhei Kadowaki <aviatesk@gmail.com>
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
JuliaLang#45993 fixed this bug, let's make
the math effect-tests actually fail
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
* ~~Add (the equivalent of) `@assume_effects :terminates_globally` to
  `unwrap_composed`. Although it could be inferred as `Const`, without
  the annotation, it was not elided for too complex inputs, resulting in
  unnecessary runtime overhead.~~
  EDIT: now JuliaLang#45993 is merged and this part isn't included.
* Reverse recursion order in `call_composed`. This prevents potentially
  changing argument types being piped through the recursion, making
  inference bail out. With the reversed order, only the tuple of
  remaining functions is changing during recursion and is becoming
  strictly simpler, letting inference succeed.

Co-authored-by: Shuhei Kadowaki <aviatesk@gmail.com>
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
JuliaLang#45993 fixed this bug, let's make
the math effect-tests actually fail
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.

Intermittent test failure in math-effect test
3 participants