Page MenuHomePhabricator

[SLP]Fix PR55734: SLP vectorizer's reduce_and formation introduces poison.
Needs ReviewPublic

Authored by ABataev on Jun 2 2022, 6:05 AM.

Details

Summary

Need either follow the original order of the operands for bool logical
ops, or emit freeze instruction to avoid poison propagation.

Diff Detail

Unit TestsFailed

TimeTest
60,050 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld

Event Timeline

ABataev created this revision.Jun 2 2022, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 6:05 AM
ABataev requested review of this revision.Jun 2 2022, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 6:05 AM
ABataev updated this revision to Diff 434522.Jun 6 2022, 10:16 AM

Rebase and use isGuaranteedNotToBePoison as an extra check

spatel added inline comments.Jul 1 2022, 8:13 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
11286

This could use a comment to describe the patterns/logic.

IIUC, we want to create a bool logic op like:

select i1 LHS, i1 RHS, i1 false ; logical and

If we have original code like this:

RedOp1 = select i1 ?, i1 LHS, i1 false
RedOp2 = select i1 RHS, i1 ?, i1 false

Then, we swap LHS/RHS to create a new op that matches the poison semantics of the original code.

If we have original code like this and both values could be poison:

RedOp1 = select i1 ?, i1 LHS, i1 false
RedOp2 = select i1 ?, i1 RHS, i1 false

Then, we must freeze LHS in the new op. Can we manufacture a test for this 2nd path? I don't think there's any coverage for that path in the current test diffs.

ABataev updated this revision to Diff 441783.Jul 1 2022, 1:39 PM

Reworked addition of the reduction. For logical ops, reduction result is added as a first operand,
it is safe to use it as an LHS in scalar logical ands/ors.
For others, it is added as a last operand.

spatel added inline comments.Jul 3 2022, 6:43 AM
llvm/test/Transforms/SLPVectorizer/X86/reduction-logical.ll
311–312

The updated patch does not fix the bug?
https://alive2.llvm.org/ce/z/YYaTVw