This is an archive of the discontinued LLVM Phabricator instance.

[ARM][MVE] Enable masked gathers from vector of pointers
ClosedPublic

Authored by anwel on Dec 20 2019, 1:27 AM.

Details

Summary

Adds a pass to the ARM backend that takes a v4i32 gather and transforms it into a call of arm's masked gather intrinsic.

There are limited legal addressing forms available for MVE; whether a gather is legal depends on the address as well as the gather itself. This makes legalising in ISel difficult. We thus handle this in a FunctionPass, creating intrinsics. This allows us to deal with the addressing modes and at the same time optimise the code to use more efficient instructions where possible.

Diff Detail

Event Timeline

anwel created this revision.Dec 20 2019, 1:27 AM
simoll added a subscriber: simoll.Dec 20 2019, 1:35 AM
dmgreen added inline comments.Dec 20 2019, 2:09 AM
llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
547

Alignment >= 4

552

We should add the "ST->hasMVEIntegerOps()" check here too.

568

I think it would be useful to add debug comments for what is happening, which will become more useful as we add more variants in here. Especially around why and where we have decided not to transform.

598

You can drop the brackets if there is only a single statement in the if block.

dmgreen added inline comments.Dec 20 2019, 2:47 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
610

Also, can you add some documentation for the ModifiedDT bool. It is a little bit of a odd name, but it essentially means "we might have modified anything else, not just the intrinsic, please start again."

anwel updated this revision to Diff 234849.Dec 20 2019, 2:50 AM
anwel retitled this revision from [ARM][MVE] Enable masked gathers from vector of pointers to [TTI][ARM][MVE] Enable masked gathers from vector of pointers.
anwel edited the summary of this revision. (Show Details)

Adopt the changes suggested in comments and give a more detailed summary of the patch and the motivation to do it this way.

anwel marked 5 inline comments as done.Dec 20 2019, 2:52 AM
anwel added inline comments.
llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
568

Yes, indeed. I have added some debug information.

RKSimon added inline comments.Dec 21 2019, 10:04 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
611

Its unusual to put transformational methods inside TargetTransformInfo - that tends to be legality/cost checks, not IR transforms.

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
535

isa<VectorType>(Ty)

dmgreen added inline comments.Dec 23 2019, 8:24 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
611

Oh yeah. We were thinking of the interleaved load lowering that we do, but now that I look again that happens in TLI. I had not realized.

Because of the amount of code this might turn into (and how long TLI is already), it might be better as a separate pass though.

We will look into this after the hols. Thanks for the comments.

anwel marked 6 inline comments as done.Jan 6 2020, 5:23 AM
anwel added inline comments.
llvm/include/llvm/Analysis/TargetTransformInfo.h
611

You are right, putting the transforming code into a pass looks a lot cleaner and should also make future additions easier and more localized. I'll update the diff with a new version.

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
535

Yes :)

anwel updated this revision to Diff 236330.Jan 6 2020, 5:31 AM
anwel marked 2 inline comments as done.
anwel retitled this revision from [TTI][ARM][MVE] Enable masked gathers from vector of pointers to [ARM][MVE] Enable masked gathers from vector of pointers.
anwel edited the summary of this revision. (Show Details)

Moved the transformation of the gather from ARMTargetTransformInfo to a separate FunctionPass.

Nice, thanks!

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
1 ↗(On Diff #236330)

This looks a touch too short, unless my eyes deceive me.

15 ↗(On Diff #236330)

It's customary in llvm to put std headers below the llvm headers.

86 ↗(On Diff #236330)

Any function that is only used in this file can be static.

178 ↗(On Diff #236330)

This isn't used it seems.

anwel marked 7 inline comments as done.Jan 6 2020, 6:41 AM
anwel added inline comments.
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
1 ↗(On Diff #236330)

Yes, you're right

15 ↗(On Diff #236330)

Okay in that case I'll of course follow the standard format

86 ↗(On Diff #236330)

Do you mean private?

178 ↗(On Diff #236330)

No, sorry. I'll remove it.

anwel updated this revision to Diff 236352.Jan 6 2020, 6:42 AM
anwel marked 3 inline comments as done.
dmgreen added inline comments.Jan 6 2020, 6:49 AM
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
86 ↗(On Diff #236330)

This is global scope I think? So the function can be static. Same for LowerGather below.

anwel updated this revision to Diff 236363.Jan 6 2020, 7:34 AM
anwel marked 3 inline comments as done.
dmgreen accepted this revision.Jan 7 2020, 3:18 AM

Thanks. LGTM, if no-one else has any other comments.

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
532

Maybe expand this a little to "For MVE, we have a custom lowering pass that will already have custom legalised any gathers that we can to mve intrinsics, and want to expand all the rest. The pass runs before the masked intrinsic lowering pass, so if we are here, we know we want to expand."

This revision is now accepted and ready to land.Jan 7 2020, 3:18 AM
SjoerdMeijer added a comment.EditedJan 7 2020, 3:43 AM

looks good to me too, other than the irrelevant nits, I have one question though, see inlined.

llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
527

nit: should it be called "at" or "in" 2 places?

532

nit: mve -> MVE

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
86 ↗(On Diff #236363)

nit: indentation

96 ↗(On Diff #236363)

nit: mve -> MVE

108 ↗(On Diff #236363)

nit: are we checking types too? Just omit "or vector type"?

117 ↗(On Diff #236363)

is same -> is the same

147 ↗(On Diff #236363)

I might be wrong, but it looks like Load is always set here? Do we need to check it here?

153 ↗(On Diff #236363)

and this is never reached? Which makes sense probably, because we should be able to lower it?

anwel updated this revision to Diff 236554.Jan 7 2020, 5:08 AM
anwel marked 11 inline comments as done.

Changed the comment in ARMTTIImpl::isLegalMaskedGather to give a better description, and addressed the nits

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
108 ↗(On Diff #236363)

Yes, we are actually checking both, so I changed the function name to reflect that.

147 ↗(On Diff #236363)

You are right, the checks for Load are superfluous

86 ↗(On Diff #236330)

Ah, ok.

SjoerdMeijer added a comment.EditedJan 7 2020, 6:55 AM

A last question/request, I guess we need 2 more tests? For completeness probably best to have a test with -enable-arm-maskedgatscat=false and another one with -mattr=-mve?

anwel updated this revision to Diff 236594.Jan 7 2020, 8:10 AM

Yes, that's a good idea. Better safe then sorry. I added a test that makes sure that the gathers are not constructed if the mve or enable-arm-maskedgatscat option is not set

Thanks, LGTM again

This revision was automatically updated to reflect the committed changes.