Page MenuHomePhabricator

[VPlan] Introduce new VPWidenCallRecipe (NFC).

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



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

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


Missing a comment (and a newline).




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


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.

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

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.


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

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.

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

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!


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.