This patch adds a command line flag to be able to test the type based cost-model analysis
for Intrinsics.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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 | ||
---|---|---|
133 | 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." |
- 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, |
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?
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. |
llvm/lib/Analysis/TargetTransformInfo.cpp | ||
---|---|---|
70 | looks like you added a tabulation here instead of spaces |
incorrect formatting