This is an archive of the discontinued LLVM Phabricator instance.

[NFCI][CodeGen, AArch64] Fix inconsistent TargetCostKind types.
ClosedPublic

Authored by dfukalov on Sep 20 2021, 12:47 PM.

Details

Summary

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.

Diff Detail

Event Timeline

dfukalov created this revision.Sep 20 2021, 12:47 PM
dfukalov requested review of this revision.Sep 20 2021, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2021, 12:47 PM
fhahn added a subscriber: fhahn.Sep 22 2021, 1:40 AM

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?

https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Analysis/TargetTransformInfo.h#L1155

Do you think we should stop using default cost kinds entirely?

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.

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)?

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...

RKSimon accepted this revision.Sep 22 2021, 8:12 AM

LGTM with one (optional) minor

llvm/lib/CodeGen/InterleavedLoadCombinePass.cpp
1133–1134

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;
This revision is now accepted and ready to land.Sep 22 2021, 8:12 AM
This revision was landed with ongoing or failed builds.Sep 22 2021, 10:16 AM
This revision was automatically updated to reflect the committed changes.
dfukalov added inline comments.Sep 22 2021, 10:17 AM
llvm/lib/CodeGen/InterleavedLoadCombinePass.cpp
1133–1134

Good point, thanks!