[PATCH] D112851: [PassManager] `buildModuleOptimizationPipeline()`: schedule `LoopDeletion` pass run before vectorization passes

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

(Copied from the other change)
I looked into `@is_not_empty_variant1()`. It seems like GVN + an instcombine cleanup is what's allowing the loop to be deleted. Currently GVN runs after the loop passes which is the issue.
I tried replacing the EarlyCSEPass at the beginning of the function simplification pipeline with a NewGVNPass and that fixes `@is_not_empty_variant1`. I think something along those lines is more principled than adding an LoopDeletionPass somewhere.
(Of course, if we want to turn on NewGVN specifically we need to iron out remaining issues)

In D112851#3099665 <https://reviews.llvm.org/D112851#3099665>, @lebedev.ri wrote:

> In D112851#3099588 <https://reviews.llvm.org/D112851#3099588>, @mkazantsev wrote:
>
>> Deletion of loops before they are vectorized absolutely makes sense to me. Please give it couple more days just in case if someone has concerns regarding the regressions, but making vectorizer run on empty doesn't sound. Please give it 2-3 days to hear others' concerns.
>
> Indeed, even if/when what @aeubanks suggested in https://reviews.llvm.org/D112840#3097891 is implemented,
> somehow i don't think it will alleviate the need for *this*-late LoopDeletion.

Can you explain why *this* late LoopDeletion would still be necessary even with the GVN phase ordering changes?
It makes sense to me that specifically GVN can optimize enough to make some loops deletable. Ideally we'd already delete all deletable loops in the function simplification pipeline rather than late in the pipeline. Anything to do with deleting unused code should trigger in the simplification pipeline, not the late optimization pipeline, since deleting unnecessary instructions is a canonicalization.
And I'm sure there are many other loop passes that could benefit from having GVN run first.

Take all of this with a grain of salt because running an extra GVN pass is very expensive. We run EarlyCSE at the beginning of the function simplification pipeline as a mini-GVN, but replacing that with NewGVN, which is supposed to be significantly faster than GVN, still results in major compile time regressions. https://llvm-compile-time-tracker.com/compare.php?from=1c05c52de2177a328b7d2d07b697af67eb9f8122&to=755e184ed0216ef39d68621a9b19a3dd34d677f2&stat=instructions

So overall I think this is a hack to fix a fairly specific use case, but a proper fix is nowhere in the near future. If you really think this snippet of code is important to optimize then I'm not opposed to this change until we have a more proper fix in the future.
We can always tack on passes to the end of the pipeline to handle missed optimizations due to phase ordering, it doesn't mean it's principled. (of course if you can explain why an extra loop deletion pass on top of the one in the simplification pipeline is principled I'm all ears)

> Thank you for the review, i'll wait a bit on @aeubanks / @asbirlea / @dmgreen,
> though given just how little of a compile-time regression this has become,
> i can't imagine this will be blocked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112851