This is an archive of the discontinued LLVM Phabricator instance.

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

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

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
11096

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
312–313

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

ABataev updated this revision to Diff 451873.Aug 11 2022, 8:43 AM

Fix poison leak for vectorized tree top values.

spatel added inline comments.Aug 31 2022, 12:10 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
10312–10315

I think we should just fix this to include isa<SelectInst>, so it matches the code comment. Then, we don't need the confusing extra variable in this patch. All of the existing uses are checking for SelectInst, so that can be a preliminary cleanup patch.

ABataev updated this revision to Diff 457083.Aug 31 2022, 1:58 PM

Address comment

spatel accepted this revision.Aug 31 2022, 3:11 PM

LGTM

This revision is now accepted and ready to land.Aug 31 2022, 3:11 PM
This revision was landed with ongoing or failed builds.Sep 1 2022, 5:35 AM
This revision was automatically updated to reflect the committed changes.