This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE/NEON] Add support for FROUNDEVEN for both NEON and fixed length SVE
ClosedPublic

Authored by bsmith on Mar 12 2021, 3:58 AM.

Details

Summary

Previously NEON used a target specific intrinsic for frintn, given that
the FROUNDEVEN ISD node now exists, move over to that instead and add
codegen support for that node for both NEON and fixed length SVE.

Diff Detail

Event Timeline

bsmith created this revision.Mar 12 2021, 3:58 AM
bsmith requested review of this revision.Mar 12 2021, 3:58 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 12 2021, 3:58 AM

Hi @ bsmith,

Thank you for adding me as a reviewer, although I don't think I am the more qualified to approve or not this patch.
But I have a question:
Why is this patch only changing int_aarch64_neon_frintn and not int_aarch64_sve_frintn?
Is there a particular reason to do so?
As you said in the commit message the ISD node for FROUNDEVEN exists now.
If so it would be too much to explain that in the commit message?
Thank you,
Carol

Why is this patch only changing int_aarch64_neon_frintn and not int_aarch64_sve_frintn?
Is there a particular reason to do so?

Things are done slightly differently for SVE in this regard, in principal yes, we could emit roundeven instead of frintn from the ACLE intrinsic, however all of the other ACLE intrinsics also emit SVE specific LLVM intrinsics rather than the arch-indep nodes. This patch doesn't change that in order to stay consistent, if we did want to change that it should be done as a separate patch that changes all of them.

Why is this patch only changing int_aarch64_neon_frintn and not int_aarch64_sve_frintn?
Is there a particular reason to do so?

Things are done slightly differently for SVE in this regard, in principal yes, we could emit roundeven instead of frintn from the ACLE intrinsic, however all of the other ACLE intrinsics also emit SVE specific LLVM intrinsics rather than the arch-indep nodes. This patch doesn't change that in order to stay consistent, if we did want to change that it should be done as a separate patch that changes all of them.

@CarolineConcatto There are two levels at play here. At the top level (C->LLVM) the SVE ACLE cannot use the roundeven intrinsic because that operation takes a single data operand whereas for SVE the operation is predicated and thus also requires predicate and passthru operands (i.e. the two intrinsics are doing different things). At the bottom level (CodeGen) we already lower scalable vector variants of both intrinsics to ISD::FROUNDEVEN_MERGE_PASSTHRU which is the "masked" version of ISD::FROUNDEVEN.

dmgreen added inline comments.Mar 15 2021, 4:49 AM
llvm/include/llvm/IR/IntrinsicsAArch64.td
476

If you are removing the old intrinsic (which is great), then it will need some AutoUpgrade code from the old to the new. Hopefully in this case that's pretty simple. Look for how aarch64.rbit is done.

bsmith updated this revision to Diff 330629.Mar 15 2021, 6:02 AM
  • Add AutoUpgrade code to convert aarch64.neon.frintn to roundeven
  • Add test for above AutoUpgrade
bsmith marked an inline comment as done.Mar 15 2021, 6:02 AM

Thanks. This looks sensible, from what I can tell.

llvm/include/llvm/Target/TargetSelectionDAG.td
158

Is this used? The one above should maybe say // fpround?

bsmith added inline comments.Mar 15 2021, 8:37 AM
llvm/include/llvm/Target/TargetSelectionDAG.td
158

No it's not, I added it for consistency, but perhaps I shouldn't? I think fround is correct for the one above, or at least is consistent with the others in this file, for example fextend below.

dmgreen added inline comments.Mar 16 2021, 2:08 AM
llvm/include/llvm/Target/TargetSelectionDAG.td
158

It's used below in def fpround : SDNode<"ISD::FP_ROUND" , SDTFPRoundOp>;, so it looks like its used with the fptrunc instruction, not the fround intrinsic.

I see your point about fextend... I would say they should both be changed to fpextend/fpround, for consistency with the nodes they act upon.

bsmith updated this revision to Diff 330951.Mar 16 2021, 5:25 AM
  • Remove SDTFPRoundEvenOp as it's not a correct mirror of SDTFPRoundOp since that is not for ISD::FROUND.
  • Fix comments in include/llvm/Target/TargetSelectionDAG.td for SDTFPRoundOp and SDTFPExtendOp.
dmgreen accepted this revision.Mar 16 2021, 8:01 AM

Thanks. LGTM, if no one else has comments.

This revision is now accepted and ready to land.Mar 16 2021, 8:01 AM
paulwalker-arm accepted this revision.Mar 16 2021, 9:42 AM

Mainly focused on the SVE side of things, which looks good to me.

This revision was landed with ongoing or failed builds.Mar 17 2021, 4:41 AM
This revision was automatically updated to reflect the committed changes.