[LV] Don't trigger legacy/vplan assert when forcing costs

[david-arm]

When forcing an instruction cost of 1, for example, the vplan cost model will treat an entire load interleave group as being a cost of 1, whereas the legacy cost model will treat each load in the group as having a cost of 1. I don't believe it makes any sense to trigger the assert for matching legacy and vplan cost models when forcing an instruction cost. Given the reason for having the option to force an instruction cost is to encourage greater testing of a PR, it seems like frequently triggering the assert will simply deter people from doing so.

[llvmbot]

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: David Sherwood (david-arm)

Changes

When forcing an instruction cost of 1, for example, the legacy cost model will treat an entire load interleave group as being a cost of 1, whereas the legacy cost model will treat each load in the group as having a cost of 1. I don't believe it makes any sense to trigger the assert for matching legacy and vplan cost models when forcing an instruction cost. Given the reason for having the option to force an instruction cost is to encourage greater testing of a PR, it seems like frequently triggering the assert will simply deter people from doing so.

---

Full diff: https://github.com/llvm/llvm-project/pull/156870.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+6)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 3fbeef1211954..36d27e89d4ebd 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -7080,7 +7080,13 @@ VectorizationFactor LoopVectorizationPlanner::computeBestVF() {
   // Verify that the VPlan-based and legacy cost models agree, except for VPlans
   // with early exits and plans with additional VPlan simplifications. The
   // legacy cost model doesn't properly model costs for such loops.
+  // NOTE: If the user has forced a target instruction cost this assert is very
+  // likely to trigger because the VPlan recipes don't map 1:1 with the scalar
+  // instructions that the legacy cost model is based on. One example of this is
+  // for interleave groups - VPlan will use the forced cost for the whole group,
+  // whereas the legacy cost model will use it for each load.
   assert((BestFactor.Width == LegacyVF.Width || BestPlan.hasEarlyExit() ||
+          ForceTargetInstructionCost.getNumOccurrences() > 0 ||
           planContainsAdditionalSimplifications(getPlanFor(BestFactor.Width),
                                                 CostCtx, OrigLoop,
                                                 BestFactor.Width) ||

[lukel97]

Makes sense to me

In the PR description:

the legacy cost model will treat an entire load interleave group as being a cost of 1, whereas the legacy cost model will treat each load in the group as having a cost of 1.

Should this say the VPlan cost model will treat an entire load interleave group as being a cost of 1?

[fhahn]

Could you add a test case that would crash w/o this patch?

[fhahn]

Hmm, I am wondering if the treatment of the legacy cost model makes more sense here. We could easily match it in the VPlan cost model I think, we know how many source instructions we had (# of members in the group) and could just multiply this by the forced cost?

[david-arm]

Hmm, but then I'd suggest we need a separate flag along the lines of -force-target-recipe-cost=1 so that users could really force the cost to 1 if they desire. Then this new flag would face the same issue.

To be honest, even if with your suggestion I still think it doesn't make sense to assert that one completely arbitrary cost model matches another completely arbitrary cost model. In this case it feels like the assert is actually getting in the way of diligent testing of all code paths in the vectoriser.

[fhahn]

Hmm, it depends I guess, both cost models still try to compute the costs of the same result.

The difference is to either consider the cost of all instructions together in an interleave group as the the forced cost (what VPInterleaveRecipe does currently) or ignore the fact that members of the group will be combined and used the forced cost for each member.

I think one could argue for either behavior (VPInterleaveRecipe will generate a single memory op, but it is very wide and in practice we should end up with one legal memory op per member); it just would be preferable to have a consistent meaning for the flag.

I've not looked into it yet, but I think we could also match the behavior of VPInterleaveRecipe in the legacy cost model, by checking if the instruction should get lowered to an interleave group, and if so only apply the forced cost to the insertion point member.

[david-arm]

OK, I'm happy to look into that approach, but still it feels like a lot of effort to fix something that the developer probably doesn't care about. Obviously this is just my own example, but when I used the flag -force-instruction-cost=1 I wasn't interested in knowing whether they agreed or not. What I really cared about was that all code paths have been tested, nothing crashes during compilation or execution. Sometimes the assert just gets in the way of making sure a patch works correctly and I frequently have to delete the assert and rebuild.

One alternative I'd also be happy with is an override flag that allows developers to disable the assert.

[fhahn]

Yeah it looks like there are at least a few other isses with forcing target instruction costs, but it seems like some of them seem more like bugs in the VPlan-based cost model. With fhahn@b781919, I can't see any crashes with -force-target-instruction-cost=1 on a quite large test set.

Might be worth a try on the test set on your end, if you have a chance