This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][BF16] Enable __bf16 for riscv targets
ClosedPublic

Authored by joshua-arch1 on May 18 2023, 7:53 PM.

Details

Summary

The RISC-V psABI recently added __bf16 in https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/367.
Now we can enable this new type in clang.

Diff Detail

Event Timeline

joshua-arch1 created this revision.May 18 2023, 7:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 7:53 PM
joshua-arch1 requested review of this revision.May 18 2023, 7:53 PM
craig.topper added a comment.EditedMay 18 2023, 10:13 PM

This test seems to crash in the backend with -march=rv64gc -mabi=lp64d

 __bf16 h1(__bf16 a) {
  return a;
}
clang/docs/LanguageExtensions.rst
807

RISC-V

This test seems to crash in the backend with -march=rv64gc -mabi=lp64d

 __bf16 h1(__bf16 a) {
  return a;
}

I haven't done any backend support fo bf16 yet. What I want to do in this patch is only to enable __bf16 for clang.

This test seems to crash in the backend with -march=rv64gc -mabi=lp64d

 __bf16 h1(__bf16 a) {
  return a;
}

I haven't done any backend support fo bf16 yet. What I want to do in this patch is only to enable __bf16 for clang.

We can’t have the compiler crashing if you use it. The backend has to go first.

This test seems to crash in the backend with -march=rv64gc -mabi=lp64d

 __bf16 h1(__bf16 a) {
  return a;
}

I haven't done any backend support fo bf16 yet. What I want to do in this patch is only to enable __bf16 for clang.

We can’t have the compiler crashing if you use it. The backend has to go first.

But we do not have any intrinsics to generate bf16 instructions right now.

craig.topper added a comment.EditedMay 18 2023, 11:18 PM

This test seems to crash in the backend with -march=rv64gc -mabi=lp64d

 __bf16 h1(__bf16 a) {
  return a;
}

I haven't done any backend support fo bf16 yet. What I want to do in this patch is only to enable __bf16 for clang.

We can’t have the compiler crashing if you use it. The backend has to go first.

But we do not have any intrinsics to generate bf16 instructions right now.

That doesn’t matter. The test I sent crashes, thats not acceptable.

Probably also means the ABI is not being implemented correctly.

That doesn’t matter. The test I sent crashes, thats not acceptable.

Probably also means the ABI is not being implemented correctly.

I have checked the log where it crashes. That's just because there are no corresponding instructions to address bf16 type in instruction selection. Everything is correct before instruction selection.

That doesn’t matter. The test I sent crashes, thats not acceptable.

Probably also means the ABI is not being implemented correctly.

I have checked the log where it crashes. That's just because there are no corresponding instructions to address bf16 type in instruction selection. Everything is correct before instruction selection.

It created an extending load from bfloat because it tried to return as an f32. That’s not the ABI. It should be in the lower 16 bits of an FP register.

kito-cheng added inline comments.May 19 2023, 1:26 AM
clang/lib/Basic/Targets/RISCV.h
118

Should be DF16b since we intend to use that for std::bfloat16_t.

clang/test/Sema/vector-decl-crash.c
0

What??? we should just remove this testcase IF we really support __bf16 IMO.

joshua-arch1 updated this revision to Diff 524148.EditedMay 21 2023, 8:00 PM

Just like AArch64 and X86, I have updated my implementation by limiting bfloat16 only when Zfbfmin/Zvfbfmin/Zvfbfwma are enabled. Now the compiler can work for bfloat16 programs without crashing.

Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2023, 8:00 PM
craig.topper added inline comments.May 21 2023, 9:14 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
435

This needs an llvm/test/CodeGen/RISCV test.

asb added a comment.May 22 2023, 9:11 AM

I feel (IMHO) this is jumping to the endpoint a bit - the usual route for something like this is:

  • MC layer support
  • LLVM codegen support (and tests!)
  • Any needed Clang support

I was hoping to push out some patches related to zfbfmin tests+codegen today but might time-out (if so, I'll return to it tomorrow).

I think the tentative conclusion of https://discourse.llvm.org/t/rfc-c-23-p1467r9-extended-floating-point-types-and-standard-names/70033 is to make __bf16_t an arithmetic type on all targets - I'd recommend keeping an eye on https://reviews.llvm.org/D150913 (and ideally helping to review it), as that's bound to interact with this patch.

This comment was removed by joshua-arch1.
clang/test/Sema/vector-decl-crash.c
0

Actually, I didn't want to change riscv64 into another target. However, I see someone directly changed x86 into riscv64 when he supported __bf16.

I feel (IMHO) this is jumping to the endpoint a bit - the usual route for something like this is:

  • MC layer support
  • LLVM codegen support (and tests!)
  • Any needed Clang support

I was hoping to push out some patches related to zfbfmin tests+codegen today but might time-out (if so, I'll return to it tomorrow).

I think the tentative conclusion of https://discourse.llvm.org/t/rfc-c-23-p1467r9-extended-floating-point-types-and-standard-names/70033 is to make __bf16_t an arithmetic type on all targets - I'd recommend keeping an eye on https://reviews.llvm.org/D150913 (and ideally helping to review it), as that's bound to interact with this patch.

Have you started the codegen part yet? I have basically tried but it may be a little troublesome since we use FPR16 for both fp16 and bf16. During instruction selection, it will be difficult to match those instructions fp16 and bf16 share.

The ABI information for __bf16 has been approved in RISCV psABI and is to be merged. Now we can enable this new type in clang.
See https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/367

The description seems verbose and inaccurate now. You can say:

The RISC-V psABI recently added __bf16 in https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/367

Since bf16 has been supported in backend, we can now come back to this clang patch. @craig.topper

joshua-arch1 edited the summary of this revision. (Show Details)Jul 28 2023, 1:09 AM

This test crashes

float foo(__bf16 a, __bf16 b)  {
  return a + b;
}
error in backend: Cannot select: t35: f32,ch = load<(dereferenceable load (s16) from %ir.b.addr), anyext from bf16> t49, FrameIndex:i32<1>, undef:i32
clang/docs/LanguageExtensions.rst
776–777

Put this above X86 so that the other X86 comment below stays together.

joshua-arch1 added a comment.EditedJul 30 2023, 11:52 PM

This test crashes

float foo(__bf16 a, __bf16 b)  {
  return a + b;
}
error in backend: Cannot select: t35: f32,ch = load<(dereferenceable load (s16) from %ir.b.addr), anyext from bf16> t49, FrameIndex:i32<1>, undef:i32

I have fixed this backend error in the patch https://reviews.llvm.org/D156646.

This revision is now accepted and ready to land.Jul 31 2023, 1:31 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 10:59 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript