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
544

Alignment >= 4

549

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

565

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.

595

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

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
565

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

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

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

isa<VectorType>(Ty)

dmgreen added inline comments.Dec 23 2019, 8:24 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
611 ↗(On Diff #234849)

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

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
532

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

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

15

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

86

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

178

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

Yes, you're right

15

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

86

Do you mean private?

178

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

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
529

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
524

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

529

nit: mve -> MVE

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
87

nit: indentation

97

nit: mve -> MVE

109

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

118

is same -> is the same

148

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

154

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
86

Ah, ok.

109

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

148

You are right, the checks for Load are superfluous

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.