Page MenuHomePhabricator

[MVT][SVE] Scalable vector MVTs (1/3)
ClosedPublic

Authored by huntergr on Apr 13 2017, 8:18 AM.

Details

Summary

This patch adds a few helper functions to obtain new vector
value types based on existing ones without needing to care
about whether they are scalable or not.

I've confined their use to a few common locations right now,
and targets that don't have scalable vectors should never
need to care about these.

Diff Detail

Repository
rL LLVM

Event Timeline

huntergr created this revision.Apr 13 2017, 8:18 AM
rengolin added inline comments.Apr 13 2017, 10:40 AM
include/llvm/CodeGen/ValueTypes.h
318 ↗(On Diff #95128)

I know it's for completeness, but you don't need this function, better not create it at this time.

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
include/llvm/CodeGen/ValueTypes.h
311 ↗(On Diff #95128)

assert that the size is even? Also, we don't seem to have a use for this function yet?

329 ↗(On Diff #95128)

Use unsigned for clarity and assert that its even? The asserts in LegalizeVectorTypes.cpp and SelectionDAG.cpp can then be dropped

asb added a subscriber: asb.Apr 14 2017, 7:04 AM
huntergr added inline comments.Apr 18 2017, 5:48 AM
include/llvm/CodeGen/ValueTypes.h
311 ↗(On Diff #95128)

As with the other function below, it would be used later, but I'll delay it until that patch.

318 ↗(On Diff #95128)

It'll be used later (in the AArch64 backend), but ok, will remove until that later patch.

329 ↗(On Diff #95128)

So the reason for the 'auto' is that this will be converted to use MVT::ElementCount in patch 3, so that it works with scalable vectors; you're right that I can move the assert about even numbers of elements here, though.

huntergr updated this revision to Diff 95713.Apr 19 2017, 4:50 AM

Removed unused functions, moved assert on halving the number of elements when there's an odd element count.

RKSimon accepted this revision.Apr 19 2017, 5:16 AM

LGTM with one minor

include/llvm/CodeGen/ValueTypes.h
314 ↗(On Diff #95713)

Use EltCnt directly?

This revision is now accepted and ready to land.Apr 19 2017, 5:16 AM
rengolin edited edge metadata.Apr 19 2017, 5:22 AM

Looks good to me to, but, what about tests?

Looks good to me to, but, what about tests?

This is effectively an NFC patch, as are the other two -- there's no way to test the scalable aspect of this code until a backend (AArch64) implements them. All existing code using the functions works, though.

Right, this one is ok, but the other two patches also don't have tests either, and they're certainly not NFC.

I'm not sure how to test them, though.

rengolin accepted this revision.Apr 20 2017, 3:15 AM
This revision was automatically updated to reflect the committed changes.