Page MenuHomePhabricator

[NFC][CostModel]Extend class IntrinsicCostAttributes to use ElementCount Type
ClosedPublic

Authored by CarolineConcatto on Mon, Nov 16, 5:14 AM.

Details

Summary

This patch replaces the attribute unsigned VF in the class
IntrinsicCostAttributes by ElementCount VF.
This is a non-functional change to help upcoming patches to compute the cost
model for scalable vector inside this class.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
CarolineConcatto requested review of this revision.Mon, Nov 16, 5:14 AM

-remove non-related changes on Target AMDGPU

Looks like a sensible change to me.

nit: can you please add NFC: in front of the title, like: NFC: Extend class ..., that way people can easily see the patch is NFC and shouldn't cause any side-effects.
nit: the title has the tag [SVE], but there is nothing specific to SVE in this patch, so you can remove it.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1209–1210

Can you also change unsigned VF and unsigned RetVF to be of type ElementCount?
From what I can see, the changes required for that are quite mechanical, as long as you 'stop' at getOperandsScalarizationOverhead, because that seems like an interface change for another patch.

CarolineConcatto retitled this revision from [SVE][CostModel]Extend class IntrinsicCostAttributes to use ElementCount Type to [NFC][CostModel]Extend class IntrinsicCostAttributes to use ElementCount Type.Mon, Nov 16, 7:44 AM

-address review's comment

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1209–1210

Thank you Sander.
I've replaced RetVF and VF by ElementCount.
I am using isScalar() where before we had comparing equal to 1 because I imagine that VF will not use a scalable vector (line 1148) and RetVF is not a vector when RetTy->isVectorTy() is false (line 1152).
Like you said I leave getOperandsScalarizationOverhead still using unsigned.

sdesmalen added inline comments.Wed, Nov 18, 7:08 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
1212

This can now check for VectorType.

1325

This can now use VectorType::get(OpTy, VF)

1331

This can use VectorType.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1212

This change means that we can now accept scalable vectors, meaning that we will have a functional change. That is not the goal of this patch. I am working on another patch after this that will make the functional change to accept scalable vectors.
I believe it is fine to leave this patch as it is and create another patch that will be able to accept the scalable vector and then add a regression test to validate the change.

david-arm accepted this revision.Mon, Nov 30, 3:25 AM
  • nit: Can you fix up the formatting issue before merging? Thanks!
llvm/include/llvm/CodeGen/BasicTTIImpl.h
1331

Hi @CarolineConcatto FWIW I think @sdesmalen is right here and you can use VectorType::get and pass in the element count directly. I think this still counts as NFC because the ElementCount will always be for a fixed vector type anyway due to how the ElementCount is created above. However, I think it's fine to leave it like this, since you're fixing this up in a later patch.

This revision is now accepted and ready to land.Mon, Nov 30, 3:25 AM
sdesmalen accepted this revision.Mon, Nov 30, 3:36 AM

LGTM - I thought I already accepted it. I'm happy with having the changes in the other patch.

This revision was landed with ongoing or failed builds.Tue, Dec 1, 3:13 AM
This revision was automatically updated to reflect the committed changes.