[CIR][CIRGen] Add support for __fp16 type #950
Conversation
There was a problem hiding this comment.
Thanks for addressing this, pretty cool!
Have you thought about making fp16 having an extra argument that tells whether it's soft? Seems like it would make CIRGen more simple and offset the intrinsic calls to lowering time, but maybe this is just my not-so-informed opinion?
| a single- or double-precision floating point value before performing any | ||
| arithmetic operations. | ||
|
|
||
| Additionally, `!cir.f16.storage` is lowered to the `i16` LLVM type. The |
There was a problem hiding this comment.
In the LLVM lowering pipeline ...
|
|
||
| Additionally, `!cir.f16.storage` is lowered to the `i16` LLVM type. The | ||
| promotion and un-promotion of `!cir.f16.storage` values are lowered to | ||
| calls to `llvm.convert.from.f16` and `llvm.convert.to.f16` LLVM intrinsic |
There was a problem hiding this comment.
... calls to cir.llvm.intrinsic convert.from.f16 and ...
| def CIR_StorageOnlyFP16 : CIR_FloatType<"StorageOnlyFP16", "f16.storage"> { | ||
| let summary = "CIR type that represents a storage-only fp16 type"; | ||
| let description = [{ | ||
| Floating-point type that represents a storage-only fp16 type. |
There was a problem hiding this comment.
Should we call this fp16.soft? This comes from the usual semantics we get for more regular floats when -msoft-float is used, which seems to be similar here.
| } else { | ||
| value = Builder.createCast(CGF.getLoc(E->getExprLoc()), | ||
| mlir::cir::CastKind::floating, input, | ||
| CGF.CGM.FloatTy); |
There was a problem hiding this comment.
Do we have a real world case for this else branch to test?
I'm thinking about how we lower this CastOp in current LLVM Lowering.
it looks like it may exercise this lowering code
rewriter.replaceOpWithNewOp<mlir::LLVM::FPExtOp>
And mlir::LLVM::FPExtOp is expecting floating type arg,
But your type converter converts cir.f16.storage into MLIR::IntegerType.....
There was a problem hiding this comment.
This else branch is the normal case and it is hit when input is of type !cir.f16. When input is of type !cir.f16.storage, the then branch would be taken instead.
Do we have a real world case for this else branch to test?
Well since the else branch is the normal one, let's consider instead the then branch. I searched in the upstream clang tests with llvm.convert.from.fp16 and remarkably I found nothing. For most targets the useFP16ConversionIntrinsics() function call would return false, which indicates for most targets the else branch would be taken. But there are indeed some exceptions, for example mips-unknown-linux-gnu: https://godbolt.org/z/feGvPYGxE , unfortunately CIR now supports none of these exceptions.
Anyway for now I think I'm going to remove the !cir.f16.storage stuff and make it assert on relevant paths, including the then branch here.
There was a problem hiding this comment.
Yep, you are right, the else branch is the most common case, and it should takes existing cir.f16 type as input. And lowering supports it without problem.
Yes I implemented this like what you said in the first attempt, but I eventually decided to invent a new type for soft fp16 since I feel we should not add soft fp16 type to the For now, as said in comment #950 (comment), I'm going to remove this new type from this PR since CIR yet supports none of the targets that generate this new type. |
Lancern
changed the title
|
LGTM, especially like the new update that focuses on supporting the common case. We can also add FP16Storage type and intrinsics when we have a need for it. |
Just a quick reminder that |
This PR adds support for the `__fp16` type. CIRGen and LLVM lowering is included. Resolve llvm#900 .
This PR adds support for the `__fp16` type. CIRGen and LLVM lowering is included. Resolve llvm#900 .
This PR adds support for the `__fp16` type. CIRGen and LLVM lowering is included. Resolve #900 .
This PR adds support for the
__fp16type. CIRGen and LLVM lowering is included.Resolve #900 .