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

SimpleLoopUnswitch not informing pass manager when child loops are deleted during nontrivial unswitch #51096

Closed
bjope opened this issue Sep 5, 2021 · 4 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@bjope
Copy link
Collaborator

bjope commented Sep 5, 2021

Bugzilla Link 51754
Resolution FIXED
Resolved on Sep 09, 2021 09:49
Version trunk
OS Windows NT
Blocks #50580
CC @tstellar
Fixed by commit(s) 0f0344d f17d60d

Extended Description

Problems were observed downstream when running tests using "opt -enable-new-pm=1 -O3 -S -extra-vectorizer-passes=1 -enable-loop-distribute". For a specific input the compiler sometimes just crashed, sometimes we hit an assertion, etc. Although this was very non-deterministic and it only failed occasionally.

A somewhat reduced test case included 2 functions "foo" and "bar". And some debugging indicated the following scenario:

  1. When running loop-distribute on "foo" LoopAccessAnalysis was calculated for some loops in that functions. Those analyses were cached in the LoopAnalysisManager associated with a LoopAnalysisManagerFunctionProxy based on a Loop*.

  2. Later when running SimpleLoopUnswitch a loop "a" was transformed, and as part of that a child loop "b" was removed/replaced. The Loop object for "b" was destroyed (still being allocated by the BumpPtrAllocator, and without removing the analyses cached on the corresponding Loop*).

  3. Here something triggeres a reset of the BumpPtrAllocator used for allocating Loop objects (maybe throwing away loop analyses at the end of the loop pass manager).

  4. When running loop-distribution later, for "bar", first a Loop object was created for a loop "c". And then LoopAccessAnalysis were requested. Depending on if the Loop object for "c" happened to use the same address as the stale analysis for loop "b" we sometimes ended up fetching the LoopAccessAnalysis result for "b" instead of recalculating the analysis for "c".

@bjope
Copy link
Collaborator Author

bjope commented Sep 5, 2021

After asking for help on llvm-dev it was suggested that perhaps some calls to markLoopAsDeleted were missing when the Loop objects were destroyed: https://lists.llvm.org/pipermail/llvm-dev/2021-September/152468.html

A patch that solves the described problem that way was submitted here:
https://reviews.llvm.org/D109257

@tstellar
Copy link
Collaborator

tstellar commented Sep 8, 2021

The fix does not apply cleanly, could someone backport this and push a branch to their local github fork?'

@bjope
Copy link
Collaborator Author

bjope commented Sep 8, 2021

Patch adjusted for release/13.x branch.
I do not have a private github clone, but I added a patch as attachment.

Hopefully you can apply it like this:

cd llvm
cat patch.for.13.x | git am -3 -k

@tstellar
Copy link
Collaborator

tstellar commented Sep 9, 2021

Merged: f17d60d

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

2 participants