Skip to content

Conversation

@ElvisWang123
Copy link
Contributor

Add legal checks in the vplan-based cost model that align to the legacy cost model (setCostBasedWideningDecision()).

This is patch is not quite a NFC patch but currently we don't have a target that support ActiveVectorLength && ScalableVector but not support gather/scatter nor return a valid cost by TTI.getGatherScatterOpCost.

Fixing the gap can prevent potentail failure in the future.

@llvmbot
Copy link
Member

llvmbot commented Sep 30, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Elvis Wang (ElvisWang123)

Changes

Add legal checks in the vplan-based cost model that align to the legacy cost model (setCostBasedWideningDecision()).

This is patch is not quite a NFC patch but currently we don't have a target that support ActiveVectorLength && ScalableVector but not support gather/scatter nor return a valid cost by TTI.getGatherScatterOpCost.

Fixing the gap can prevent potentail failure in the future.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp (+5)
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index 2f0ba5510b8f34..c941479e7c6589 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -2148,6 +2148,11 @@ InstructionCost VPWidenMemoryRecipe::computeCost(ElementCount VF,
     const Value *Ptr = getLoadStorePointerOperand(&Ingredient);
     assert(!Reverse &&
            "Inconsecutive memory access should not have the order.");
+    if ((isa<LoadInst>(Ingredient) &&
+         !Ctx.TTI.isLegalMaskedGather(Ty, Alignment)) ||
+        (isa<StoreInst>(Ingredient) &&
+         !Ctx.TTI.isLegalMaskedScatter(Ty, Alignment)))
+      return InstructionCost::getInvalid();
     return Ctx.TTI.getAddressComputationCost(Ty) +
            Ctx.TTI.getGatherScatterOpCost(Ingredient.getOpcode(), Ty, Ptr,
                                           IsMasked, Alignment, CostKind,

Add legal checks in the vplan-based cost model that align to the legacy
cost model (setCostBasedWideningDecision()).

This is patch is not quite a NFC patch but currently we don't have a
target that support `ActiveVectorLength && ScalableVector` but not support
`gather/scatter` nor return valid cost by `TTI.getGatherScatterOpCost`.

Fixing the gap can prevent potentail failure in the future.
@ElvisWang123 ElvisWang123 force-pushed the add-legal-checks-in-VPWidenMemoryRecipe-computeCost branch from 987ecd8 to c324fc5 Compare September 30, 2024 05:40
!Ctx.TTI.isLegalMaskedGather(Ty, Alignment)) ||
(isa<StoreInst>(Ingredient) &&
!Ctx.TTI.isLegalMaskedScatter(Ty, Alignment)))
return InstructionCost::getInvalid();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't getGatherScatterOpCost return Invalid in those cases as we pass IsMasked ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure who has the responsibility to do the legal check transformations or TTI.
In RISCV, getGatherScatterOpCost will do the legal check and return invalid cost when it fail.
In the default target, getGatherScatterOpCost just return the constant cost no matter it is legal or not.

I found the legacy cost model explicit check if the gather/scatter is legal in LoopVectorizationCostModel::setCostBasedWideningDecision().

So adding this check can prevent misaligned between legacy cost model and the vplan-based cost model when a target isLegalMaskedGather() = false && getGatherScatterOpCost() is valid .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I think the difference is that while the legacy cost model needs to consider all options for the input, no masked VPWidenMemoryRecipe should be created , if masked mem-ops aren't legal I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants