The pass uses different cost kinds to estimate "old" and "interleaved" costs:
default cost kind for all targets override getInterleavedMemoryOpCost() is
TCK_SizeAndLatency. Although at the moment estimated TCK_Latency costs are
equal to TCK_SizeAndLatency, (so the change is NFC) it may change in future.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks reasonable in general, thanks!
default cost kind for all targets override getInterleavedMemoryOpCost() is TCK_SizeAndLatency
Interesting, it looks like the top-level define in TTI defaults to TTI::TCK_RecipThroughput, so should this trump the default in the target-specific defines?
Do you think we should stop using default cost kinds entirely?
I think this would be for the best, and I thought I already changed most of the calls to be explicit.
@dfukalov Is this something you'd be willing to do in this patch? At least for getInstructionCost + getInterleavedMemoryOpCost (TTI + Impl)?
I guess suggested changes should be in different patch, I just checked who uses pure latency cost kind our days and found this inconsistency.
Perhaps the fix may be "less invasive" with using TCK_Latency in both places, but they seems equal at the moment and I'm not sure the author meant exactly pure latency here...
LGTM with one (optional) minor
llvm/lib/CodeGen/InterleavedLoadCombinePass.cpp | ||
---|---|---|
1133 | Would it make sense to add a common CostKind variable here and update all the getCost calls to use that? We already do something similar in SLP. TTI::TargetCostKind CostKind = TTI::TCK_SizeAndLatency; |
llvm/lib/CodeGen/InterleavedLoadCombinePass.cpp | ||
---|---|---|
1133 | Good point, thanks! |
Would it make sense to add a common CostKind variable here and update all the getCost calls to use that? We already do something similar in SLP.