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

Change loop cloning to work with pre-header blocks #62665

Closed
BruceForstall opened this issue Dec 11, 2021 · 2 comments
Closed

Change loop cloning to work with pre-header blocks #62665

BruceForstall opened this issue Dec 11, 2021 · 2 comments
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 Dec 11, 2021

After #62560, if a loop pre-header is created before loop cloning (say, immediately after, or during, loop recognition), loop cloning breaks the pre-header invariants by inserting loop choice conditions after the pre-header and before the cloned loops.

Fix this so loop cloning preserves the pre-header, if it exists, so the pre-header invariants (and assertion checking) works.

category:cq
theme:loop-opt
skill-level:expert
cost:medium
impact:small

@BruceForstall BruceForstall added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 11, 2021
@BruceForstall BruceForstall added this to the 7.0.0 milestone Dec 11, 2021
@ghost
Copy link

ghost commented Dec 11, 2021

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

Issue Details

After #62560, if a loop pre-header is created before loop cloning (say, immediately after, or during, loop recognition), loop cloning breaks the pre-header invariants by inserting loop choice conditions after the pre-header and before the cloned loops.

Fix this so loop cloning preserves the pre-header, if it exists, so the pre-header invariants (and assertion checking) works.

Author: BruceForstall
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: 7.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Dec 11, 2021
@BruceForstall BruceForstall removed the untriaged New issue has not been triaged by the area owner label Dec 11, 2021
@BruceForstall BruceForstall modified the milestones: 7.0.0, 8.0.0 Aug 4, 2022
BruceForstall added a commit to BruceForstall/runtime that referenced this issue Apr 6, 2023
As part of finding natural loops and creating the loop
table, create a loop pre-header for every loop. This
simplifies a lot of downstream phases, as the loop
pre-header will be guaranteed to exist, and will already
exist in the dominator tree.

Introduce code to preserve an empty pre-header block through
the optimization phases.

Remove now unnecessary code in hoisting and elsewhere.

Fixes dotnet#77033, dotnet#62665
BruceForstall added a commit that referenced this issue Apr 6, 2023
* Always create loop pre-header

As part of finding natural loops and creating the loop
table, create a loop pre-header for every loop. This
simplifies a lot of downstream phases, as the loop
pre-header will be guaranteed to exist, and will already
exist in the dominator tree.

Introduce code to preserve an empty pre-header block through
the optimization phases.

Remove now unnecessary code in hoisting and elsewhere.

Fixes #77033, #62665

* Fix loop unrolling to work with loop pre-headers

* Add `optLoopsRequirePreHeaders` variable

* Prevent removing pre-header blocks

* Allow removing unreachable pre-headers

Disallow creating pre-header after SSA is built

* Make optLoopCloningEnabled() static

* Teach loop cloning to expect and respect loop pre-headers

* Remove special case pre-header handling in hoisting

* Remove unused SSA update code in fgCreateLoopPreHeader

* Remove unneeded pre-header code from fgDominate

* Remove workaround to avoid extraneous LSRA diffs due to bbNum ordering

* Update comments

* Improve loop table rebuilding with pre-headers

When the loop table is built, it looks around for various loop patterns,
including looking for a guaranteed-executed, pre-loop constant initializer.
This is used in loop cloning and loop unrolling. It needs to look
"a little harder" in the case we created loop pre-headers, then
rebuild the loop table (currently, only due to loop unrolling of loops that
contain nested loops). The new code only allows for empty pre-headers. This
works since in our current phase ordering, no hoisting happens by the time
the loop table is rebuilt.

(Actually, it's currently not necessary to do this at all, since the constant
initializer info is only used by cloning and loop unrolling, both of which
have finished by the time the loop table is rebuilt. However, we might someday
choose to rebuild the loop table after cloning and before unrolling, at which
point it would be necessary.)

* Update comments
@BruceForstall
Copy link
Member Author

Fixed with #83956

@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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

1 participant