[PATCH] D143786: [X86] Add `TuningPreferShiftShuffle` for when Shifts are preferable to shuffles.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D143786#4163291 <https://reviews.llvm.org/D143786#4163291>, @rupprecht wrote:

> After this patch, I see an msan issue running this test internally; strangely I don't see a failure on any sanitizer buildbot yet.
>
> There are ~400 test failures in LLVM, e.g. for the test in `llvm/test/CodeGen/Generic/vector-casts.ll`:
>
>   ==3443==WARNING: MemorySanitizer: use-of-uninitialized-value
>       #0 0x55f66059dba1 in matchUnaryPermuteShuffle(llvm::MVT, llvm::ArrayRef<int>, llvm::APInt const&, bool, bool, llvm::SelectionDAG const&, llvm::X86Subtarget const&, unsigned int&, llvm::MVT&, unsigned int&) llvm/lib/Target/X86/X86ISelLowering.cpp:38834:38
>       #1 0x55f66058ed18 in combineX86ShuffleChain(llvm::ArrayRef<llvm::SDValue>, llvm::SDValue, llvm::ArrayRef<int>, int, bool, bool, bool, llvm::SelectionDAG&, llvm::X86Subtarget const&) llvm/lib/Target/X86/X86ISelLowering.cpp:39512:9
>       #2 0x55f6603aa410 in combineX86ShufflesRecursively(llvm::ArrayRef<llvm::SDValue>, int, llvm::SDValue, llvm::ArrayRef<int>, llvm::ArrayRef<llvm::SDNode const*>, unsigned int, unsigned int, bool, bool, bool, llvm::SelectionDAG&, llvm::X86Subtarget const&) llvm/lib/Target/X86/X86ISelLowering.cpp:40791:27
>       #3 0x55f660620689 in combineX86ShufflesRecursively(llvm::SDValue, llvm::SelectionDAG&, llvm::X86Subtarget const&) llvm/lib/Target/X86/X86ISelLowering.cpp:40818:10
>       #4 0x55f66045d4b4 in combineVectorPack(llvm::SDNode*, llvm::SelectionDAG&, llvm::TargetLowering::DAGCombinerInfo&, llvm::X86Subtarget const&) llvm/lib/Target/X86/X86ISelLowering.cpp:48525:21
>       #5 0x55f6603c092c in llvm::X86TargetLowering::PerformDAGCombine(llvm::SDNode*, llvm::TargetLowering::DAGCombinerInfo&) const llvm/lib/Target/X86/X86ISelLowering.cpp:57229:36
>       #6 0x55f661b6df5b in (anonymous namespace)::DAGCombiner::combine(llvm::SDNode*) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2014:16
>       #7 0x55f661b6c8f4 in Run llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1795:18
>       #8 0x55f661b6c8f4 in llvm::SelectionDAG::Combine(llvm::CombineLevel, llvm::AAResults*, llvm::CodeGenOpt::Level) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:26935:36
>       #9 0x55f66214e2e3 in llvm::SelectionDAGISel::CodeGenAndEmitDAG() llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:923:13
>       #10 0x55f662147e92 in llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1633:7
>       #11 0x55f662141c18 in llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:480:3
>       #12 0x55f660981cf0 in (anonymous namespace)::X86DAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:191:25
>       #13 0x55f661633d50 in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) llvm/lib/CodeGen/MachineFunctionPass.cpp:91:13
>       #14 0x55f664c0c6f7 in llvm::FPPassManager::runOnFunction(llvm::Function&) llvm/lib/IR/LegacyPassManager.cpp:1430:27
>       #15 0x55f664c192d9 in llvm::FPPassManager::runOnModule(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:1476:16
>       #16 0x55f664c0d785 in runOnModule llvm/lib/IR/LegacyPassManager.cpp:1545:27
>       #17 0x55f664c0d785 in llvm::legacy::PassManagerImpl::run(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:535:44
>       #18 0x55f65d621281 in compileModule(char**, llvm::LLVMContext&) llvm/tools/llc/llc.cpp:733:8
>       #19 0x55f65d61a872 in main llvm/tools/llc/llc.cpp:420:22
>   
>   
>     Uninitialized value was created by an allocation of 'Shuffle' in the stack frame
>       #0 0x55f66058a88e in combineX86ShuffleChain(llvm::ArrayRef<llvm::SDValue>, llvm::SDValue, llvm::ArrayRef<int>, int, bool, bool, bool, llvm::SelectionDAG&, llvm::X86Subtarget const&) llvm/lib/Target/X86/X86ISelLowering.cpp:39461:3
>   
>   SUMMARY: MemorySanitizer: use-of-uninitialized-value llvm/lib/Target/X86/X86ISelLowering.cpp:38834:38 in matchUnaryPermuteShuffle(llvm::MVT, llvm::ArrayRef<int>, llvm::APInt const&, bool, bool, llvm::SelectionDAG const&, llvm::X86Subtarget const&, unsigned int&, llvm::MVT&, unsigned int&)

I think the issue is:

  +      // Byte shifts can be slower so only match them on second attempt.
  +      if (Order == 0 &&
  +          (Shuffle == X86ISD::VSHLDQ || Shuffle == X86ISD::VSRLDQ))
  +        continue;

It comes before the check of

  +      if (0 < ShiftAmt && (!ShuffleVT.is512BitVector() || Subtarget.hasBWI() ||
  +                           32 <= ShuffleVT.getScalarSizeInBits())) {
  +        PermuteImm = (unsigned)ShiftAmt;
  +        return true;
  +      }

and the `0 < ShiftAmt` check if basically a check if actually found/set `Shuffle`.
Don't think the bug actually can change behavior but is bug none the less.

Will post patch to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143786