Page MenuHomePhabricator

[AArch64][SVE] Implement extractelement of i1 vectors.
ClosedPublic

Authored by efriedma on Sep 14 2020, 4:18 PM.

Details

Summary

The implementation just extends the vector to a larger element type, and extracts from that. Not fancy, but generates reasonable code.

While I'm here, fix warning from computeKnown bits.

Diff Detail

Event Timeline

efriedma created this revision.Sep 14 2020, 4:18 PM
efriedma requested review of this revision.Sep 14 2020, 4:18 PM
paulwalker-arm added inline comments.Sep 15 2020, 3:16 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9050–9056

This is a common route when expanding i1 based operations so downstream we did it in common code, for example:

setOperationAction(ISD::EXTRACT_VECTOR_ELT, MVT::nxv2i1, Promote);
AddPromotedToType(ISD::EXTRACT_VECTOR_ELT, MVT::nxv2i1, MVT::nxv2i64);

and updated SelectionDAGLegalize::PromoteNode accordingly. Is this route something we can do upstream? instead of having custom lowering.

efriedma added inline comments.Sep 15 2020, 2:19 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9050–9056

The problem is that "promote" is currently interpreted to mean "bitcast the vector operands" by vector legalization, not integer promotion, so it would be confusing to use it like this. Maybe worth messing with LegalizeAction to distingush between the two kinds of "promotion", though.

paulwalker-arm added inline comments.Sep 16 2020, 2:55 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9050–9056

Within AArch64ISelLowering.cpp I can see:

setOperationAction(ISD::FADD, MVT::v4f16, Promote);
AddPromotedToType(ISD::FADD, MVT::v4f16, MVT::v4f32);

With SelectionDAGLegalize::PromoteNode applying the appropriate extension. I can see similar usages like this for other targets. To me it looks like "Promote" can mean promotion or bitcast, with nodes having support for whichever has been required in the past. Based on this it seems safe to extend PromoteNode's handling of EXTRACT_VECTOR_ELT to include real promotion.

efriedma updated this revision to Diff 299476.Oct 20 2020, 2:36 PM
efriedma edited the summary of this revision. (Show Details)

Rebase and clang-format.

I looked into handling this in LegalizeVectorOps, but that doesn't really work out: we actually legalize EXTRACT_VECTOR_ELT nodes in LegalizeDAG. And I don't want to do this in LegalizeDAG: LegalizeDAG can't handle arbitrary illegal vector operations.

There might be some way to sort out this mess, but it isn't trivial.

Perhaps I'm missing something but I've created D89950 to show what I mentioned in my previous comment. To me it's preferable to have this as target independent code as it seems a common enough solution. I know it results in "Pomote" having multiple meanings but that boat has sailed because different nodes have already chosen different meanings and I think it's pretty clear from the old and new VTs what's being asked for.

I'm worried the usage of ANY_EXTEND and TRUNCATE in D89950 is going to cause a mess in the future. LegalizeDAG doesn't know how to legalize most vector operations; LegalizeVectorOps handles them. If someone tries to use "Promote" on a target where those operations aren't legal, it'll break. And then someone will try to "fix" LegalizeDAG, and cause other problems.

Note this is specific to the vector operations we legalize in LegalizeDAG; so essentially, INSERT_VECTOR_ELT, EXTRACT_VECTOR_ELT, and VECTOR_SHUFFLE.

paulwalker-arm accepted this revision.Oct 23 2020, 4:31 AM

I just don't buy this argument. You're saying that we must force all target's to perform custom lowering for promotable illegal operations on i1 vectors because somebody in the future might try to "fix" LegalizeDAG when they should implement custom lowering for extend/truncate operations. This is the situation Target/AArch64 is in and we never considered "fixing" LegalizeDAG so I don't see why others wouldn't just follow our example.

I've already pointed out:

setOperationAction(ISD::FADD, MVT::v4f16, Promote);
AddPromotedToType(ISD::FADD, MVT::v4f16, MVT::v4f32);

that exists today and is happy to emit ISD::FP_EXTEND. We mirrored this to support i1 based int->fp operations, although perhaps that's a case of two wrongs not making a right :)

Sure we can teach LegalizeVectorOps LegalizeDAG's PromoteNode abilities but that feels like wasted effort considering it just works. My understand of the two legalisers is that LegalizeVectorOps might emit scalar operations that require further legalisation, which is a situation that doesn't apply here considering it's vector in vector out.

So ultimately I don't know what to do here. There's nothing functionally wrong with the patch (other than a couple of minor suggestions) so I guess I'll begrudgingly accept the patch but I just think you're causing artificial restrictions that others have not felt the need to apply and thus hope you'll reconsider.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3311

typo "implemented"

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9040–9049

You can use getPromotedVTForPredicate here.

9052

Can ANY_EXTEND be used here?

9056

As above can this be getAnyExtOrTrunc?

llvm/test/CodeGen/AArch64/sve-extract-element.ll
252

Not a problem for this patch but I'm surprised to see the and here.

This revision is now accepted and ready to land.Oct 23 2020, 4:31 AM

Just making a note here that I've push D90093 so that we follow the same principle shown here for the lowering of i1 based int_to_fp conversions. This also means getPromotedVTForPredicate is now useable for this patch also.

Matt added a subscriber: Matt.Apr 25 2021, 6:35 AM
This revision was landed with ongoing or failed builds.May 17 2021, 2:51 PM
This revision was automatically updated to reflect the committed changes.