Page MenuHomePhabricator

[LV] Allow assume calls in predicated blocks.
Needs ReviewPublic

Authored by fhahn on Oct 10 2019, 9:50 AM.

Details

Summary

The assume intrinsic is intentionally marked as may reading/writing
memory, to avoid passes moving them around. When flattening the CFG
for predicated blocks, we have to drop the assume calls, as they
are control-flow dependent.

Note that this patch also moves setting State->CFG.PrevVPBB after
we executed the recipes in a VPBB. This way, we have access to the real
VPBB while executing a block.

Fixes PR43620.

Diff Detail

Event Timeline

fhahn created this revision.Oct 10 2019, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 9:50 AM
xbolva00 added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
904

You can drop m_Value()

if (match(&I, m_Intrinsic<Intrinsic::assume>())) {

Wil work too

hfinkel added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6941

Unintentional whitespace change?

fhahn updated this revision to Diff 224464.Oct 10 2019, 1:44 PM

Address comments, thanks!

fhahn marked 2 inline comments as done.Oct 10 2019, 1:44 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
904

Excellent, thanks!

fhahn marked an inline comment as done.Oct 10 2019, 1:46 PM
fhahn added inline comments.
llvm/test/Transforms/LoopVectorize/predicate-assume.ll
3

I've not added a test with calls to assume in the loop header here, because we already have them in llvm/test/Transforms/LoopVectorize/X86/assume.ll.

Ayal added a comment.Oct 13 2019, 3:25 AM

If conditional assumes are to be dropped, better do so on entry to VPlan, as in DeadInstructions, rather than representing them in ReplicateRecipe (as do unconditional assumes) and silencing their code generation.

To retain conditional assumes along with their control flow, they could be marked under isScalarWithPredication; but this complicates vectorization, plus what use are such assumes when all else is if-converted(?)

Conditional assumes under uniform control flow could be retained, along with the uniform control flow they depend upon; this may be mostly relevant for outerloop vectorization.

hsaito added a comment.EditedOct 14 2019, 10:44 AM

If conditional assumes are to be dropped, better do so on entry to VPlan, as in DeadInstructions, rather than representing them in ReplicateRecipe (as do unconditional assumes) and silencing their code generation.

To retain conditional assumes along with their control flow, they could be marked under isScalarWithPredication; but this complicates vectorization, plus what use are such assumes when all else is if-converted(?)

Conditional assumes under uniform control flow could be retained, along with the uniform control flow they depend upon; this may be mostly relevant for outerloop vectorization.

To be more precise:

Assumes should be dropped at the time the vectorizer makes the incompatible decision ---- i.e. a assume is no longer "to be guarded" by the original (per-element or uniform) condition. Right now, in the non-VPlan-native path, recording of the decisions already made happens at entry to VPlan. In the VPlan native path, this would be VPlan to VPlan xform, as the decisions are recorded as they are made.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
155

Do we want to keep this white space?

fhahn added a comment.Oct 20 2019, 4:23 PM

If conditional assumes are to be dropped, better do so on entry to VPlan, as in DeadInstructions, rather than representing them in ReplicateRecipe (as do unconditional assumes) and silencing their code generation.

To retain conditional assumes along with their control flow, they could be marked under isScalarWithPredication; but this complicates vectorization, plus what use are such assumes when all else is if-converted(?)

Conditional assumes under uniform control flow could be retained, along with the uniform control flow they depend upon; this may be mostly relevant for outerloop vectorization.

I thought it might be desirable to only drop the calls at the point where we know for certain that we cannot preserve them: when we emit them in a block that gets merge in its predecessor. By doing it in the recipe, it should also be applicable to the outer loop vectorization. Alternatively we can drop them all as a first step, if that is preferred. Please let me know :)

I think we should try too hard to retain them (through marking them as scalar-with-predication), as the benefit of them for the vectorized loop after the vectorizer is likely to be quite small.

Ayal added a comment.Oct 21 2019, 1:52 PM

If conditional assumes are to be dropped, better do so on entry to VPlan, as in DeadInstructions, rather than representing them in ReplicateRecipe (as do unconditional assumes) and silencing their code generation.

To retain conditional assumes along with their control flow, they could be marked under isScalarWithPredication; but this complicates vectorization, plus what use are such assumes when all else is if-converted(?)

Conditional assumes under uniform control flow could be retained, along with the uniform control flow they depend upon; this may be mostly relevant for outerloop vectorization.

I thought it might be desirable to only drop the calls at the point where we know for certain that we cannot preserve them: when we emit them in a block that gets merge in its predecessor. By doing it in the recipe, it should also be applicable to the outer loop vectorization. Alternatively we can drop them all as a first step, if that is preferred. Please let me know :)

I vote to simply drop conditional assumes as a first step, at this time. In any case it's better in general if decisions get represented explicitly in VPlan, rather than having such logic kick in during code-gen. (Even if a block gets merged in its predecessor, better also check that the latter has more than one successor.) Following D68577 such assumes could be dropped as a VPlan-to-VPlan transformation.

I think we should try too hard to retain them (through marking them as scalar-with-predication), as the benefit of them for the vectorized loop after the vectorizer is likely to be quite small.

should >> should not?

For the sake of completeness, note that a conditional assume could be retained even under predication, by combining its block predicate within the assume predicate. I.e., a conditional "if (c) assume (p)" can be converted to an unconditional "assume (c implies p)" or "assume (!c || p)".

fhahn planned changes to this revision.Oct 31 2019, 3:38 PM

Thanks, I’ll update the patch to drop the assume calls for now and let’s go from there

uenoku added a subscriber: uenoku.Nov 2 2019, 9:24 AM
fhahn updated this revision to Diff 231111.Tue, Nov 26, 11:15 AM

Updated to use Vplan2VPlan transform to drop assume calls, if required.

Build result: FAILURE - Could not check out parent git hash "dcb1398d042ac24cdf8ff0ff7ac7ad2e310ac2dd". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt

gilr added a subscriber: gilr.Fri, Dec 6, 1:44 AM
gilr added inline comments.
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
89

Shouldn't this recurse into regions for completeness?

98

This block is only asserting. Can it be compressed into the asserted expression / go under an #ifdef assert / use llvm_unreachable()?

llvm/lib/Transforms/Vectorize/VPlanTransforms.h
32

dropAssumes()?

llvm/test/Transforms/LoopVectorize/predicate-assume.ll
10

Enough to CHECK for VPlan and then CHECK-NOT for the assert?

39

Enough to CHECK for vector.body and then CHECK-NOT for the assert?