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

Loop canonicalization should always create loop pre-headers #77033

Closed
BruceForstall opened this issue Oct 13, 2022 · 4 comments · Fixed by #83956
Closed

Loop canonicalization should always create loop pre-headers #77033

BruceForstall opened this issue Oct 13, 2022 · 4 comments · Fixed by #83956
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 Oct 13, 2022

It would simplify a lot of code if loop canonicalization always creates pre-header blocks, and they are maintained late into compilation. This would require both allowing them to exist, but also to be maintained, e.g., #62665.

Another motivating issue: #68844 (comment)

@BruceForstall BruceForstall added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 13, 2022
@BruceForstall BruceForstall added this to the 8.0.0 milestone Oct 13, 2022
@BruceForstall BruceForstall self-assigned this Oct 13, 2022
@ghost
Copy link

ghost commented Oct 13, 2022

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

Issue Details

It would simplify a lot of code if loop canonicalization always creates pre-header blocks, and they are maintained late into compilation. This would require both allowing them to exist, but also to be maintained, e.g., #62665.

Author: BruceForstall
Assignees: BruceForstall
Labels:

area-CodeGen-coreclr

Milestone: 8.0.0

@BruceForstall
Copy link
Member Author

optHoistThisLoop has a bunch of special processing that could be eliminated if pre-headers always existed (and thus dominators included them).

@BruceForstall
Copy link
Member Author

BruceForstall commented Mar 11, 2023

Should be able to remove pre-header special handling in optAddCopies, which assumes pre-headers are not in the dominator tree.

This phase was recently removed with #83310

@BruceForstall
Copy link
Member Author

fgDominate also contains special handling for pre-headers that could be removed if pre-headers always exist before dominators are built.

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
@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

Successfully merging a pull request may close this issue.

1 participant