This is an archive of the discontinued LLVM Phabricator instance.

[ARM][MVE] Enable tail predication for loops containing MVE gather/scatters
ClosedPublic

Authored by anwel on Aug 3 2020, 8:39 AM.

Details

Summary

Currently, MVE tail predication will only tail predicate a loop if there only are predicated loads and stores present. This patch widens the scope to also accept gathers and scatters, such that loops that are auto-vectorized with the option -enable-arm-maskedgatscat (and actually end up containing an MVE gather or scatter) can be tail predicated.

Diff Detail

Event Timeline

anwel created this revision.Aug 3 2020, 8:39 AM
anwel requested review of this revision.Aug 3 2020, 8:39 AM

I was looking at this pass for something else recently. I'm not sure if the legality checks are really necessary in here. We might want to change it to just check for active lane masks and convert them to vctp's (when legal). I think that will always be better in terms of code quality than the expansion of the active lane mask, no matter if we end up transforming to a tail predicated loop or not.

That's not really relevant for this patch though, which looks OK as far as I can tell.

llvm/lib/Target/ARM/MVETailPredication.cpp
155–156

This comment can be removed now.

236–249

Can this entire function use:

  • The return type of a load/gather
  • The value type of a store/scatter

?

242

I don't think this pass should ever see valid Intrinsic::masked_scatter's that have not been transformed into some form of Intrinsic::arm_mve_vstr_...

samparker added inline comments.Aug 4 2020, 5:32 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.h
860

Like Dave also said, do we really need the generic IR opcodes in these helpers?

anwel updated this revision to Diff 283516.Aug 6 2020, 12:57 AM
anwel marked 4 inline comments as done.

Changed getVectorType.

llvm/lib/Target/ARM/ARMBaseInstrInfo.h
860

In the way this function is intended to work, no we don't. Checking for the generic IR opcodes in the case the function is used for here might seem a bit overkill, but not harmful to the intentions of the check (please do correct me if I'm mistaken).
It is however only a re-use of the function, which was originally intended for a use in MVEGatherScatterLowering, for checking whether all users of a getelementptr instruction are gathers or scatters. Knowing this allows for certain transformations based on assumptions that can be made about such addresses. In that case it is important to include the generic ones, because at the time the check is made not all gathers and scatters have been transformed yet.
I was trying to avoid code duplication with this, but if your advice is that it would be better to separate the functionality more clearly, I'm happy to introduce an isMVEGather etc

llvm/lib/Target/ARM/MVETailPredication.cpp
236–249

Yes, if I see it correctly, it can. I changed it to do that.

samparker added inline comments.Aug 6 2020, 2:04 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.h
860

Okay, sounds fair enough to me.

dmgreen accepted this revision.Aug 6 2020, 6:46 AM

Sounds good to me then, thanks.

This revision is now accepted and ready to land.Aug 6 2020, 6:46 AM