-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CIR][CIRGen][Builtin][Neon] Lower __builtin_neon_vrndns_f32 #858
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor nits
// CIR: cir.store %arg0, [[ARG_SAVE:%.*]] : !cir.float, !cir.ptr<!cir.float> | ||
// CIR: [[INTRIN_ARG:%.*]] = cir.load [[ARG_SAVE]] : !cir.ptr<!cir.float>, !cir.float | ||
// CIR: [[INTRIN_RES:%.*]] = cir.llvm.intrinsic "llvm.roundeven.f32" [[INTRIN_ARG]] : (!cir.float) | ||
// TODO: Add check making sure below return actually gets value from the intrinsic call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the TODO
here, the redundant load/store for return is a common pattern in CIR and we are not really testing anything about cir.return, checking the cir.return is useful here to delimit the test boundary, nothing more.
llvm_unreachable("NYI"); | ||
mlir::Value arg0 = buildScalarExpr(E->getArg(0)); | ||
args.push_back(arg0); | ||
return buildAArch64NeonCall(NEON::BI__builtin_neon_vrndns_f32, *this, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename buildAArch64NeonCall
to buildNeonCall
. If there's anything AArch64 specific in its behavior, should be checked / asserted inside the function.
As title. Also introduced buildAArch64NeonCall skeleton, which is partially the counterpart of OG's EmitNeonCall. And this could be use for many other neon intrinsics. --------- Co-authored-by: Guojin He <guojinhe@meta.com>
As title. Also introduced buildAArch64NeonCall skeleton, which is partially the counterpart of OG's EmitNeonCall. And this could be use for many other neon intrinsics. --------- Co-authored-by: Guojin He <guojinhe@meta.com>
As title. Also introduced buildAArch64NeonCall skeleton, which is partially the counterpart of OG's EmitNeonCall. And this could be use for many other neon intrinsics. --------- Co-authored-by: Guojin He <guojinhe@meta.com>
As title. Also introduced buildAArch64NeonCall skeleton, which is partially the counterpart of OG's EmitNeonCall. And this could be use for many other neon intrinsics. --------- Co-authored-by: Guojin He <guojinhe@meta.com>
As title. Also introduced buildAArch64NeonCall skeleton, which is partially the counterpart of OG's EmitNeonCall. And this could be use for many other neon intrinsics. --------- Co-authored-by: Guojin He <guojinhe@meta.com>
As title.
Also introduced buildAArch64NeonCall skeleton, which is partially the counterpart of OG's EmitNeonCall. And this could be use for many other neon intrinsics.