Page MenuHomePhabricator

[IntrinsicEmitter] Add overloaded types for SVE intrinsics (Subdivide2 & Subdivide4)
ClosedPublic

Authored by kmclaughlin on Sep 13 2019, 6:27 AM.

Details

Summary

Both match the type of another intrinsic parameter of a vector type, but where each element is subdivided to form a vector with more elements of a smaller type.

Subdivide2Argument allows intrinsics such as the following to be defined:

  • declare <vscale x 4 x i32> @llvm.something.nxv4i32(<vscale x 8 x i16>)

Subdivide4Argument allows intrinsics such as:

  • declare <vscale x 4 x i32> @llvm.something.nxv4i32(<vscale x 16 x i8>)

Tests are included in follow up patches which add intrinsics using these types.

Diff Detail

Repository
rL LLVM

Event Timeline

kmclaughlin created this revision.Sep 13 2019, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 6:27 AM
rovka added a subscriber: rovka.Sep 16 2019, 9:53 AM
rovka added inline comments.
include/llvm/IR/DerivedTypes.h
515 ↗(On Diff #220087)

Why not move this logic to getTruncatedElementVectorType and drop getNarrowerFpElementVectorType altogether?

include/llvm/IR/Intrinsics.h
130 ↗(On Diff #220087)

Not directly related to your change: Is there a reason why VecOfAnyPtrsToElt isn't in this assert? If not, maybe this is a good time to add a First/LastArgumentKind and just check that Kind is in the range.

lib/IR/Function.cpp
989 ↗(On Diff #220087)

Maybe replace this with an assert and drop the unreachable.

997 ↗(On Diff #220087)

Ditto,

1312 ↗(On Diff #220087)

Can you share this code? Either a small helper or a fallthrough with a repeat check on D.Kind to get the number of subdivisions would work.

sdesmalen added inline comments.Sep 19 2019, 12:35 AM
include/llvm/IR/DerivedTypes.h
500 ↗(On Diff #220087)

nit: Use llvm_unreachable("message") instead of assert(0 && "message")

kmclaughlin marked 5 inline comments as done.
kmclaughlin added a reviewer: rovka.
  • Moved getNarrowerFpElementVectorType logic into getTruncatedElementVectorType
  • Shared code which handles Subdivide2Argument and Subdivide4Argument where possible
  • Replaced an unreachable with an assert in Function.cpp
  • Replaced an assert with an unreachable in DerivedTypes.h

Thanks for reviewing this patch, @rovka and @sdesmalen!

include/llvm/IR/Intrinsics.h
130 ↗(On Diff #220087)

I think it's probably best not to include VecOfAnyPtrsToElt here as it was removed from the assert when the functions getOverloadArgNumber and getRefArgNumber below were added (these should be used instead of getArgumentNumber in this case).

lib/IR/Function.cpp
1312 ↗(On Diff #220087)

This code is now shared, as is the code to handle Subdivide2Argument & Subdivide4Argument in Function.cpp above.

Thanks for the changes @kmclaughlin! Just a few more nits from me, but looks good otherwise.

include/llvm/IR/DerivedTypes.h
493 ↗(On Diff #220845)

nit: I don't think the break is needed because of the llvm_unreachable

lib/IR/Function.cpp
992 ↗(On Diff #220845)

nit: you can simplify this by:

int SubDiv = D.Kind == IITDescriptor::Subdivide2Argument ? 1 : 2;
return VectorType::getSubdividedVectorType(VTy, SubDiv);
1302 ↗(On Diff #220845)

nit: you can use auto *VTy here since the dyn_cast<VectorType> makes it clear that the return type is VectorType.

1303 ↗(On Diff #220845)

nit: can be simplified by:

int SubDiv = D.Kind == IITDescriptor::Subdivide2Argument ? 1 : 2;
NewTy = VectorType::getSubdividedVectorType(VTy, SubDiv);
1310 ↗(On Diff #220845)

nit: Move this return statement into the if(VectorType *VTy = dyn_cast<VectorType>(NewTy)) block?

  • Some minor changes, including removing an unnecessary break
  • Simplified checks of D.Kind in Function.cpp to determine if it is a Subdivide2Argument or Subdivide4Argument
sdesmalen accepted this revision.Sep 20 2019, 12:50 AM

Thanks, LGTM!

This revision is now accepted and ready to land.Sep 20 2019, 12:50 AM
This revision was automatically updated to reflect the committed changes.