Page MenuHomePhabricator

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

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

Details

Summary

Adds MVT::ElementCount to represent the length of a
vector which may be scalable, then adds helper functions
that work with it.

Diff Detail

Repository
rL LLVM

Event Timeline

huntergr created this revision.Apr 13 2017, 8:24 AM
rengolin added inline comments.Apr 13 2017, 10:49 AM
include/llvm/CodeGen/MachineValueType.h
607 ↗(On Diff #95133)

Can't you just return EC here?

include/llvm/CodeGen/ValueTypes.h
77 ↗(On Diff #95133)

Add the comment inside the assert, like:

assert(!IsScalable && "we don't support extended scalable types yet");
97 ↗(On Diff #95133)

ditto

157 ↗(On Diff #95133)

I don't think this check is necessary, and may bite you later if you forget.

Either make it an assert, like the others, or just let the comparison get inlined.

284 ↗(On Diff #95133)

ditto

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
include/llvm/CodeGen/MachineValueType.h
241 ↗(On Diff #95133)

Why uint64_t and not unsigned like the other existing vector element count uses?

huntergr added inline comments.Apr 18 2017, 6:02 AM
include/llvm/CodeGen/MachineValueType.h
241 ↗(On Diff #95133)

It was mostly to match the equivalent for IR -- VectorType::getNumElements() returns a uint64_t now (since NumElements was moved to SequentialType). I guess it doesn't need to be the same for CodeGen.

607 ↗(On Diff #95133)

There isn't a defined EC variable for an MVT; it just contains a single SimpleValueType. Each MVT used would be noticeably larger (and increasing from 8bits to 16bits may already cause some problems, see comments on patch 2) if it included an ElementCount instance, so we just construct them on the fly based on the enum value of SimpleTy.

include/llvm/CodeGen/ValueTypes.h
77 ↗(On Diff #95133)

Ok, will do.

157 ↗(On Diff #95133)

So I would have preferred to use an assert, but this is called by existing code on extended types because the asserts query it.

This check and the asserts can all be removed once the IR type is in place.

rengolin added inline comments.Apr 18 2017, 6:21 AM
include/llvm/CodeGen/MachineValueType.h
607 ↗(On Diff #95133)

Right, makes sense.

include/llvm/CodeGen/ValueTypes.h
157 ↗(On Diff #95133)

I see.

Asserts for unsupported things are known to be ephemeral, but this check isn't. Adding a FIXME with that rationale would make it so, for now.

huntergr updated this revision to Diff 95717.Apr 19 2017, 4:59 AM

Changed 'Min' to unsigned.
Moved text to asserts instead of comments.
Added FIXME to explain why extended scalable vector types aren't handled yet.

rengolin edited edge metadata.Apr 19 2017, 5:54 AM

Right, the code looks good now, but I'm worried about tests... We don't seem to have unit tests on this area and I'm not sure how we'd create one (not terribly familiar here).

We could wait to have IR tests, like the rest, when AArch64 uses it... If everyone agrees with it.

Right, the code looks good now, but I'm worried about tests... We don't seem to have unit tests on this area and I'm not sure how we'd create one (not terribly familiar here).

We could wait to have IR tests, like the rest, when AArch64 uses it... If everyone agrees with it.

It looks like I may be able to test it via C++ code (in llvm/unittests/CodeGen) instead of the usual .ll/.mir/.s tests. I'll try that and see how easy it is.

huntergr updated this revision to Diff 95916.Apr 20 2017, 3:08 AM

Added unit tests via gtest framework.
Changed getHalfNumVectorElementsVT to use 'EltCnt.Min' when determining whether the current vector length is evenly divisible.
Changed iterator names to be more obvious.

rengolin accepted this revision.Apr 20 2017, 3:14 AM

Nice! Thanks Graham! This looks good to me, now.

This revision is now accepted and ready to land.Apr 20 2017, 3:14 AM
This revision was automatically updated to reflect the committed changes.