search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Failures under Transforms/SLPVectorizer under NPM #46183

Closed Quuxplusone closed 4 years ago

Quuxplusone commented 4 years ago
Bugzilla Link PR47214
Status RESOLVED FIXED
Importance P enhancement
Reported by Arthur Eubanks (aeubanks@google.com)
Reported on 2020-08-17 17:24:43 -0700
Last modified on 2020-08-25 11:19:20 -0700
Version unspecified
Hardware PC Windows NT
CC htmldeveloper@gmail.com, llvm-bugs@lists.llvm.org, spatel+llvm@rotateright.com
Fixed by commit(s)
Attachments
Blocks PR46651
Blocked by
See also

Many tests under Transforms/SLPVectorizer seem to fail when enable-new-pm is on by default. It looks like it's not transforming the input.

Example: llvm/test/Transforms/SLPVectorizer/X86/limit.ll

Quuxplusone commented 4 years ago 
That particular test was calling -instcombine too, but that just seems like an
overstep, so I fixed that here:
https://reviews.llvm.org/rGc98fcba55cf6

Does that make it work with new pass manager?

Which other tests are causing problems?
Quuxplusone commented 4 years ago 
limit.ll still fails under the NPM at head
$ ./build_debug/bin/opt.exe -S -slp-vectorizer -enable-new-pm=0
./llvm/test/Transforms/SLPVectorizer/X86/limit.ll
$ ./build_debug/bin/opt.exe -S -slp-vectorizer -enable-new-pm=1
./llvm/test/Transforms/SLPVectorizer/X86/limit.ll
with -enable-new-pm=1, the function isn't transformed at all.

Full list of failures:
  LLVM :: Transforms/SLPVectorizer/AArch64/64-bit-vector.ll
  LLVM :: Transforms/SLPVectorizer/AArch64/PR38339.ll
  LLVM :: Transforms/SLPVectorizer/AArch64/accelerate-vector-functions.ll
  LLVM :: Transforms/SLPVectorizer/AArch64/remarks.ll
  LLVM :: Transforms/SLPVectorizer/AArch64/spillcost-di.ll
  LLVM :: Transforms/SLPVectorizer/AArch64/spillcost-order.ll
  LLVM :: Transforms/SLPVectorizer/AMDGPU/packed-math.ll
  LLVM :: Transforms/SLPVectorizer/SystemZ/pr34619.ll
  LLVM :: Transforms/SLPVectorizer/WebAssembly/no-vectorize-rotate.ll
  LLVM :: Transforms/SLPVectorizer/X86/PR32086.ll
  LLVM :: Transforms/SLPVectorizer/X86/broadcast.ll
  LLVM :: Transforms/SLPVectorizer/X86/commutativity.ll
  LLVM :: Transforms/SLPVectorizer/X86/different-vec-widths.ll
  LLVM :: Transforms/SLPVectorizer/X86/funclet.ll
  LLVM :: Transforms/SLPVectorizer/X86/insert-after-bundle.ll
  LLVM :: Transforms/SLPVectorizer/X86/jumbled-load-multiuse.ll
  LLVM :: Transforms/SLPVectorizer/X86/jumbled-load-shuffle-placement.ll
  LLVM :: Transforms/SLPVectorizer/X86/jumbled-load-used-in-phi.ll
  LLVM :: Transforms/SLPVectorizer/X86/jumbled-load.ll
  LLVM :: Transforms/SLPVectorizer/X86/jumbled_store_crash.ll
  LLVM :: Transforms/SLPVectorizer/X86/limit.ll
  LLVM :: Transforms/SLPVectorizer/X86/lookahead.ll
  LLVM :: Transforms/SLPVectorizer/X86/opt.ll
  LLVM :: Transforms/SLPVectorizer/X86/pr27163.ll
  LLVM :: Transforms/SLPVectorizer/X86/pr35497.ll
  LLVM :: Transforms/SLPVectorizer/X86/remark_horcost.ll
  LLVM :: Transforms/SLPVectorizer/X86/resched.ll
  LLVM :: Transforms/SLPVectorizer/X86/reuse-extracts-in-wider-vect.ll
  LLVM :: Transforms/SLPVectorizer/X86/schedule-bundle.ll
  LLVM :: Transforms/SLPVectorizer/X86/store-jumbled.ll
  LLVM :: Transforms/SLPVectorizer/X86/stores_vectorize.ll
  LLVM :: Transforms/SLPVectorizer/X86/supernode.ll
  LLVM :: Transforms/SLPVectorizer/X86/vect_copyable_in_binops.ll
  LLVM :: Transforms/SLPVectorizer/int_sideeffect.ll
Quuxplusone commented 4 years ago 
Ah, this reminds me of a problem that I had with NPM while writing a
PhaseOrdering test:
https://reviews.llvm.org/rGd43fac052e164

NPM does not automatically invoke alias analysis, so you have to make that
explicit. Otherwise, SLP may just bail out at some point.

We may have to modify the RUN lines to something like this?
RUN: opt < %s -basic-aa -slp-vectorizer -enable-new-pm=1 -S | FileCheck %s
Quuxplusone commented 4 years ago 
Aha adding -basic-aa fixes the vast majority of tests, thanks!
There's no need to pin the test to the NPM since -basic-aa is a no-op under the
legacy PM.
Quuxplusone commented 4 years ago 
Should be mostly fixed with https://reviews.llvm.org/D86167.
Just one left, Transforms/SLPVectorizer/AArch64/accelerate-vector-functions.ll
Quuxplusone commented 4 years ago 
(In reply to Arthur Eubanks from comment #5)
> Should be mostly fixed with https://reviews.llvm.org/D86167.
> Just one left,
> Transforms/SLPVectorizer/AArch64/accelerate-vector-functions.ll

That one needs to be made aware of the extra lib functions. I don't know what
the preferred spelling for this is, but this RUN line gets it to pass for me:

; RUN: opt -passes='inject-tli-mappings,slp-vectorizer' -vector-
library=Accelerate -S %s | FileCheck %s
Quuxplusone commented 4 years ago 
Oh interesting...

INITIALIZE_PASS_BEGIN(SLPVectorizer, SV_NAME, lv_name, false, false)
INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass)
INITIALIZE_PASS_DEPENDENCY(TargetTransformInfoWrapperPass)
INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
INITIALIZE_PASS_DEPENDENCY(ScalarEvolutionWrapperPass)
INITIALIZE_PASS_DEPENDENCY(LoopSimplify)
INITIALIZE_PASS_DEPENDENCY(DemandedBitsWrapperPass)
INITIALIZE_PASS_DEPENDENCY(OptimizationRemarkEmitterWrapperPass)
INITIALIZE_PASS_DEPENDENCY(InjectTLIMappingsLegacy)
INITIALIZE_PASS_END(SLPVectorizer, SV_NAME, lv_name, false, false)

shows that under the legacy PM SLPVectorizer has a dependency on
InjectTLIMappingsLegacy, but that doesn't happen under the new PM (I don't
think you can declare a pass as dependent on another non-analysis pass).

Thanks!
Quuxplusone commented 4 years ago

Last one fixed with https://reviews.llvm.org/D86492. Thanks spatel!