This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Fixed-length vector MVT ranges
ClosedPublic

Authored by huntergr on Aug 16 2019, 4:03 AM.

Details

Summary
  • New range functions in MachineValueType.h to only iterate over the fixed-length int/fp vector types
  • Stopped backends which don't support scalable vector types from iterating over scalable types.

Diff Detail

Event Timeline

huntergr created this revision.Aug 16 2019, 4:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 4:03 AM

Split out from D53137.

I added new range functions for fp and int fixedlen types, but not all fixedlen vector valuetypes; the fixedlen MVTs are not contiguous.

If we want to provide that, I could reorder the MVT list to group all fixedlen types together (and the same for all scalable types), but we then lose direct lists of all fp/int types. The concat or filter iterators from STLExtras could be used to fix those if needed. Any preferences?

Split out from D53137.

Thanks!

I added new range functions for fp and int fixedlen types, but not all fixedlen vector valuetypes; the fixedlen MVTs are not contiguous.

If we want to provide that, I could reorder the MVT list to group all fixedlen types together (and the same for all scalable types), but we then lose direct lists of all fp/int types. The concat or filter iterators from STLExtras could be used to fix those if needed. Any preferences?

I like your suggestion of using concat_iterator(_range), but I think grouping all fixed-width types together is sufficient for now. If there is ever a good reason to iterate through (all combined scalable and fixed-width fp vector types)or (all combined scalable and fixed-width integer vector types), we can always introduce the extra concat_iterators then.

greened added a comment.EditedAug 16 2019, 10:20 AM

I like your suggestion of using concat_iterator(_range), but I think grouping all fixed-width types together is sufficient for now. If there is ever a good reason to iterate through (all combined scalable and fixed-width fp vector types)or (all combined scalable and fixed-width integer vector types), we can always introduce the extra concat_iterators then.

I'm not sure what you're suggesting here. Are you asking Graham to keep all the fixed-width types contiguous (i.e. int and fp)? This patch does not do that. I don't have a strong preference either way. Given that the existing state is non-contiguous I see no compelling reason to change it.

greened added inline comments.Aug 16 2019, 10:24 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
312

Should we replace this with a concat/filter iterator on the vector value types in the future? Not suggesting this needs to be done now. Obviously this would apply to the many similar patterns below.

I like your suggestion of using concat_iterator(_range), but I think grouping all fixed-width types together is sufficient for now. If there is ever a good reason to iterate through (all combined scalable and fixed-width fp vector types)or (all combined scalable and fixed-width integer vector types), we can always introduce the extra concat_iterators then.

I'm not sure what you're suggesting here. Are you asking Graham to keep all the fixed-width types contiguous (i.e. int and fp)? This patch does not do that. I don't have a strong preference either way. Given that the existing state is non-contiguous I see no compelling reason to change it.

I was suggesting to change the order of the MVTs to keep all the fixed-width types contiguous (int and fp), as a lighter weight alternative to introducing the filter/concat iterators, since this would probably suffice for most use-cases. We can always introduce the concat iterators later if needed.

I was suggesting to change the order of the MVTs to keep all the fixed-width types contiguous (int and fp), as a lighter weight alternative to introducing the filter/concat iterators, since this would probably suffice for most use-cases. We can always introduce the concat iterators later if needed.

Got it. That sounds fine to me.

huntergr updated this revision to Diff 216415.Aug 21 2019, 9:01 AM
  • Reordered MVTs to group fixed-length types together (and the same for scalable).
  • Replaced existing range accessors to remove explicit isScalableVector checks on MVTs in various backends.
greened added inline comments.Sep 3 2019, 7:00 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
899

This is confusing to me. It doesn't do what the comment says (and maybe that comment should have been updated earlier) but it also seems like this could do this:

setLoadExtAction(ISD::EXTLOAD, v4i8, v4i16, Legal);

That seems wrong. What am I missing?

I know this patch didn't introduce this.

Since I don't see anything strange that was introduced by this patch, LGTM.

greened accepted this revision.Sep 3 2019, 7:02 AM
This revision is now accepted and ready to land.Sep 3 2019, 7:02 AM
This revision was automatically updated to reflect the committed changes.