Most of these operations are reasonable for scalable vectors. Due to
this, we have decided not to change the interface to specifically take
FixedVectorType despite the fact that the current implementations make
fixed width assumptions. Instead, we cast to FixedVectorType and assert
in the body. If a developer makes some change in the future that causes
one of these asserts to fire, they should either change their code or
make the function they are trying to call handle scalable vectors.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
90 | I think it's reasonable to bikeshed this name. I picked FIXME_ScalableVectorNotSupported because I wanted a name that is very aggressive in expressing that it's a fixme conversion. However, I recognize that it doesn't strictly conform to our documented coding style. |
I agree it makes sense to keep the TargetTransformInfo API as-is; we almost certainly want it to take a generic VectorType.
I'm not sure the FIXME_ScalableVectorNotSupported thing is useful here, as opposed to just writing cast<FixedVectorType>(). Most of these default cost implementations are working under the assumption that it makes sense to scalarize the operation, and estimating costs based on that. That's not generally useful for scalable vectors, and I suspect that we're just going to override all these methods in the AArch64 target, rather than try to fix the target-independent cost estimation. (At least in the short term.) If we do end up crashing anyway, it should be obvious why it's happening.
I'm not sure the FIXME_ScalableVectorNotSupported thing is useful here, as opposed to just writing cast<FixedVectorType>().
I like it for the reasons I mentioned. Strictly speaking, it is equivalent to just doing the cast, but I think it provides documentation value. If people feel strongly about removing it, I can do so.
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
90 | I went ahead and removed the helper. |
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
99 | We'd be better off just changing the function to take FixedVectorType, same for all the other cases below. |
llvm/include/llvm/CodeGen/BasicTTIImpl.h | ||
---|---|---|
99 | For the private methods, or all of them? I think changing the private methods to take FixedVectorType is fine. I prefer to leave the public methods as VectorType so that scalable architectures can use this interface. |
I think it's reasonable to bikeshed this name. I picked FIXME_ScalableVectorNotSupported because I wanted a name that is very aggressive in expressing that it's a fixme conversion. However, I recognize that it doesn't strictly conform to our documented coding style.