This is an archive of the discontinued LLVM Phabricator instance.

[ARM] MVE predicate register support
ClosedPublic

Authored by dmgreen on Jul 21 2019, 7:04 AM.

Details

Summary

This adds support code for building and shuffling i1 predicate registers. It generally uses two basic principles, either converting the predicate into an scalar (through a PREDICATE_CAST) and doing scalar operations on it there, or by converting the register to an full vector register and back.

Some of the code here is a not super efficient but will hopefully cover most cases of moving i1 vectors around and can be improved in subsequent patches.

Some code by David Sherwood.

Diff Detail

Repository
rL LLVM

Event Timeline

dmgreen created this revision.Jul 21 2019, 7:04 AM

Just a first scan, I will look more closely tomorrow.

llvm/lib/Target/ARM/ARMISelLowering.cpp
6720 ↗(On Diff #210995)

perhaps a message in this assert.

7385 ↗(On Diff #210995)

is it weird to call createNEONModImm here?

7408 ↗(On Diff #210995)

what do you mean with the "real predicate"?

7679 ↗(On Diff #210995)

do we want to check or assert that we're targeting MVE here?

7787 ↗(On Diff #210995)

Can this be a little helper function/lambda so that we don't duplicate this below for Op2?

7870 ↗(On Diff #210995)

and this (plus the switch above) looks suspiciously similar, except that the node is a ISD::EXTRACT_VECTOR_ELT here.

dmgreen marked 6 inline comments as done.Jul 22 2019, 2:31 PM
dmgreen added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
7385 ↗(On Diff #210995)

It was never renamed, and the same between Neon and MVE (it was used in the VMOVimm code too.)

I can rename it easily enough. createVModImm sound OK?

7408 ↗(On Diff #210995)

The predicate may be a v4i1 or a v8i1. We cast it to a v16i1 to get the types right, using the principle that a v4i1 of abcd and a v16i1 of aaaabbbbccccdddd are the same thing.

dmgreen updated this revision to Diff 211193.Jul 22 2019, 2:32 PM
dmgreen marked 2 inline comments as done.
SjoerdMeijer added inline comments.Jul 22 2019, 11:53 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
7398 ↗(On Diff #211193)

I can rename it easily enough. createVModImm sound OK?

Yep, sounds good

7666 ↗(On Diff #211193)

Perhaps better to hide magic constant 16 in a MVE subtarget function or something like that?

7408 ↗(On Diff #210995)

Ah okay, cheers, got it.
Perhaps you just want to add this to the comments instead of the "If the real predicate is not v16i1 ".

samparker added inline comments.Jul 23 2019, 3:54 AM
llvm/test/CodeGen/Thumb2/mve-pred-build-var.ll
188 ↗(On Diff #211193)

Why aren't these just one: bfi r2, r0, #0, #16 or uxth r2, r0?

dmgreen marked 7 inline comments as done.Jul 23 2019, 4:09 AM
dmgreen added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
7398 ↗(On Diff #211193)

It ended up as createVMOVModImm in rL366790.

7666 ↗(On Diff #211193)

I changed this around a little to use the scalar size.

llvm/test/CodeGen/Thumb2/mve-pred-build-var.ll
188 ↗(On Diff #211193)

Thats "FIXME: Handle splats of the same value better." Maybe just a conditional mov of -1 (or something)? It's on the list of things to do.

dmgreen updated this revision to Diff 211274.Jul 23 2019, 4:12 AM
dmgreen marked an inline comment as done.

Updates and rebase

SjoerdMeijer accepted this revision.Jul 23 2019, 5:16 AM

This all looks very reasonable to me. I am happy with this if @samparker is happy too.

This revision is now accepted and ready to land.Jul 23 2019, 5:16 AM
This revision was automatically updated to reflect the committed changes.