This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by joshua-arch1 on May 24 2023, 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.May 24 2023, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 2:57 AM
joshua-arch1 requested review of this revision.May 24 2023, 2:57 AM
joshua-arch1 edited the summary of this revision. (Show Details)
asb requested changes to this revision.May 24 2023, 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.May 24 2023, 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.EditedMay 24 2023, 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.May 24 2023, 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.May 24 2023, 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.May 25 2023, 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.

This comment was removed by joshua-arch1.
This comment was removed by joshua-arch1.

The first version of this patch came in May 24th, earlier than a similar patch implemented by @asb https://reviews.llvm.org/D151663. Why could @asb's patch be accepted first?

The first version of this patch came in May 24th, earlier than a similar patch implemented by @asb https://reviews.llvm.org/D151663. Why could @asb's patch be accepted first?

Some parts of @asb’s patch look more correct. Like the use isSoftFloatABI().

I think in this case, it should be to provide review suggestions for modification, instead of immediately accepting other people's improvement patch, which has a great impact on the enthusiasm of contributors :)

craig.topper added inline comments.Jun 19 2023, 12:42 PM
llvm/test/CodeGen/RISCV/bfloat.ll
2

This file already exists in trunk. Not sure if is the same or not?

craig.topper added inline comments.Jun 19 2023, 12:52 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
429

This wasn't needed by D151663? Is there a test case missing from D151663 that requires this?

433

D151663 uses

setOperationAction(ISD::FP_TO_BF16, MVT::f32,
                   Subtarget.isSoftFPABI() ? LibCall : Custom);

Is that difference important?

2461

Why wasn't this part needed in D151663?

joshua-arch1 abandoned this revision.Jul 28 2023, 1:05 AM
joshua-arch1 added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
429

Maybe I can remove this part. In my first version of this patch, the expand is needed for LoadExt and TruncStore.

433

Since we use also libcall in the lowerFP_TO_BF16 function, I don't think we need to use "Libcall" here for SoftABI.

2461

Although bf16 type isn't legal, that does not mean this condition will never happen.