This is an archive of the discontinued LLVM Phabricator instance.

[RFC][LV] VPlan-based cost model
Needs ReviewPublic

Authored by arcbbb on Aug 24 2023, 1:12 AM.

Details

Summary

This patch follows D89322 to add an initial skeleton of vplan-based cost model.

This difference is that instead of incorporating a cost() interface to VPRecipes,
all cost implementations are put together in VPlanCostModel.

This allows VPlanCostModel to concentrate on assigning costs to vplan,
thus seprating the cost model code from the vplan IR, similar to LLVM IR cost
modeling.

During the transition, it will still use the legacy model to obtain cost until
all cost calculation for recipes are implemented.

Please let me know if you agree with the main idea of this patch.
If there is a general consensus, I'll proceed to implement the cost for the
other recipes for review.

Diff Detail

Event Timeline

arcbbb created this revision.Aug 24 2023, 1:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 1:12 AM
arcbbb requested review of this revision.Aug 24 2023, 1:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 1:12 AM
wangpc added inline comments.Aug 24 2023, 8:44 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8710

Is it always true even for scalar VF? Why can't we have the same signature as CM.expectedCost?

llvm/lib/Transforms/Vectorize/VPlanCostModel.h
31

RVL->VF?

arcbbb updated this revision to Diff 553895.Aug 28 2023, 5:08 AM
  • Address comments
arcbbb added inline comments.Aug 28 2023, 5:12 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8710

VectorizationCostTy is not exported in the header, so I change the interface trying to decouple the dependency.

llvm/lib/Transforms/Vectorize/VPlanCostModel.h
31

Fixed. Thanks!

rengolin added inline comments.Aug 28 2023, 5:58 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8708

It's costly to create this object in every inner loop.

This should either be hoisted to the outer loop, or outside of the loop altogether if you allow expectedCost to have the Plan as an argument, since it's the only function in VPlanCostModel that uses it.

arcbbb updated this revision to Diff 554135.Aug 28 2023, 7:08 PM
  • Move VPCM object outside of the loop
arcbbb added inline comments.Aug 28 2023, 7:10 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8708

Good point!

nikolaypanchenko added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8708

Cost model may eventually have a state to track context information. Even though it's possible to have a single instance of VPCM, but this is going to add extra boilerplate to manage that state.

I agree with the general idea and the implementation seems reasonable, but I haven't worked in this part of the code for a long time, so would like folks like @fhahn and @Ayal to approve this one.

I'd like to see tests with the new flag. Perhaps reusing existing tests with a new RUN line and checking that the decisions are the same. After all, we don't want to change the behaviour with this patch, just the infrastructure.

Here is going to be read-only, so I moved this patch to github pr67647

I'd like to see tests with the new flag. Perhaps reusing existing tests with a new RUN line and checking that the decisions are the same. After all, we don't want to change the behaviour with this patch, just the infrastructure.

Sure, I'll incorporate tests with the new flag and try existing tests with a new RUN line to ensure that the decisions remain consistent.