This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Add F16 and BF16.
ClosedPublic

Authored by bixia on Jun 3 2022, 3:38 PM.

Details

Summary

This is the first PR to add F16 and BF16 support to the sparse codegen. There are still problems in supporting these two data types, such as BF16 is not quite working yet.

Add tests cases.

Diff Detail

Event Timeline

bixia created this revision.Jun 3 2022, 3:38 PM
Herald added a project: Restricted Project. · View Herald Transcript
bixia requested review of this revision.Jun 3 2022, 3:38 PM
bixia added a reviewer: wrengr.Jun 3 2022, 3:38 PM
bixia updated this revision to Diff 434183.Jun 3 2022, 4:02 PM

Add CMakeLists.txt and BUILD.bazel change.

aartbik added inline comments.Jun 3 2022, 4:22 PM
mlir/include/mlir/ExecutionEngine/Float16bits.h
1

nit: typically, more of the empty white space is filled with the ------- line here

10

them -> these types

20–21

The reference to the FOREVERY macro seems a bit out of in place in this file (given that others may use it outside the sparse compiler context). I would omit it, or, if you want to keep it, otherwise put it in a separate sentence with some intro explanation.

34

Add documentation on providing the output methods for both.

mlir/include/mlir/ExecutionEngine/SparseTensorUtils.h
79

can we keep FP before INT, just like we do at all other places?

mlir/lib/ExecutionEngine/Float16bits.cpp
1

same nit: fill the whitespace with -----------------------===//

10

them -> these types

18

Add documentation, i.e. union used to make the int/float aliasing explicit so we can access the raw bits

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
1570

Can we say,

// Two-byte floats

just so we start with a capital letter

bixia updated this revision to Diff 434205.Jun 3 2022, 4:44 PM
bixia marked 8 inline comments as done.

Address review comments.

mlir/include/mlir/ExecutionEngine/Float16bits.h
20–21

Removed the reference.

wrengr edited the summary of this revision. (Show Details)Jun 3 2022, 6:47 PM
wrengr added inline comments.Jun 3 2022, 7:09 PM
mlir/lib/ExecutionEngine/Float16bits.cpp
36–37

Why not combine these into f.u ^= (f.u & signMask); ?

72–73

ditto earlier comment about using constexpr

76

You can (and should) drop the equals sign here, to ensure there's no extra temporary value constructed

78–83

I feel like these (112 <<23) and (113 << 23) expression should be factored out into constexpr variables to be shared throughout the file. Especially since doing so means giving them names to indicate what they mean/do.

119

why not f16::f16(float f) : bits(float2half(f)) {} ?

121

ditto

bixia updated this revision to Diff 434242.Jun 3 2022, 10:14 PM

Fixed a test.

aartbik added inline comments.Jun 4 2022, 12:54 PM
mlir/lib/ExecutionEngine/Float16bits.cpp
78–83

+1 on that. Also, probably the shift constants (13, 23, 16) could get some symbolic names

bixia updated this revision to Diff 434478.Jun 6 2022, 7:59 AM
bixia marked 2 inline comments as done.

Replaced some constants with names.

I think something went wrong. In one revision, you had addressed comments but in a later upload, some of these were undone again?

bixia updated this revision to Diff 434843.Jun 7 2022, 9:14 AM

Restore changes that addressed the first round of comments.

mlir/lib/ExecutionEngine/Float16bits.cpp
36–37

sign is used in line 64

72–73

I think you are referring to the comment for line 76, will wait for your response there to clarify the suggestion.

76

Sorry, I don't understand the suggestion here. Would you please spell out the whole statement for this line here?

78–83

Added kF32Magic, kF32HalfExpAdjust and a few others.

aartbik accepted this revision.Jun 7 2022, 2:42 PM

One last nit. Also, wait a bit before submitting to see if Wren has some more feedback on addressing the comment she made earlier.

mlir/lib/ExecutionEngine/Float16bits.cpp
48

if you give these two hex numbers better symbolic names, it reads even better

This revision is now accepted and ready to land.Jun 7 2022, 2:42 PM
bixia updated this revision to Diff 434986.Jun 7 2022, 4:08 PM

Name the constants for qNan and Inf.

bixia marked an inline comment as done.Jun 7 2022, 4:09 PM
This revision was automatically updated to reflect the committed changes.
wrengr added a comment.Jun 8 2022, 2:50 PM

Sorry I missed the comments; I was out yesterday for a doctor's appt.

I'll make a followup CL with the changes I had in mind. While working on that I recall that we might not be able to use some of my suggestions, since they're C++11 features whereas this file must be C++98 compliant. Nevertheless, once I finish the CL I'll post it just to help clarify what I meant.

mehdi_amini added inline comments.Aug 26 2023, 3:27 PM
mlir/lib/ExecutionEngine/Float16bits.cpp
117

Isn't this kind of type punning actually UB?

mehdi_amini added inline comments.Aug 26 2023, 3:42 PM
mlir/lib/ExecutionEngine/Float16bits.cpp
33

I can't find a reference in Eigen, I found https://gitlab.com/libeigen/eigen/-/blob/master/Eigen/src/Core/arch/Default/BFloat16.h#L347-359 which is much simpler actually. Can you help?