Page MenuHomePhabricator

Add Bfloat IR type
ClosedPublic

Authored by stuij on Apr 15 2020, 3:27 AM.

Details

Summary

The Bfloat IR type is introduced to provide support for, initially, the BFloat16
datatype introduced with the Armv8.6 architecture (optional from Armv8.2
onwards). It has an 8-bit exponent and a 7-bit mantissa and behaves like an IEEE
754 floating point IR type.

This is part of a patch series upstreaming Armv8.6 features. Subsequent patches
will upstream intrinsics support and C-lang support for Bfloat.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: ctetreau. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
stuij added a comment.Apr 15 2020, 3:57 AM

The need for a BFloat IR type was discussed on the review for the Bfloat C type implementation: https://reviews.llvm.org/D76077

fhahn added a subscriber: fhahn.Apr 15 2020, 6:44 AM

Is this missing support in SelectionDAG's type system? MachineValueType.h and ValueType.h or is that coming in another patch?

llvm/docs/LangRef.rst
2967

bfloat doesn't have 10-bits of mantissa. its 1 bit of sign, 8-bits of exponent and 7-bits of mantissa.

llvm/lib/Support/APFloat.cpp
4836–4837

This is a bit of strange interace. I wonder if it wouldn't be possible to have the callers pass the semantics instead of the bitwidth and then this function could get the bitwidth from the semantics. It looks like there aren't many callers. I believe the one in Constants.cpp could get the semantics from getFltSemantics on the Type. Probably similar could be done at other callers?

stuij updated this revision to Diff 258036.Apr 16 2020, 6:21 AM
stuij marked 2 inline comments as done.

addressing review comments

Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 6:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
stuij marked an inline comment as done.Apr 16 2020, 6:36 AM

@craig.topper : Yes, I thought to keep this patch confined to IR. Codegen and intrinsics patches are still to come.

llvm/docs/LangRef.rst
2967

Ai, such a fail. Thanks!

llvm/lib/Support/APFloat.cpp
4836–4837

Yes, I wholeheartedly concur. Turns out the function was also used in both front- and backend, but luckily there were already similar getters available for FltSemantics. I hoped to get also get rid of the BitWidth argument, but it turns out that it's not guaranteed that that FltSemantics has that filled in properly. Hardcoding exceptions didn't feel very nice and would make this a fn that can easily be overlooked to update for new types.

jdoerfert added inline comments.
llvm/include/llvm/IR/Constants.h
793

Drive by: The documentation of these functions need to be changed and it needs to be clarified what would happen if you pass double to the uint16_y version.

arsenm added a subscriber: arsenm.Apr 16 2020, 8:42 AM
arsenm added inline comments.
llvm/test/Assembler/bfloat-constprop.ll
1 ↗(On Diff #258036)

This test is doing way too much. You can reduce the to just ret fadd K0, K1

llvm/test/Assembler/bfloat-conv.ll
1 ↗(On Diff #258036)

Ditto

llvm/test/Assembler/bfloat.ll
7

Ditto, all of these assembler tests can be in one file

stuij updated this revision to Diff 258185.Apr 16 2020, 3:09 PM
stuij marked 4 inline comments as done.

added some type constraints to the various getFP containter methods, adjusted getFP documentation, and simplified test cases

stuij marked an inline comment as done.Apr 16 2020, 3:12 PM
stuij added inline comments.
llvm/include/llvm/IR/Constants.h
793

Thanks. I changed the documentation and added type asserts to guard against passing unfit types.

llvm/test/Assembler/bfloat-constprop.ll
1 ↗(On Diff #258036)

Fair to all the testcase comments.

stuij marked 2 inline comments as done.Apr 17 2020, 3:26 AM

Marked all comments as having been addressed.

SjoerdMeijer accepted this revision.Apr 23 2020, 6:54 AM

This direction of an IR type was taken after a discussion on the list. All previous comments have been addressed, and the changes here all look very reasonable to me. So, LGTM, but I think we need a LGTM from someone else too. So please wait for one more LGTM.

llvm/include/llvm/IR/DataLayout.h
656

Nit:

case Type::HalfTyID:
case Type::BfloatTyID:
  return TypeSize::Fixed(16);
llvm/lib/Support/APFloat.cpp
3266

I am not real big fan of all the code duplication here in this file (just the constants are different). But there's enough prior art here, so good for now I think, and a nice opportunity for a refactoring follow-up.

This revision is now accepted and ready to land.Apr 23 2020, 6:54 AM
scanon requested changes to this revision.Apr 23 2020, 4:15 PM
scanon added a subscriber: scanon.
scanon added inline comments.
llvm/docs/LangRef.rst
2967

bfloat and fp128 should agree w.r.t. whether or not the implicit bit counts. Either 7 and 112 or 8 and 113. Also, we should use "significand" instead of "mantissa". "Mantissa" has slipped in in a bunch of places, but "significand" is the IEEE 754 terminology, and we should follow it.

3314

bfloat, half, and float

3315

bfloat, IEEE 754 half, and IEEE 754 single precision

llvm/include/llvm-c/Core.h
149

Throughout this, I think BFloat would be more consistent with other types than Bfloat is.

llvm/include/llvm/IR/Type.h
56–68

Please add a parenthetical to disambiguate from HalfTyID, like the larger-than-double types have.

This revision now requires changes to proceed.Apr 23 2020, 4:15 PM
jfb added a comment.Apr 23 2020, 4:50 PM

Thanks for adding the IR type. At a high-level this looks good to me, but I haven't done an in-depth review.

What's the plan for handling this in LLT where we don't have specific types with the same size?

Thanks for doing it this way, I think this is going to be much better long-term for LLVM.

llvm/docs/LangRef.rst
2967

I agree with Steve. In general, there's no reason for these descriptions to be as terse as they are, especially for the non-standard formats. Someone reading IR and seeing bfloat for the first time is going to come here and not be any wiser unless they figure out the right web search.

stuij updated this revision to Diff 260326.Apr 27 2020, 8:00 AM

addressed comments: throughout the patch changed the string Bfloat to BFloat, expanded description of bfloat in the documentation, addressed misc comments.

also added Type *getBFloatTy() to IRBuilder.h

thanks for the comments

stuij marked 8 inline comments as done.Apr 27 2020, 8:25 AM
stuij added inline comments.
llvm/include/llvm-c/Core.h
149

Agreed. I continued the convention of the bfloat C type patch, but it did subconsciously grate a bit. I capitalized it everywhere where the B is capitalized.

rjmccall added inline comments.Apr 27 2020, 9:12 AM
llvm/docs/LangRef.rst
2967

Hmm, now this reads more like a rationale than documentation. I would suggest:

16-bit "brain" floating-point value (7-bit significand). Provides the same number of exponent bits as `float`, so that it matches its dynamic range, just with greatly reduced precision. Used in Intel's AVX-512 BF16 extensions and ARM's ARMv8.6-A extensions, among others.

scanon added inline comments.Apr 27 2020, 9:41 AM
llvm/docs/LangRef.rst
2967

Yup, I agree. The important thing here is that someone can figure out what it is (the top half of a float); it's ok for them to have to do some reading to figure out *why* it is.

stuij updated this revision to Diff 260373.Apr 27 2020, 10:35 AM
stuij marked an inline comment as done.

updated bfloat documentation as per comments

stuij marked an inline comment as done.Apr 27 2020, 10:39 AM
stuij added inline comments.
llvm/docs/LangRef.rst
2967

Does look cleaner. Done.

stuij added a comment.May 4 2020, 9:48 AM

Hi all. Are you OK for me to commit this change?

It was about a week ago since we resolved the last outstanding issues, and in general it looks like our noses have been pointing in the same direction on this patch.

Hi @stuij,

thank you for working on this!

Admittedly, I don't know much about the Asm parser, but I have left some comments anyway.

  1. Shouldn't we test also that the parser is happy with the following expressions?
bfloat *
%... = fadd <bfloat x 4> %..., %... 
similar for <vscale x bfloat x 8>

Or is this not needed, or left to be done in a separate patch?

  1. Would it make sense to to split this patch into 2 separate patches? One that defines the enums and interfaces for bfloat, and one that does the actual parsing/emission in the IR? I suspect there is much intertwine going on, so probably not - in that case, I am happy for everything to go via a single patch.
  1. Do you need those changes in the Hexagon and x86 backend? Could they be submitted separately, with some testing?

Kind regards,

Francesco

stuij updated this revision to Diff 262122.May 5 2020, 8:36 AM

added vector tests

fpetrogalli accepted this revision.May 5 2020, 9:01 AM

Hi @stuij,

thank you for adding the vector tests. I can say the patch LGTM now :)

Francesco

stuij added a comment.May 5 2020, 9:42 AM
  1. Shouldn't we test also that the parser is happy with the following expressions?

Added. Thanks.

  1. Would it make sense to to split this patch into 2 separate patches? One that defines the enums and interfaces for bfloat, and one that does the actual parsing/emission in the IR? I suspect there is much intertwine going on, so probably not - in that case, I am happy for everything to go via a single patch.

Yes, the parts are a bit intertwined. There are some lines along which we can cut the patch, but I personally don't feel we'd gain enough clarity to justify the cost. It's not a very sexy patch in my opinion, just a number of places to add the new type to. Also, this is also a nice unit of functionality to be able to test against. And lastly, I think we already had a good number of eyes on this patch. It seems like a duplication of effort to have people again review the different parts.

  1. Do you need those changes in the Hexagon and x86 backend? Could they be submitted separately, with some testing?

This is a backend agnostic patch. If the Hexagon and/or x86 communities want to make use of the IR type in some way, then yes, they can for sure submit the necessary patches.

stuij added a comment.May 5 2020, 10:05 AM
  1. Do you need those changes in the Hexagon and x86 backend? Could they be submitted separately, with some testing?

This is a backend agnostic patch. If the Hexagon and/or x86 communities want to make use of the IR type in some way, then yes, they can for sure submit the necessary patches.

Ah sorry, I misread your comment. You're talking about the hexagon/x86 changes in this patch. They're there because of interface changes: the x86 change is because of the getAllOnesValue arguments have changed. The hexagon one is there to squash a warning because we added the bfloat Type.

stuij added a comment.May 5 2020, 10:10 AM

So are we happy with the patch as it is? Anybody else wants to LGTM?

stuij added a comment.May 11 2020, 2:48 AM

Hi there, a gentle ping: does this look good to you?

SjoerdMeijer accepted this revision.May 13 2020, 10:57 AM

Nothing much has changed here: there was already broad consensus on this change and direction, and the last few weeks we have only seen a few rounds of minor comments and nits, so still LGTM.
Please wait a day with committing to provide the opportunity for a last-minute objection to this.

This revision was not accepted when it landed; it landed in state Needs Review.May 15 2020, 7:00 AM
This revision was automatically updated to reflect the committed changes.
tstellar added inline comments.
llvm/include/llvm-c/Core.h
149

Can you move this new enum value to the end of enum definition? Adding it here changes the C ABI.

stuij marked an inline comment as done.Jun 18 2020, 9:30 AM
stuij added inline comments.
llvm/include/llvm-c/Core.h
149

Yes, np. Will make a patch today.

stuij marked an inline comment as done.Jun 18 2020, 4:55 PM
stuij added inline comments.
llvm/include/llvm-c/Core.h
149

patch up for review: https://reviews.llvm.org/D82135