[PATCH] D99149: [LoopUnrollAndJam] Change LoopUnrollAndJamPass to LoopNest pass

Mon Jun 7 02:19:19 PDT 2021

hvdijk added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:511
 INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(LoopSimplify)
+INITIALIZE_PASS_DEPENDENCY(LCSSAWrapperPass)
----------------
uint256_t wrote:
> hvdijk wrote:
> > uint256_t wrote:
> > > hvdijk wrote:
> > > > uint256_t wrote:
> > > > > hvdijk wrote:
> > > > > > uint256_t wrote:
> > > > > > > @hvdijk Could you try reversing the running order of `LoopSimplify` and `LCSSAWrapperPass`, and check if the problem still reproduces?
> > > > > > Tried now, and I see the same results if I reverse those two lines.
> > > > > Thank you for reporting! I didn't care about the legacy pass manager. Maybe I'll make a new patch to fix the problem, so I think this need not be reverted. 
> > > > > Do you mind if unroll-and-jam fails on legacy pass manager now?
> > > > It's not my call to make, but according to the description of D95380, users are supposed to be able to switch to the legacy pass manager if they encounter problems with the new pass manager. It may be acceptable for the two pass managers to produce slightly different results, but if we now get crashes with the legacy pass manager, users may not have the option to switch if needed. Unless the legacy pass manager is removed entirely (which should at least have some discussion on the mailing list), I think at least the crash will need fixing.
> > > I reverted the commit to fix the problem. thanks.
> > Thanks. Happy to re-test if you later have a new version, if you want.
> In my environment there's no more crash on legacy pass manager. Could you please re-test yourself?
Nice, I see no crash, and the tests I mentioned all pass even with the old pass manager. One other test fails (Transforms/SimpleLoopUnswitch/implicit-null-checks.ll) but it's not a crash, just different test output. I'll check deeper later to make sure there's no real issue, but I think it's probably fine.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99149/new/

https://reviews.llvm.org/D99149