[VPlan] Use VPUser to manage CondBit

[fhahn]

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.

[a.elovikov]

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]

In D95382#2528700, @a.elovikov wrote:

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]

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]

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

[fhahn]

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.

[gilr]

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).

[fhahn]

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 :)