This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Implement masked load intrinsics
ClosedPublic

Authored by kmclaughlin on Oct 11 2019, 10:04 AM.

Details

Summary

Adds support for codegen of masked loads, with non-extending,
zero-extending and sign-extending variants.

Diff Detail

Event Timeline

kmclaughlin created this revision.Oct 11 2019, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 10:04 AM

Sam has been looking at extending masked loads and stores in D68337 and related patches. There looks like there would be some overlap with this, especially in the target independent parts. Make sure you co-ordinate with him.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10453

I'm not convinced that just because a sext load is legal and a masked load is legal, that a sext masked load is always legal.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1077

What if the passthru isn't undef?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
151

This can handle all masked loads? Of any type, extended into any other type, with any alignment?

153

This patch doesn't handle stores yet.

llvm/lib/Target/AArch64/SVEInstrFormats.td
296

Can this just use "undef"?

kmclaughlin edited the summary of this revision. (Show Details)
  • Rebased patch, removed extra sext & zext combine from DAGCombine which are no longer necessary
  • Added isVectorLoadExtDesirable to AArch64ISelLowering
  • Added more checks to isLegalMaskedLoad
  • Changed SVEUndef to SVEDup0Undef, handling undef or all zeros
  • Changed SelectionDAG::getConstant to return SPLAT_VECTOR instead of BUILD_VECTOR for scalable types
kmclaughlin marked 4 inline comments as done.Oct 21 2019, 9:24 AM

Thanks for reviewing this, @dmgreen! I have updated the patch to make use of the changes to DAGCombine introduced by D68337.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10453

Removed, as this patch can also use tryToFoldExtOfMaskedLoad for sext & zext masked loads.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
151

I have added checks on types we can handle here

153

Removed!

Thanks!

It looks like the only supported parameter of the PassThru here is a splat of 0 or undef. This might get in the way of IR level optimisation that could try to producing a masked load with different passthru, which would then fail to select. The ARM backed added a custom lowering to turn this case into a select of the loaded value and the passthru. Like this, which was further modified in later commits:
https://github.com/llvm/llvm-project/commit/b325c057322ce14b5c561d8ac49508adab7649e5#diff-b853ecd5d5e0f73c2c093ffb5b4f2fbbR8855
That way you only have to check on the zero, not undef. I'm not sure if there is support yet for vector selects in the SVE codegen?

I was also expecting something that says "setOperationAction(ISD::MLOAD, VT, Legal)" somewhere, but I guess that's already the default?

llvm/lib/Target/AArch64/SVEInstrFormats.td
4753

Can you explain why is this pseudo is needed, exactly? I feel that using pseudos is often the wrong solution to a problem (it may be required here, I'm just sure why exactly).

We currently seem to generate ld1b (for example), over ldnf1b. Is there ever a time that we expect to generate a nf load?

  • Removed unnecessary pseudo from SVEInstrFormats.td
kmclaughlin marked an inline comment as done.Oct 23 2019, 5:52 AM

I'm not sure if there is support yet for vector selects in the SVE codegen?

There is not yet support for vector selects, so for this patch the intention was that any passthru which is not all zero or undef would result in a selection failure.
Do you think it would acceptable to handle different passthrus in a future patch which also implements vector selects for SVE?

llvm/lib/Target/AArch64/SVEInstrFormats.td
4753

The pseudo was a workaround that was added downstream for non-faulting loads, but it is not needed here.

I was also expecting something that says "setOperationAction(ISD::MLOAD, VT, Legal)" somewhere, but I guess that's already the default?

It is; we will need to set the operation action for supported VTs to 'Custom' once we have codegen for vselects in order to handle any passthru values which aren't undef or zero (which you've already noticed as missing in the review), but the operation itself is legal once it gets through type legalization.

There's a separate mechanism to handle illegal masked loads/stores in the ScalarizeMaskedMemIntrin pass before lowering to a SelectionDAG. Scalarization needs to create new basic blocks for a loop over the vector elements, which isn't easy after lowering. The TargetTransformInfo::isLegalMaskedLoad (or Store) functions are what determine whether scalarization takes place or not for a given target.

dmgreen accepted this revision.Oct 23 2019, 6:44 AM

There is not yet support for vector selects, so for this patch the intention was that any passthru which is not all zero or undef would result in a selection failure.
Do you think it would acceptable to handle different passthrus in a future patch which also implements vector selects for SVE?

Yeah OK. That makes sense. I remember the same chicken and egg from MVE. We ended up doing them the other way around there. Can you try and add a FIXME comment somewhere?

Otherwise LGTM.

llvm/lib/Target/AArch64/SVEInstrFormats.td
4753

Righteo. We know where to find it if it turns out we do need them.

This revision is now accepted and ready to land.Oct 23 2019, 6:44 AM
This revision was automatically updated to reflect the committed changes.
This comment was removed by Yi-Hong.Lyu.