This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Extend findMoreOptimalIndexType so BUILD_VECTORs do not force 64bit indices.
ClosedPublic

Authored by paulwalker-arm on Jul 25 2022, 4:54 PM.

Details

Summary

Extends findMoreOptimalIndexType to allow ISD::BUILD_VECTOR based
indices to be truncated when such truncation is lossless. This can
enable the use of 32bit gather/scatter indices thus making it less
likely to have to split a gather/scatter in two.

Depends on D125194

Diff Detail

Event Timeline

paulwalker-arm created this revision.Jul 25 2022, 4:54 PM
Herald added a project: Restricted Project. · View Herald Transcript
paulwalker-arm requested review of this revision.Jul 25 2022, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 4:54 PM

Fixed typo in comment.

Matt added a subscriber: Matt.Jul 26 2022, 12:36 PM

The patch looks very sensible to me and should be a performance improvement! I just had a couple of minor comments ...

llvm/include/llvm/CodeGen/SelectionDAGNodes.h
122

I think it's worth adding a comment here saying that this currently only works for BUILD_VECTOR nodes. The name of the function is quite generic, so I think users would probably expect it to also work for things like SPLAT_VECTOR, STEP_VECTOR, etc.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
312

I assume that only one of these cases is tested, right? Either Signed=false or Signed=true, but not both. Is it possible to make sure we have tests for both? If it's too much trouble then don't worry, but I thought maybe it's possible to write a unit test?

paulwalker-arm added inline comments.Aug 15 2022, 7:38 AM
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
122

How strongly do you feel about this @david-arm? Personally I see that as an implementation detail rather than something linked to the interface. It is my hope that this function will be expanded whenever somebody finds a new use case, so there's nothing about this function that is intrinsically linked to BUILD_VECTOR.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
312

I've tried and failed to exercise this condition. I believe it's not possible with the current usage because isVectorShrinkable is only called for i64 based vector types. The problem is that gathers and scatters only make use of unsigned offsets when the offsets are explicitly zero extended from something that is smaller than i64 and thus by the time that transformation happens no i64 based BUILD_VECTORs exist. We don't have this problem for signed offsets because that is the default type for PTR sized offsets.

david-arm accepted this revision.Aug 15 2022, 7:53 AM

LGTM! Thanks for looking into writing those test cases. The code itself looks sound.

This revision is now accepted and ready to land.Aug 15 2022, 7:53 AM
This revision was landed with ongoing or failed builds.Aug 18 2022, 10:03 AM
This revision was automatically updated to reflect the committed changes.