Page MenuHomePhabricator

[SVE] Eliminate calls to VectorType::getNumElements from BasicTTIImpl.h
AbandonedPublic

Authored by ctetreau on May 19 2020, 1:23 PM.

Details

Summary

Handle the fallout in the various backends that use this

Diff Detail

Event Timeline

ctetreau created this revision.May 19 2020, 1:23 PM
Herald added a project: Restricted Project. · View Herald Transcript
samparker accepted this revision.May 20 2020, 12:40 AM

There's quite a few casts of ShuffleVectorInst types and I was wondering whether this class will also be ported to the Fixed type?

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1146

I'm assuming this cast isn't necessary?

This revision is now accepted and ready to land.May 20 2020, 12:40 AM
ctetreau marked an inline comment as done.May 20 2020, 9:50 AM

There's quite a few casts of ShuffleVectorInst types and I was wondering whether this class will also be ported to the Fixed type?

It's possible to do shufflevector on scalable vectors if you jump through some hoops. For instance, you can make a splat if you insertelement the splat value into the zeroeth element of undef, then shufflevector the result of that with undef and zeroinitializer as the mask. There's an RFC to extend shufflevector on scalable vectors: http://lists.llvm.org/pipermail/llvm-dev/2020-January/138762.html. It hasn't been updated in a while, but it's not dead, just dormant.

For now, the plan is to assume all usages of getNumElements are in fact fixed-width operations. If the assumption is incorrect, LLVM will fail to cast right away, rather than miscompile or blow up 10 functions down the line.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1146

It actually is. RetTy is a Type * and getScalarizationOverhead calls FixedVectorType::getNumElements().

It looks like the type returned by ICA.getReturnType() might be void, so we can't assign it to a FixedVectorType *.

ctetreau updated this revision to Diff 267342.May 29 2020, 12:55 PM

Before attempting to dyn_cast to FixedVectorType, assert that the type is not a scalable vector.

ctetreau updated this revision to Diff 268849.Jun 5 2020, 9:37 AM

add scalable vector assert

ctetreau abandoned this revision.Jun 9 2020, 12:45 PM

After spending time trying to debug this test failure, I had some time to reflect on my approach. I have decided that changing the interface to explicitly work on FixedVectorType would be a mistake. Many or all of these operations should be possible for scalable vectors. The fact that they don't work is an issue to be fixed, and not a feature. With that in mind, I will abandon this patch in favor of D81495 which just does the cast internally and does not change the interface.