This is an archive of the discontinued LLVM Phabricator instance.

[IR] Change CreateStepVector to work with element types smaller than i8
ClosedPublic

Authored by david-arm on Nov 12 2021, 6:27 AM.

Details

Summary

Currently the stepvector intrinsic only supports element types that
are integers of size 8 bits or more. This patch adds support for the
creation of stepvectors with smaller element types by creating
the intrinsic with i8 elements that we then truncate to the requested
size.

It's not currently possible to write a vectoriser test to exercise
this code path so I have added a unit test here:

llvm/unittests/IR/IRBuilderTest.cpp

Diff Detail

Event Timeline

david-arm created this revision.Nov 12 2021, 6:27 AM
david-arm requested review of this revision.Nov 12 2021, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2021, 6:27 AM

Where does the restriction on the element type of stepvector come from? I mean, I see the code in the verifier, but I'm not sure why it's there...

When first introduced, the stepvector intrinsic defined wrapping as undefined behaviour. At the time we pointed out this effectively made its usage for i1 vectors practically useless because only the first two lanes of a scalable vector could ever have a defined value. This was discussed during an SVE sync call and there wasn't agreement to define the wrapping behaviour so as a compromised the supported element type was restricted to i8 or bigger.

Since then, ISD::STEP_VECTOR has changed so its wrapping behaviour is now defined and thus I see little reason for the intrinsic's definition to be different from the ISD node, however. We cannot just change the definition because there's code generation work required to ensure the new combinations work. This is something we'll fix but in the mean time there exists critical clang failures that result from (currently) invalid stepvector intrinsics being created and so we'd like to fix those issues first as a priority by modelling the necessary functionality whilst maintaining the current definition.

paulwalker-arm accepted this revision.Nov 16 2021, 5:18 AM
This revision is now accepted and ready to land.Nov 16 2021, 5:18 AM

Sure, makes sense.

Maybe leave a comment in the code noting that you expect this special case to be temporary.

Matt added a subscriber: Matt.Nov 16 2021, 4:13 PM
This revision was landed with ongoing or failed builds.Nov 17 2021, 2:48 AM
This revision was automatically updated to reflect the committed changes.

HI all, thanks for the review comments! I've added a TODO to CreateStepVector explaining this is temporary.