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

Flaws and bugs caused by exception handler IR #33

Open
2 of 3 tasks
dubiousconst282 opened this issue Feb 16, 2024 · 0 comments
Open
2 of 3 tasks

Flaws and bugs caused by exception handler IR #33

dubiousconst282 opened this issue Feb 16, 2024 · 0 comments

Comments

@dubiousconst282
Copy link
Owner

dubiousconst282 commented Feb 16, 2024

Exception handlers are currently represented implicitly in the IR via guard and leave instructions, and complemented by ProtectedRegionAnalysis - a standalone analysis that provides a more concrete region tree representation. This makes it relatively complicated and expansive for other analysis and optimizations to correctly reason about exception regions.

One serious flaw that I only recently discovered (after tweaking the inliner/devirt) was register allocation not accounting for live-outs variables in finally handlers due to the lack of explicit successor edges, which lead to reuse of vars that are live after endfinally/resume:

bad_regalloc

(LiveReg_Loop_Finally - improper regalloc for phi @2 and conv @2)

This sample also demonstrates that GVN unsafely propagates the value of store $loc1, r2 into the finally handler (which is fine in the sample above, but it doesn't actually check for in-between exceptions).

At first, these are some things that need to be done:

  • Re-model finally and filter handler exits - make succ edges more explicit somehow (maybe add a dedicated exit block for the entire region?)
    • Modeling exact control-flow for filters might be too complicated, maybe it will be enough to add edges to catch and region exit blocks just so that dominance/liveness are conservative?
    • Since there's no single exit out of a protected region (as leave instructions can point to different targets), the simplest solution I can think of would be to add a backedge from the resume to the guard instruction. But this feels unnatural and I'm unsure if there would be other more profound side effects (it would most certainly interfere with loop analysis, but it seems simple enough to ignore them).
  • Make GVN aware of data-flow going into handler blocks
  • Remove LocalSlot.IsHardExposed and detect cross-region variables in SsaPromotion rather than ILImporter
    • SROA may create new variables without considering EHs, which may get incorrectly promoted.
    • This would also allow promotion of cloned variables after removal of empty finally blocks
  • Consider representing EHRs in a more concrete way (maybe keep parent ProtectedRegion objects in BBs directly?)
    • This could make block maintenance more complicated (for eg. inlining/cloning), but maybe most of it could be handled transparently by fns like CreateBlock()/Split()/Remove().
dubiousconst282 added a commit that referenced this issue May 9, 2024
This fixes the regalloc problem described in #33.

One downside of this approach is that it will require all ResumeInsts
to be in sync with region exit targets, but there are currently no
passes messing with those.
dubiousconst282 added a commit that referenced this issue May 9, 2024
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

No branches or pull requests

1 participant