Page MenuHomePhabricator

[RISCV][BF16] Make backend type bf16 to follow the psABI
Needs ReviewPublic

Authored by joshua-arch1 on Wed, May 24, 2:57 AM.

Details

Summary

Recently, bf16 type has been added in RISCV psABI. Now we need to support this datatype in LLVM backend. Right now, according to psABI, we basically address __bf16 like FP16.

Diff Detail

Event Timeline

joshua-arch1 created this revision.Wed, May 24, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 24, 2:57 AM
joshua-arch1 requested review of this revision.Wed, May 24, 2:57 AM
joshua-arch1 edited the summary of this revision. (Show Details)
asb requested changes to this revision.Wed, May 24, 5:19 AM

Sorry to not have specifics on this - I'm working on a patch in this same area and as you've found as well, it's quite fiddly. Using the __truncsfhf2 and __extendhfsf2 libcalls isn't correct - they expect 16-bit floats in a different format to bfloat16.

This revision now requires changes to proceed.Wed, May 24, 5:19 AM
This comment was removed by joshua-arch1.

Sorry to not have specifics on this - I'm working on a patch in this same area and as you've found as well, it's quite fiddly. Using the __truncsfhf2 and __extendhfsf2 libcalls isn't correct - they expect 16-bit floats in a different format to bfloat16.

I have checked compiler-rt/lib/builtins. There is truncsfbf2 for single-bfloat conversion, but I cannot find any library for bfloat extend.

joshua-arch1 updated this revision to Diff 525183.EditedWed, May 24, 7:49 AM

Modify libcall for single-bfloat conversion.

libgcc has defined truncsfbf2, truncdfbf2, truncxfbf2, trunctfbf2, trunchfbf2, truncbfhf2 and extendbfsf2

I guess this might made me seems like a GNUism, but following the same rule should be easier for our life :P

Ref: https://github.com/gcc-mirror/gcc/commit/c2565a31c1622ab0926aeef4a6579413e121b9f9

craig.topper added inline comments.Wed, May 24, 10:46 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2458

If the libcall doesn't exist, you can cast it to i16, any_extend to i32, and shift it left by 16 and bitcast it to f32.

craig.topper added inline comments.Wed, May 24, 10:50 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2458

Which is what the default lowering in LegalizeDAG.cpp does.

Address bf16_to_fp/fp_to_bf16 via bitcast and shift.

Modify implementation for f64 input.

craig.topper added inline comments.Thu, May 25, 10:42 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2438

This is an incorrect conversion. We can't truncate the mantissa. It would turn some nan encodings into infinity. Probably other issues I haven't thought of yet.

2438

We need to use truncsfbf2

2469

No need for else after an if that returns

2486

Doesn't this need to be i64 on 64 bit target?

2487

This creates an MVT::i16 after type legalization which is illegal.

2493

You share the shift code between both paths by using XLenVT.

2495

Use getShiftAmountConstant

2509

No need for else after if returns

joshua-arch1 marked 8 inline comments as done.

Update FP_TO_BF16 lowering by __truncsfbf2 libcall.

This comment was removed by joshua-arch1.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2458

We cannot directly use the default lowering in LegalizeDAG.cpp. It uses a bitcast from i32 to f32. However, i32 is not a legal type in RV64 backend.

2487

In fact, without Zfbfmin enabled, bf16 type isn't legal and this condition will never happen. If Zfbfmin is enabled, maybe we can use FCVT.S.BF16 to directly convert BF16 value to an FP32 value.