stuij marked an inline comment as done.
stuij added a comment.
@craig.topper : Yes, I thought to keep this patch confined to IR. Codegen and
intrinsics patches are still to come.
================
Comment at: llvm/docs/LangRef.rst:2896
+ * - ``bfloat``
+ - 16-bit brain floating-point value (10-bit mantissa)
+
----------------
craig.topper wrote:
> bfloat doesn't have 10-bits of mantissa. its 1 bit of sign, 8-bits of
> exponent and 7-bits of mantissa.
Ai, such a fail. Thanks!
================
Comment at: llvm/lib/Support/APFloat.cpp:4837
APFloat APFloat::getAllOnesValue(unsigned BitWidth, bool isIEEE) {
if (isIEEE) {
----------------
craig.topper wrote:
> This is a bit of strange interace. I wonder if it wouldn't be possible to
> have the callers pass the semantics instead of the bitwidth and then this
> function could get the bitwidth from the semantics. It looks like there
> aren't many callers. I believe the one in Constants.cpp could get the
> semantics from getFltSemantics on the Type. Probably similar could be done at
> other callers?
Yes, I wholeheartedly concur. Turns out the function was also used in both
front- and backend, but luckily there were already similar getters available
for FltSemantics. I hoped to get also get rid of the BitWidth argument, but it
turns out that it's not guaranteed that that FltSemantics has that filled in
properly. Hardcoding exceptions didn't feel very nice and would make this a fn
that can easily be overlooked to update for new types.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78190/new/
https://reviews.llvm.org/D78190
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
