[LV] Don't trigger legacy/vplan assert when forcing costs #156870
Conversation
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.
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: David Sherwood (david-arm) ChangesWhen 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:
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) ||
|
There was a problem hiding this comment.
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?
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.