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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
@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.
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. |
- Add AutoUpgrade code to convert aarch64.neon.frintn to roundeven
- Add test for above AutoUpgrade
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? |
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. |
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. |
- 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.
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.