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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D112851#3101031 <https://reviews.llvm.org/D112851#3101031>, @aeubanks wrote:

> (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#3101031 <https://reviews.llvm.org/D112851#3101031>, @aeubanks wrote:

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

Well, i'm not actually saying that, i'm only speculating as much.
Let's suppose GVN phase ordering change happens, and D112840 <https://reviews.llvm.org/D112840> would become unnecessary,
But, i posted this change (replacing D112840 <https://reviews.llvm.org/D112840>) because while D112840 <https://reviews.llvm.org/D112840> (on vanilla test-suite) caused

  | statistic name                                   |  baseline |  proposed |      Δ |      % |    |%| |
  |--------------------------------------------------|----------:|----------:|-------:|-------:|-------:|
  | loop-delete.NumBackedgesBroken                   |       542 |       557 |     15 |  2.77% |  2.77% |
  | loop-delete.NumDeleted                           |      8055 |      8096 |     41 |  0.51% |  0.51% |

while this variant causes

  | statistic name                                   |  baseline |  proposed |     Δ |      % |    |%| |
  |--------------------------------------------------|----------:|----------:|------:|-------:|-------:|
  | loop-delete.NumBackedgesBroken                   |       542 |       559 |    17 |  3.14% |  3.14% |
  | loop-delete.NumDeleted                           |      8055 |      8128 |    73 |  0.91% |  0.91% |

Aka, as compared to baseline, this deletes ~80% more loops than D112840 <https://reviews.llvm.org/D112840> would.
Notably, this new LoopDelete is placed after LoopRotatePass, because i have checked, and that
also causes an increase in num loops deleted, as compared with placing it before LoopRotatePass.
So right now, i would not expect that GVN phase ordering improvement will make this late LoopDeletion unnecessary,
but if i'm proven wrong and it does, then great and this could be reverted.

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

Sure, i'm not arguing that we *should't* succeed with this in simplification pipeline,
i'm only saying that at least currently, even more loops become dead *after* the simplification pipeline, in optimization pipeline.

> 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

Right.

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

Yay, i love ugly hacks.

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