-
Notifications
You must be signed in to change notification settings - Fork 125
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] support builtin signbit #1033
Conversation
handle PPC fp128 clangir/clang/lib/CodeGen/CGBuiltin.cpp Lines 668 to 685 in b53784a
|
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 in general, some minor nits inline
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.
Looks mostly good, we still need to track missing PPC support, see comments inline. Thanks for working on this
c12f959
to
3c2ff4d
Compare
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.
Thanks for the updates, few more comments
858bd29
to
86d44eb
Compare
My suggestion is for you to first tackle the non-fp80 case, and add unrecheable for that case, in a follow up PR you tackle all the fp80 related shenenigans in one go. How about that? |
86d44eb
to
1c4fc42
Compare
Thanks for your suggestion! I agree that it makes sense to tackle the non-fp80 case first. |
1c4fc42
to
961b4ce
Compare
mlir::dyn_cast<cir::LongDoubleType>(op.getInput().getType())) { | ||
if (mlir::isa<cir::FP80Type>(longDoubleType.getUnderlying())) { | ||
// see https://github.com/llvm/clangir/issues/1057 | ||
llvm_unreachable("NYI"); |
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.
For next rounds: during LLVM lowering we're better off erroring out return op.emitError(...)
- I'm gonna merge this anyway now cause I know you're gonna be working on this soon anyways! Thanks for the fixes.
follow #1033 handle `LongDoubleType` with `FP80Type`.
This patch adds support for the
__builtin_signbit
intrinsic. The intrinsic requires special handling for PowerPC; however, since ClangIR does not currently support PowerPC, this handling is omitted in this implementation.