[PATCH] D78190: Add Bfloat IR type

Thu Apr 16 06:43:19 PDT 2020

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