This is an archive of the discontinued LLVM Phabricator instance.

[Costmodel] Add "type-based-intrinsic-cost" cli option
ClosedPublic

Authored by malharJ on Jul 5 2022, 1:17 AM.

Details

Summary

This patch adds a command line flag to be able to test the type based cost-model analysis
for Intrinsics.

Diff Detail

Event Timeline

malharJ created this revision.Jul 5 2022, 1:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 1:17 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
malharJ requested review of this revision.Jul 5 2022, 1:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 1:17 AM
malharJ updated this revision to Diff 442289.Jul 5 2022, 6:45 AM
  • Added updates when printing instruction cost using new pass manager.
malharJ updated this revision to Diff 442365.Jul 5 2022, 11:27 AM
  • Updated LIT test
  • minor formatting updates
mgabka added a subscriber: mgabka.
mgabka added a comment.Jul 6 2022, 1:34 AM

I think it would be good to explain in the commit message that this patch allows to print cost calculated only based on argument types and that it is useful feature to have for testing purposes.

llvm/include/llvm/Analysis/TargetTransformInfo.h
134

incorrect formatting

llvm/lib/Analysis/TargetTransformInfo.cpp
62

would it be beneficial to keep the TypeBasedOnly as a property of IntrinsicCostAttributes ?

A couple of drive-through Nit comments. Feel free to skip.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1787–1793 ↗(On Diff #442365)

Nit: Would this be better for here and below?

llvm/lib/Analysis/CostModel.cpp
118

Nit: Consider using braces everywhere in an if-else construct, here and below.

" For example, readability is also harmed if an if/else chain does not use braced bodies for either all or none of its members, ... etc."
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

malharJ updated this revision to Diff 442543.Jul 6 2022, 6:09 AM
malharJ marked 2 inline comments as done.
  • Addressed minor formatting issues
llvm/lib/Analysis/TargetTransformInfo.cpp
62

I think given that there is only one usage right now, ie. in the constructor below,
perhaps it's not worth making into a member variable ?

malharJ retitled this revision from [Costmodel] Add type based costmodel analysis for masked scatter/gather intrinsics to [Costmodel] Add "type-based-intrinsic-cost" cli option.Jul 12 2022, 1:49 AM
malharJ edited the summary of this revision. (Show Details)
malharJ edited the summary of this revision. (Show Details)

It looks reasonable to me.
The main issue we saw in the past was assertion failure when the cost for gather/scatter wasn't implemented, but you are saying that now it is simply returning invalid cost so the type based cost model for gather/scatter is not needed.
But I guess it does not hurt to add it.
@david-arm you did some work on the cost model for gather/scatter from what I can see, could you have a look if you agree with the proposed changes?

mgabka added inline comments.Jul 12 2022, 6:15 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
1782 ↗(On Diff #442543)

@malharJ could you remove this and the case below?

I think this patch could be limited to just adding the new interface to the cost model to allow extra testing.

With the cost for masked_gather and massked_scatter, it needs to be done a bit differently as with this changes you made it will always add in the legalization cost. I think it needs to be done by adding extra AArch64TTIImpl::getTypeBasedIntrinsicInstrCost method

But the main purpose of this patch is to ensure that all paths in the cost model can be unit tested and returning an invalid cost for scalable vectors is still something what compiler should support in other parts.

malharJ updated this revision to Diff 445510.Jul 18 2022, 8:04 AM
  • Removed the SVE gather/scatter case as per review comment
malharJ edited the summary of this revision. (Show Details)Jul 18 2022, 8:08 AM
malharJ updated this revision to Diff 445520.EditedJul 18 2022, 8:26 AM
  • removed blank line
mgabka added inline comments.Jul 22 2022, 5:03 AM
llvm/lib/Analysis/TargetTransformInfo.cpp
70

looks like you added a tabulation here instead of spaces

mgabka accepted this revision.Jul 22 2022, 7:39 AM
This revision is now accepted and ready to land.Jul 22 2022, 7:39 AM
This revision was landed with ongoing or failed builds.Jul 22 2022, 7:51 AM
This revision was automatically updated to reflect the committed changes.