Page MenuHomePhabricator

[mlir] Fix representation of BF16 constants
ClosedPublic

Authored by dcaballe on Jun 4 2020, 8:20 PM.

Details

Summary

This patch is a follow-up on https://reviews.llvm.org/D81127

BF16 constants were represented as 64-bit floating point values due to the lack
of support for BF16 in APFloat. APFloat was recently extended to support
BF16 so this patch is fixing the BF16 constant representation to be 16-bit.

Diff Detail

Event Timeline

dcaballe created this revision.Jun 4 2020, 8:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 8:20 PM
dcaballe marked 2 inline comments as done.Jun 4 2020, 8:31 PM
dcaballe added inline comments.
mlir/test/IR/parser.mlir
1095

This test is actually not passing right now because the output values are not in hex format. This is what I get:

func @bfloat16_special_values() {
  %cst = constant 1.272040e-29 : bf16
  %cst_0 = constant -1.272040e-29 : bf16
  %cst_1 = constant 1.893270e-29 : bf16
  %cst_2 = constant -1.893270e-29 : bf16
  %cst_3 = constant 1.262180e-29 : bf16
  %cst_4 = constant -1.262180e-29 : bf16
  return
}

Not sure if this means that something else needs to be changed and I missed it, or this is just the way it should be printed now.

mlir/unittests/IR/AttributeTest.cpp
146

FloatAttrSplat was already resting BF16 using an attribute. I decided to change that test to F32 and then use the BF16 attribute for this. I couldn't find a way to pass a 16-bit value (not attribute) to testSplat that worked for BF16. I tried with int16_t value = ... but it crashes.

rriddle accepted this revision.Jun 4 2020, 8:42 PM

Thanks for doing this! Was just thinking about this today.

mlir/test/IR/parser.mlir
1095

That leads me to believe that you have the wrong nan/inf values. The printer checks for that when deciding whether to use hex or not, unless something is wrong for the APFloat hex formatter for bfloat.

https://github.com/llvm/llvm-project/blob/d76e62fdb7a93d9a33f642b6b528f2562cc3c3f4/mlir/lib/IR/AsmPrinter.cpp#L1103

Can you manually print a nan or inf to make sure that your hex is correct/being parsed correctly?

https://github.com/llvm/llvm-project/blob/d76e62fdb7a93d9a33f642b6b528f2562cc3c3f4/mlir/lib/IR/AsmPrinter.cpp#L1137

mlir/unittests/IR/AttributeTest.cpp
146

That is fine, and expected. Literal types are checked to match, and BF16 doesn't have a c++ literal type.

This revision is now accepted and ready to land.Jun 4 2020, 8:42 PM
dcaballe marked an inline comment as done.Jun 5 2020, 1:33 PM

If no more comments, I'll land this after https://reviews.llvm.org/D81302

mlir/test/IR/parser.mlir
1095

Thanks! The values are OK. It's a bug in LLVM: https://reviews.llvm.org/D81302

This revision was automatically updated to reflect the committed changes.