This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Code generation for fixed length vector adds.
ClosedPublic

Authored by paulwalker-arm on Jun 24 2020, 10:27 AM.

Details

Summary

Teach LowerToPredicatedOp to lower fixed length vector operations.

Add AArch64ISD nodes and isel patterns for predicated integer
and floating point adds.

Together this enables SVE code generation for fixed length vector adds.

Diff Detail

Event Timeline

Herald added a reviewer: efriedma. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I've deviated from the base fixed length support to create this patch that adds the base work that's likely common to many of the arithmetic and logical operations. I went with refactoring LowerToPredicatedOp as it's likely to be used in all the same places as those required for fixed length types.

I've probably gone overboard with the tests but I figure doing so for just the adds gives us enough coverage so that people adding support for similar operations only require minimal tests. Perhaps this matches what I've done for element types like double or perhaps we can just add tests for a single vector length (e.g. 512bit).

NOTE: Patch requires D82466 for sve-fixed-length-int-arith.ll to pass.

Fixed a couple of lines missing -DAG.

Removed rogue comments.

Hi @paulwalker-arm, are you expecting those four tests to fail on this patch?

Hi @paulwalker-arm, are you expecting those four tests to fail on this patch?

There's a comment higher up where I mention this patch requires D82466, which adds some missing MVT.

Hi @paulwalker-arm, ok fair enough. It was just because you said "sve-fixed-length-int-arith.ll" fails due to missing D82466, but there are 4 tests failing. Just making sure these are all due to D82466 that's all. :)

Thanks Dave, turns out changing FADD to custom lowering is breaking the cost model test. I've added an entry to restore the original cost because lowering to SVE should not add additional cost beyond requiring a predicate, which is likely to be spread across many instructions.

This should be extended to more ISD nodes, hence me using plurals within the comment, but currently we only test FADD so I'm keeping my change minimal.

Rebase to gain required MVTs.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14913

Could we modify the assert to remove the branch?

assert(isa<CondCodeSDNode>(V) || V.getValueType().isScalableVector() &&
           "Only scalable vectors are supported!");

Same with the code above too.

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

Is having both ir_op and int_op problematic going forward? E.g. how to match an intrinsic with an undef merge.

X86 solves a similar problem with the tables in lib/Target/X86/X86IntrinsicsInfo.h. That might be something to consider long term. I'm not sure if it's a great fit though.

I don't think this needs to be changed now, but something to consider...

paulwalker-arm marked 4 inline comments as done.Jun 26 2020, 8:41 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14913

I can modify this case but the earlier code is not a straight copy so some kind of control flow is required.

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

I don't believe there's anything problematic as there's nothing to prevent adding SVE_3_Op_Pat_SelZero patterns for ir_op and SVE_3_Op_Pat for int_op. It's just that this patch does not have tests for those cases so I've not added the patterns.

paulwalker-arm marked 2 inline comments as done.

Post review fixes.

cameron.mcinally accepted this revision.Jun 26 2020, 8:50 AM

LGTM

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14913

Oh, I missed that. If you want to leave it so the two cases are structured similarly, that's fine.

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

Good point. I'm not sure if canonicalizing the intrinsics would save code or add more, so I'll leave it there.

This revision is now accepted and ready to land.Jun 26 2020, 8:50 AM
sdesmalen added inline comments.Jun 26 2020, 9:33 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14808

nit: is getAllTruePredicate not a better name given the value this function returns? (similar for getAllTruePredicateFor(Scalable|FixedLength)Vector)

paulwalker-arm marked 2 inline comments as done.Jun 26 2020, 10:31 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14808

getPredicateForFixedLengthVector (and by extension getPredicateForVector) doesn't return an AllTrue predicate but rather a predicate with only the first VT.getNumElts() being true with everything else being false.

I think we're going to have a similar situation with types like <n x 2 x f32> so tried to avoid AllTrue since that's an artifact of our current ISEL rather than representative of what is actually going on.

This revision was automatically updated to reflect the committed changes.
paulwalker-arm marked an inline comment as done.
sdesmalen added inline comments.Jun 26 2020, 1:20 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14808

Yes, that's actually what I meant with AllTruePredicateForFixedLengthVector(..., EVT VT) . The ForFixedLengthVector part (passed as VT) suggests to me that it returns a predicate that is all true for the lanes of the fixed-length vector, not beyond that.