Page MenuHomePhabricator

[SVE] Remove calls to VectorType::getNumElements from CodeGen
ClosedPublic

Authored by ctetreau on Jun 19 2020, 10:14 AM.

Diff Detail

Event Timeline

ctetreau created this revision.Jun 19 2020, 10:14 AM
Herald added a project: Restricted Project. · View Herald Transcript
RKSimon accepted this revision.Jul 1 2020, 1:24 AM

LGTM with one minor

llvm/lib/CodeGen/ScalarizeMaskedMemIntrin.cpp
522–523

I know you're being cautious about changes but I think this assertion can use isa<FixedVectorType>? Or drop the assert entirely - cast<> implicitly asserts for the correct type.

This revision is now accepted and ready to land.Jul 1 2020, 1:24 AM
ctetreau updated this revision to Diff 274848.Jul 1 2020, 9:39 AM

address code review issues

@sdesmalen Would you like me to hold off on this one until after the branch?

@sdesmalen Would you like me to hold off on this one until after the branch?

Thanks for checking @ctetreau. I actually think most of the changes are fine to go in now, with the exception of:

  • the cast to FixedVectorType in llvm/lib/CodeGen/ValueTypes.cpp.
  • the changes to InterleavedAccessPass, because this pass is not yet guarded by bailing out early.

The other changes are fine because:

  • The ACLE uses custom target-specific intrinsics instead of llvm.masked.load/store/gather/scatter intrinsics.
  • The ACLE uses custom target-specific intrinsics instead of the llvm.experimental.reduction intrinsics.
  • GlobalISel already falls back to DAGISel if the function uses scalable vectors (D81557 and D82524)
  • The InterleavedLoadCombinePass was guarded in D79700 to bail out early for scalable vectors.
  • shouldConvertSplatType returns nullptr for AArch64 so the change in CodeGenPrepare::optimizeShuffleVectorInst is never hit.

If you can add a check to bail out early for the InterleavedAccessPass for scalable vectors and move the change to EVT::getExtendedVectorNumElements into a separate patch, then I'm happy with most of this to go in now.

ctetreau updated this revision to Diff 276506.Wed, Jul 8, 11:51 AM

address code review issues

@sdesmalen I went ahead and made the requested changes. In ValueType.cpp, rather than leaving the code alone, I went ahead and returned getElementCount().Min along with adding a warning if the vector was scalable. This way getNumElement() can be removed in the future without having to touch this file again.

ctetreau updated this revision to Diff 276810.Thu, Jul 9, 12:43 PM

account for abandoned D82329

This revision was automatically updated to reflect the committed changes.