[PATCH] D61726: [Pass Pipeline] Run another round of reassociation after loop pipeline

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai marked 2 inline comments as done.
nemanjai added a comment.

In D61726#1497009 <https://reviews.llvm.org/D61726#1497009>, @efriedma wrote:

> Running reassociate after unroll probably makes sense.  But I'd like to see compile-time numbers.
>
> How carefully have you considered the exact placement?  It looks like you're using different placement for the legacy vs. new pass manager.  Do we want to run before the late LICM pass?


I will collect some compile time numbers with `test-suite`.
Regarding placement: I didn't really consider it very careful as I don't really know what the tradeoffs would be for various positions. The one thing that seems clear is that it needs to run after unrolling. However, where in the pipeline after unrolling... I am most certainly open to suggestions and can experiment with a few suggested options.

Thanks again for your feedback.



================
Comment at: test/Transforms/LoopVectorize/X86/masked_load_store.ll:2-4
 ; RUN: opt < %s  -O3 -mcpu=corei7-avx -S | FileCheck %s -check-prefix=AVX -check-prefix=AVX1
 ; RUN: opt < %s  -O3 -mcpu=core-avx2 -S | FileCheck %s -check-prefix=AVX -check-prefix=AVX2
 ; RUN: opt < %s  -O3 -mcpu=knl -S | FileCheck %s -check-prefix=AVX512
----------------
spatel wrote:
> Regardless of anything else, this test file was over-reaching, so I fixed that problem:
> rL360340
> 
> If you update/rebase, this should not wiggle with this patch now.
Will do, thank you.


================
Comment at: test/Transforms/Reassociate/reassociate-after-unroll.ll:1
+; RUN: opt -O2 -S < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-n32:64"
----------------
spatel wrote:
> This test file belongs in test/Transforms/PhaseOrdering. I prefer to have the baseline test with complete, auto-generated checks (utils/update_test_checks.py) committed as a preliminary step, so we can see the before/after diff in this review.
> 
> If you're updating the new pass manager in this patch, this test should have another RUN line to exercise/verify that path.
I will move it and add a RUN line for the NPM. Thanks for the suggestions.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61726