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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
1209–1210 | Thank you Sander. |
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. |
- 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. |
LGTM - I thought I already accepted it. I'm happy with having the changes in the other patch.
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.