[PATCH] D100865: [COST] Improve shuffle kind detection if shuffle mask is provided.

Sun Apr 25 13:58:13 PDT 2021

ABataev added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:753
+    case TTI::SK_PermuteTwoSrc:
+      if (ShuffleVectorInst::isSelectMask(Mask))
+        return TTI::SK_Select;
----------------
dmgreen wrote:
> ABataev wrote:
> > dmgreen wrote:
> > > ABataev wrote:
> > > > dmgreen wrote:
> > > > > Some of these Mask functions do not handle all sizes of mask. Do we need to be careful about shuffles that changesLength?
> > > > Here we don't have info about length changes, looks like all these functions use the size of the Mask array as vector factor.
> > > I may have the wrong impression of what is going on exactly, but I think it's about the elements being out of range for the mask size.
> > > (Hopefully) Try this example:
> > > https://godbolt.org/z/WWrPbPxcW
> > This function does not use 2 different length, it has just one, deduced out of Mask size. It is just impossible to have a `change of size` situation here, I assume. We do not work with actual shuffles, functions are static.
> > Also, could you provide more details about your example? What shall I expect as a result!
> > Also, could you provide more details about your example? What shall I expect as a result!
> 
> It should not crash with this patch :-)
> 
> It says for me:
> ```
> SLP: Adding cost 0 for bundle that starts with   %add.i33 = fadd fast half %21, %22.
> SLP: Current total cost = 0
> SLP: Adding cost 6 for bundle that starts with   %21 = extractelement <8 x half> %20, i32 0.
> SLP: Current total cost = 6
> clang: /work/llvm-project/llvm/lib/IR/Instructions.cpp:2122: bool isSingleSourceMaskImpl(llvm::ArrayRef<int>, int): Assertion `I
> >= 0 && I < (NumOpElts * 2) && "Out-of-bounds shuffle mask element"' failed.
> ...
> 0.      Program arguments: bin/clang -target arm-arm-none-eabi -mcpu=cortex-m55 -Ofast tmp.c -S -o - -mllvm -debug -mllvm -print-
> after-all
> ...
>  #9 0x0000558afa5bc6de llvm::ShuffleVectorInst::isSingleSourceMask(llvm::ArrayRef<int>) (bin/clang+0x728e6de)
> #10 0x0000558afa5bc911 llvm::ShuffleVectorInst::isSelectMask(llvm::ArrayRef<int>) (bin/clang+0x728e911)
> #11 0x0000558af90f9c40 llvm::ARMTTIImpl::getShuffleCost(llvm::TargetTransformInfo::ShuffleKind, llvm::VectorType*, llvm::ArrayRef
> <int>, int, llvm::VectorType*) ARMTargetTransformInfo.cpp:0:0
> #12 0x0000558af9fcac6a llvm::TargetTransformInfo::getShuffleCost(llvm::TargetTransformInfo::ShuffleKind, llvm::VectorType*, llvm:
> :ArrayRef<int>, int, llvm::VectorType*) const (bin/clang+0x6c9cc6a)
> #13 0x0000558afb0d4f50 llvm::slpvectorizer::BoUpSLP::getEntryCost(llvm::slpvectorizer::BoUpSLP::TreeEntry*) (bin/clang+0x7da6f50)
> ```
> > Also, could you provide more details about your example? What shall I expect as a result!
> 
> It should not crash with this patch :-)
> 
> It says for me:
> ```
> SLP: Adding cost 0 for bundle that starts with   %add.i33 = fadd fast half %21, %22.
> SLP: Current total cost = 0
> SLP: Adding cost 6 for bundle that starts with   %21 = extractelement <8 x half> %20, i32 0.
> SLP: Current total cost = 6
> clang: /work/llvm-project/llvm/lib/IR/Instructions.cpp:2122: bool isSingleSourceMaskImpl(llvm::ArrayRef<int>, int): Assertion `I
> >= 0 && I < (NumOpElts * 2) && "Out-of-bounds shuffle mask element"' failed.
> ...
> 0.      Program arguments: bin/clang -target arm-arm-none-eabi -mcpu=cortex-m55 -Ofast tmp.c -S -o - -mllvm -debug -mllvm -print-
> after-all
> ...
>  #9 0x0000558afa5bc6de llvm::ShuffleVectorInst::isSingleSourceMask(llvm::ArrayRef<int>) (bin/clang+0x728e6de)
> #10 0x0000558afa5bc911 llvm::ShuffleVectorInst::isSelectMask(llvm::ArrayRef<int>) (bin/clang+0x728e911)
> #11 0x0000558af90f9c40 llvm::ARMTTIImpl::getShuffleCost(llvm::TargetTransformInfo::ShuffleKind, llvm::VectorType*, llvm::ArrayRef
> <int>, int, llvm::VectorType*) ARMTargetTransformInfo.cpp:0:0
> #12 0x0000558af9fcac6a llvm::TargetTransformInfo::getShuffleCost(llvm::TargetTransformInfo::ShuffleKind, llvm::VectorType*, llvm:
> :ArrayRef<int>, int, llvm::VectorType*) const (bin/clang+0x6c9cc6a)
> #13 0x0000558afb0d4f50 llvm::slpvectorizer::BoUpSLP::getEntryCost(llvm::slpvectorizer::BoUpSLP::TreeEntry*) (bin/clang+0x7da6f50)
> ```

================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:753
+    case TTI::SK_PermuteTwoSrc:
+      if (ShuffleVectorInst::isSelectMask(Mask))
+        return TTI::SK_Select;
----------------
ABataev wrote:
> dmgreen wrote:
> > ABataev wrote:
> > > dmgreen wrote:
> > > > ABataev wrote:
> > > > > dmgreen wrote:
> > > > > > Some of these Mask functions do not handle all sizes of mask. Do we need to be careful about shuffles that changesLength?
> > > > > Here we don't have info about length changes, looks like all these functions use the size of the Mask array as vector factor.
> > > > I may have the wrong impression of what is going on exactly, but I think it's about the elements being out of range for the mask size.
> > > > (Hopefully) Try this example:
> > > > https://godbolt.org/z/WWrPbPxcW
> > > This function does not use 2 different length, it has just one, deduced out of Mask size. It is just impossible to have a `change of size` situation here, I assume. We do not work with actual shuffles, functions are static.
> > > Also, could you provide more details about your example? What shall I expect as a result!
> > > Also, could you provide more details about your example? What shall I expect as a result!
> > 
> > It should not crash with this patch :-)
> > 
> > It says for me:
> > ```
> > SLP: Adding cost 0 for bundle that starts with   %add.i33 = fadd fast half %21, %22.
> > SLP: Current total cost = 0
> > SLP: Adding cost 6 for bundle that starts with   %21 = extractelement <8 x half> %20, i32 0.
> > SLP: Current total cost = 6
> > clang: /work/llvm-project/llvm/lib/IR/Instructions.cpp:2122: bool isSingleSourceMaskImpl(llvm::ArrayRef<int>, int): Assertion `I
> > >= 0 && I < (NumOpElts * 2) && "Out-of-bounds shuffle mask element"' failed.
> > ...
> > 0.      Program arguments: bin/clang -target arm-arm-none-eabi -mcpu=cortex-m55 -Ofast tmp.c -S -o - -mllvm -debug -mllvm -print-
> > after-all
> > ...
> >  #9 0x0000558afa5bc6de llvm::ShuffleVectorInst::isSingleSourceMask(llvm::ArrayRef<int>) (bin/clang+0x728e6de)
> > #10 0x0000558afa5bc911 llvm::ShuffleVectorInst::isSelectMask(llvm::ArrayRef<int>) (bin/clang+0x728e911)
> > #11 0x0000558af90f9c40 llvm::ARMTTIImpl::getShuffleCost(llvm::TargetTransformInfo::ShuffleKind, llvm::VectorType*, llvm::ArrayRef
> > <int>, int, llvm::VectorType*) ARMTargetTransformInfo.cpp:0:0
> > #12 0x0000558af9fcac6a llvm::TargetTransformInfo::getShuffleCost(llvm::TargetTransformInfo::ShuffleKind, llvm::VectorType*, llvm:
> > :ArrayRef<int>, int, llvm::VectorType*) const (bin/clang+0x6c9cc6a)
> > #13 0x0000558afb0d4f50 llvm::slpvectorizer::BoUpSLP::getEntryCost(llvm::slpvectorizer::BoUpSLP::TreeEntry*) (bin/clang+0x7da6f50)
> > ```
> > > Also, could you provide more details about your example? What shall I expect as a result!
> > 
> > It should not crash with this patch :-)
> > 
> > It says for me:
> > ```
> > SLP: Adding cost 0 for bundle that starts with   %add.i33 = fadd fast half %21, %22.
> > SLP: Current total cost = 0
> > SLP: Adding cost 6 for bundle that starts with   %21 = extractelement <8 x half> %20, i32 0.
> > SLP: Current total cost = 6
> > clang: /work/llvm-project/llvm/lib/IR/Instructions.cpp:2122: bool isSingleSourceMaskImpl(llvm::ArrayRef<int>, int): Assertion `I
> > >= 0 && I < (NumOpElts * 2) && "Out-of-bounds shuffle mask element"' failed.
> > ...
> > 0.      Program arguments: bin/clang -target arm-arm-none-eabi -mcpu=cortex-m55 -Ofast tmp.c -S -o - -mllvm -debug -mllvm -print-
> > after-all
> > ...
> >  #9 0x0000558afa5bc6de llvm::ShuffleVectorInst::isSingleSourceMask(llvm::ArrayRef<int>) (bin/clang+0x728e6de)
> > #10 0x0000558afa5bc911 llvm::ShuffleVectorInst::isSelectMask(llvm::ArrayRef<int>) (bin/clang+0x728e911)
> > #11 0x0000558af90f9c40 llvm::ARMTTIImpl::getShuffleCost(llvm::TargetTransformInfo::ShuffleKind, llvm::VectorType*, llvm::ArrayRef
> > <int>, int, llvm::VectorType*) ARMTargetTransformInfo.cpp:0:0
> > #12 0x0000558af9fcac6a llvm::TargetTransformInfo::getShuffleCost(llvm::TargetTransformInfo::ShuffleKind, llvm::VectorType*, llvm:
> > :ArrayRef<int>, int, llvm::VectorType*) const (bin/clang+0x6c9cc6a)
> > #13 0x0000558afb0d4f50 llvm::slpvectorizer::BoUpSLP::getEntryCost(llvm::slpvectorizer::BoUpSLP::TreeEntry*) (bin/clang+0x7da6f50)
> > ```
> 
> 
Ok, I see. I added some checks in SLP vectorizer but looks like we need some extra checks in other places. Or relax asserts in isSingleSourceMaskImpl function (maybe, using an extra parameter)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100865