This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Introduce new VPWidenCallRecipe (NFC).
ClosedPublic

Authored by fhahn on Apr 4 2020, 10:41 AM.

Details

Summary

This patch moves calls to their own recipe, to simplify the transition
to VPUser for operands of VPWidenRecipe, as discussed in D76992.

Subsequently additional information can be added to the recipe rather
than computing it during the execute step.

Diff Detail

Event Timeline

fhahn created this revision.Apr 4 2020, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2020, 10:41 AM
fhahn updated this revision to Diff 255066.Apr 4 2020, 11:38 AM

Update comment.

gilr added inline comments.Apr 4 2020, 12:36 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4234

Instruction::Call now belongs here with the other special-widening instructions to explicitly denote it's being handled by another recipe.

llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
110

Missing a comment (and a newline).

llvm/lib/Transforms/Vectorize/VPlan.cpp
717

"WIDEN-CALL"

717

Since it's a single-ingredient recipe, better make this a single-line print as in VPWidenPHIRecipe (actually also the case for VPWidenRecipe now).

llvm/lib/Transforms/Vectorize/VPlan.h
787

Missing a comment.

fhahn updated this revision to Diff 255083.Apr 4 2020, 1:57 PM

Address comments, thanks!

fhahn marked 6 inline comments as done.Apr 4 2020, 2:00 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.cpp
717

Right, I wasn't entirely sure about the DOT syntax. I think the DOT syntax might be a bit verbose for just printing it in -debug. Maybe we should add a way to print a VPlan without DOT syntax (for regular debug output) and an option to toggle DOT/non-DOT output. WDYT?

actually also the case for VPWidenRecipe now).

If the style here is fine I'll push a similar fix for VPWidenRecipe.

Ayal added inline comments.Apr 5 2020, 2:23 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7074–7075

add here

(Recipe = tryToWidenCall(Instr, Range)) ||

promoting VPWidenCallRecipe to join the "specific widening recipes", keeping tryToWiden() called below as kitchen-sink? It's somewhat confusing to have tryToWiden() call tryToWidenCall(). Similar to the promotion of inlined "tryToWidenGEP" in D69067.

Also augmenting above "with memory operations, [calls,] inductions and Phi nodes" comment.

llvm/lib/Transforms/Vectorize/VPlan.h
618

Lexical order.

fhahn updated this revision to Diff 255133.Apr 5 2020, 3:41 AM
fhahn marked 3 inline comments as done.

Address Ayal's comments, thanks!

Ayal added inline comments.Apr 5 2020, 4:03 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4364

Note that the *decision* not to widen calls to debug intrinsics should be made when building VPlan, in tryToWidenCall() or perhaps added to DeadInstructions(?), instead of here during code-gen.
Can be taken care of here, or in a subsequent patch along with migrating NeedToScalarize/UseVectorIntrinsics decisions "how" to widen calls.

fhahn marked an inline comment as done.Apr 5 2020, 4:21 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4364

I noticed the same thing but I didn't want to turn this into an assertion in this patch, but rather as a follow-up. There might be something better we can do for debug info intrinsics (e.g. update the values they reference to the vector values), but as long as we just drop them, we should drop them early. I think best addressed in subsequent patches. WDYT?

Ayal added inline comments.Apr 5 2020, 4:56 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4364

Sure; perhaps worth leaving a TODO behind.

fhahn updated this revision to Diff 255142.Apr 5 2020, 5:38 AM
fhahn marked an inline comment as done.

Add todo about handling of debug intrinsics.

gilr accepted this revision.Apr 5 2020, 7:29 AM

LGTM, thanks!

llvm/lib/Transforms/Vectorize/VPlan.cpp
717

Sure, would be good to also have a text-only dump; AFAIK DOTs are indeed usually dumped in response to some -dot-xxx flag.

This revision is now accepted and ready to land.Apr 5 2020, 7:29 AM
vkmr added a subscriber: vkmr.Apr 6 2020, 8:06 AM
This revision was automatically updated to reflect the committed changes.