[PATCH] D157204: [MLIR][Math] Add support for f16 in the expansion of math.roundeven

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp:321
+  unsigned mantissaWidth =
+      llvm::cast<FloatType>(operandETy).getFPMantissaWidth() - 1;
+  unsigned exponentWidth = bitWidth - mantissaWidth - 1;
----------------
alexander-shaposhnikov wrote:
> qcolombet wrote:
> > Why the `-1` here?
> getFPMantissaWidth returns the width that takes into account the implicitly set bit,
> see e.g. https://github.com/openxla/stablehlo/blob/a9ab84b453c26ecf179bd6e845ee8bbcc9f7cac0/stablehlo/reference/Element.cpp#L953 
> 
Thanks for the pointer.

Please add a comment saying that the mantissa includes the integer bit.

For reference, a more llvm-ish justification would be:
https://github.com/llvm/llvm-project/blob/81ccfeb86afc13e1ef3d4872452c855060b8f67e/llvm/lib/Support/APFloat.cpp#L108

Which is used from:
https://github.com/llvm/llvm-project/blob/81ccfeb86afc13e1ef3d4872452c855060b8f67e/llvm/lib/Support/APFloat.cpp#L292

Which is finally used from `getFPMantissaWidth`:
https://github.com/llvm/llvm-project/blob/81ccfeb86afc13e1ef3d4872452c855060b8f67e/mlir/lib/IR/BuiltinTypes.cpp#L152C16-L152C16

(May be worth adding a comment on `getFPMantissaWidth` directly too)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157204/new/

https://reviews.llvm.org/D157204