Page MenuHomePhabricator

[SVE] Remove invalid calls to VectorType::getNumElements from BasicTTIImpl
ClosedPublic

Authored by ctetreau on Jun 9 2020, 12:40 PM.

Details

Summary

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.

Diff Detail

Event Timeline

ctetreau created this revision.Jun 9 2020, 12:40 PM
ctetreau marked an inline comment as done.Jun 9 2020, 12:48 PM
ctetreau added inline comments.
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.

ctetreau updated this revision to Diff 269701.Jun 9 2020, 4:52 PM

Catch some stragglers, remove helper

ctetreau updated this revision to Diff 269702.Jun 9 2020, 4:55 PM

actually remove the helper

ctetreau marked an inline comment as done.Jun 9 2020, 4:56 PM
ctetreau added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
90

I went ahead and removed the helper.

ctetreau edited the summary of this revision. (Show Details)Jun 9 2020, 4:57 PM
ctetreau updated this revision to Diff 270220.Jun 11 2020, 1:22 PM

remove superfluous casts and potential null dereferences

RKSimon added inline comments.Jun 16 2020, 1:18 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
84

We'd be better off just changing the function to take FixedVectorType, same for all the other cases below.

ctetreau marked an inline comment as done.Jun 16 2020, 9:39 AM
ctetreau added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
84

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.

ctetreau updated this revision to Diff 271131.Jun 16 2020, 10:01 AM

address code review issues

This revision is now accepted and ready to land.Jun 16 2020, 1:40 PM
This revision was automatically updated to reflect the committed changes.