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

Improve hoisting, CSE, LSRA for MD array accesses #71676

Open
Tracked by #65342
BruceForstall opened this issue Jul 5, 2022 · 1 comment
Open
Tracked by #65342

Improve hoisting, CSE, LSRA for MD array accesses #71676

BruceForstall opened this issue Jul 5, 2022 · 1 comment
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@BruceForstall
Copy link
Member

BruceForstall commented Jul 5, 2022

As described in the comment #70271 (comment) and those that follow, with change #70271 a regression was introduced in the MDMulMatrix benchmark due to limitations or heuristics in the optimization phases downstream of MD array index expansion.

E.g.,

  • LSRA inserts spills and reloads in unfortunate places, including an identified case of un-spilling and re-spilling in the same block, when the value wasn't modified. One issue to investigate this was opened: Investigate the heuristics for creating DummyDef in LSRA #71539
  • there are many cases where a GT_MDARR_LOWER_BOUND or GT_MDARR_LENGTH node is loop-invariant, but can't be hoisted due to exception ordering, as these could cause null reference exception. This is due to hoisting having a very simple notion of side-effects and ordering. In the identified case, we already know that the specific null reference exception can't occur because a previous expression would have caused it, so we should be free to hoist up to that location.
  • CSE seems to create too many long-lifetime CSE, leading LSRA to spill. For the two mentioned nodes, they are very cheap to compute. If they get assigned a register, it's useful to hoist them, but if they don't, they shouldn't be CSE'ed. Maybe CSE's costing should be improved, or we should implement rematerialization for "undo-ing" some CSE.

category:cq
theme:md-arrays
skill-level:expert
cost:large

impact:medium

@BruceForstall BruceForstall added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 5, 2022
@BruceForstall BruceForstall added this to the Future milestone Jul 5, 2022
@ghost
Copy link

ghost commented Jul 5, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

As described in the comment #70271 (comment) and those that follow, with change #70271 a regression was introduced in the MDMulMatrix benchmark due to limitations or heuristics in the optimization phases downstream of MD array index expansion.

E.g.,

  • LSRA inserts spills and reloads in unfortunate places, including an identified case of un-spilling and re-spilling in the same block, when the value wasn't modified. One issue to investigate this was opened: Investigate the heuristics for creating DummyDef in LSRA #71539
  • there are many cases where a GT_MDARR_LOWER_BOUND or GT_MDARR_LENGTH node is loop-invariant, but can't be hoisted due to exception ordering, as these could cause null reference exception. This is due to hoisting having a very simple notion of side-effects and ordering. In the identified case, we already know that the specific null reference exception can't occur because a previous expression would have caused it, so we should be free to hoist up to that location.
  • CSE seems to create too many long-lifetime CSE, leading LSRA to spill. For the two mentioned nodes, they are very cheap to compute. If they get assigned a register, it's useful to hoist them, but if they don't, they shouldn't be CSE'ed. Maybe CSE's costing should be improved, or we should implement rematerialization for "undo-ing" some CSE.

category:cq
theme:md-arrays
skill-level:expert
cost:large

Author: BruceForstall
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: Future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

No branches or pull requests

2 participants