Page MenuHomePhabricator

[VPlan] Use VPUser to manage CondBit
ClosedPublic

Authored by fhahn on Jan 25 2021, 11:43 AM.

Details

Summary

VP blocks keep track of a condition, which is a VPValue. This patch
updates VPBlockBase to manage the value using VPUser, so
replaceAllUsesWith properly updates the condition bit as well.

This is required to enable VP2VP transformations and it helps with
simplifying some of the code required to manage condition bits.

Diff Detail

Event Timeline

fhahn created this revision.Jan 25 2021, 11:43 AM
fhahn requested review of this revision.Jan 25 2021, 11:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2021, 11:43 AM
Herald added a subscriber: vkmr. · View Herald Transcript

Hi Florian, I'm starting to familiarize myself with VPlan and wonder if that is a short/mid-term solution or something that is expected to be a long-term.

Have you considered modeling the edges using terminator instructions similar to what regular LLVM IR does? I see the following potential advantages:

  1. More straightforward thinking about dominance relation/theoretical conception. CondBit might be defined inside the VPBB itself and in order to have that use not to break the dominance we need the VPBB "defined" at its end, which might be slightly confusing. I'm not sure if that can lead to any practical difficulties though.
  2. By having a dedicated terminator we can put some special semantics/operation on the condition into that terminator. For example, VPBranchInst instruction for a regular branch on a uniform condition and something like VPBranchIfAllZero/VPBranchIfAllOnes(mask) when condition is varying. Of course, the same can be encoded into the VPBB itself, but the explicit instruction might be considered slightly more readable.
  3. Potential simplification of successors/predecessors handling - that will be done by the def/use edges.

A big disadvantage is, obviously, that the change would be more invasive and the benefits aren't really too "objective". Still, it might be worth considering this direction.

What do you think?

fhahn added a comment.Feb 1 2021, 7:47 AM

Hi Florian, I'm starting to familiarize myself with VPlan and wonder if that is a short/mid-term solution or something that is expected to be a long-term.

Hi, that's great!

Have you considered modeling the edges using terminator instructions similar to what regular LLVM IR does? I see the following potential advantages:

  1. More straightforward thinking about dominance relation/theoretical conception. CondBit might be defined inside the VPBB itself and in order to have that use not to break the dominance we need the VPBB "defined" at its end, which might be slightly confusing. I'm not sure if that can lead to any practical difficulties though.
  2. By having a dedicated terminator we can put some special semantics/operation on the condition into that terminator. For example, VPBranchInst instruction for a regular branch on a uniform condition and something like VPBranchIfAllZero/VPBranchIfAllOnes(mask) when condition is varying. Of course, the same can be encoded into the VPBB itself, but the explicit instruction might be considered slightly more readable.
  3. Potential simplification of successors/predecessors handling - that will be done by the def/use edges.

A big disadvantage is, obviously, that the change would be more invasive and the benefits aren't really too "objective". Still, it might be worth considering this direction.

What do you think?

Modeling terminators explicitly would certainly make a few things a bit nicer.

But I think in the short term, the amount of churn required to change that would not be worth it. The CondBit use in VPBlockBase is one of the few remaining cases not in the VPlan def-use chains and I think it would be preferable to get this wrapped up ASAP and re-visit the issue of explicit terminators in the future.

gilr added inline comments.Feb 7 2021, 1:44 PM
llvm/lib/Transforms/Vectorize/VPlan.h
388

Would it suffice for VPBlockBase to have a VPUser rather than being one? (may also align more easily with terminators in the future)

fhahn updated this revision to Diff 322065.Feb 8 2021, 3:04 AM

Do not make VPBlockBase a VPUser yet, use a VPUser to manage CondBit instead, as suggested.

fhahn added inline comments.Feb 8 2021, 3:06 AM
llvm/lib/Transforms/Vectorize/VPlan.h
388

Good idea, I updated the code to use a VPUser member to just manage the CondBit for now. There is at least VPValue *Predicate which should be updated before making VPBlockBase a VPUser, if we ever want to do that.

fhahn retitled this revision from [VPlan] Make VPBlockBase a VPUser. to [VPlan] Use VPUser to manage CondBit.Feb 8 2021, 7:10 AM
fhahn edited the summary of this revision. (Show Details)
gilr accepted this revision.Feb 8 2021, 1:25 PM
gilr added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
409

Nit: IIRC the condition bit is only used by the native path (LV uses the BranchOnMask recipe instead), but LV still sets the condition bit in the call to setTwoSuccessors in insertTwoBlocksAfter, which will now create a redundant user of the condition bit. Not sure it is/will be a problem, but we could pass nullptr there to avoid it (requires removing the assert in setTwoSuccessors).

This revision is now accepted and ready to land.Feb 8 2021, 1:25 PM
This revision was landed with ongoing or failed builds.Feb 9 2021, 1:54 PM
This revision was automatically updated to reflect the committed changes.
fhahn added inline comments.Feb 9 2021, 2:24 PM
llvm/lib/Transforms/Vectorize/VPlan.h
409

I am not sure I follow directly here. Do you mean using VPUser * and allocate it on demand? I think the extra field is currently not worth the trouble, and hopefully we will be able to unify the VPNative & regular paths in the future :)