Page MenuHomePhabricator

[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

Repository
rL LLVM

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 ↗(On Diff #215558)

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.Tue, Sep 3, 7:00 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
899 ↗(On Diff #216415)

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.Tue, Sep 3, 7:02 AM
This revision is now accepted and ready to land.Tue, Sep 3, 7:02 AM
This revision was automatically updated to reflect the committed changes.