This is an archive of the discontinued LLVM Phabricator instance.

Add LLVM type support for fp8
Needs RevisionPublic

Authored by kushanam on Dec 14 2022, 9:04 PM.

Details

Summary

(see the change adding fp8 support to APFloat: https://reviews.llvm.org/D133823)

The purpose of this patch is to add the two FP8 data types (E5M2 and E4M3) to the LLVM. This is the first step towards adding high level FP8 support. The consequent steps will enable the target specific codegen datatype lowerings.

Update: The RFC is posted here: https://discourse.llvm.org/t/add-llvm-type-support-for-fp8-data-types-f8e4m3-and-f8e5m2/67598

Diff Detail

Event Timeline

kushanam created this revision.Dec 14 2022, 9:04 PM
Herald added a project: Restricted Project. · View Herald Transcript
kushanam requested review of this revision.Dec 14 2022, 9:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 9:04 PM

Would it be possible to qualify the E4M3 type similarly to the APFloat semantics?
My personal preference would be the FN (finite with NaN) suffix. E4M3FN with appropriate casing in each context.

ezhulenev retitled this revision from add fp8 support to Add LLVM type support for fp8.Dec 15 2022, 10:25 AM
ezhulenev edited the summary of this revision. (Show Details)
ezhulenev accepted this revision.Dec 15 2022, 11:07 AM
This revision is now accepted and ready to land.Dec 15 2022, 11:07 AM

Make sure you add a note to the changelog, so people know you've changed all the enums involved.

Hi Renato - I've reviewed and this lgtm, but I rarely work on/review this part of the codebase. Do you feel like you are a good reviewer for it (the author reached out to me asking for some help coordinating reviewers)?

ezhulenev added inline comments.Dec 15 2022, 1:37 PM
llvm/include/llvm/CodeGen/ValueTypes.td
242

Comments mention f16 vectors (should be f8)

kushanam updated this revision to Diff 483339.Dec 15 2022, 2:16 PM

addressing the "Comments mention f16 vectors (should be f8)" comment

llvm/include/llvm/CodeGen/ValueTypes.td
242

Comments mention f16 vectors (should be f8)

Thanks, Addressed.

llvm/include/llvm/CodeGen/SelectionDAG.h
1819

I think we agreed that this should be f8e4m3 (no "p") to match the others. We would follow the convention that we established in MLIR, as that was what was discussed.

https://github.com/llvm/llvm-project/blob/main/mlir/lib/AsmParser/TokenKinds.def#L96

arsenm requested changes to this revision.Dec 16 2022, 5:43 AM
arsenm added a subscriber: arsenm.

Missing bitcode and assembler tests

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

Typo but

llvm/include/llvm/IR/Type.h
161

Capitalize Bit

190–192

Should add some transform tests that hit this

This revision now requires changes to proceed.Dec 16 2022, 5:43 AM
arsenm added inline comments.Dec 16 2022, 5:45 AM
llvm/lib/IR/Constants.cpp
1543–1555

This isn't tested

3165–3172

Not tested

arsenm added inline comments.Dec 16 2022, 5:48 AM
llvm/lib/IR/Type.cpp
40–43

Move to bottom?

dmgreen added inline comments.
llvm/docs/BitCodeFormat.rst
1150

code 26 -> code 27

llvm/include/llvm/Support/MachineValueType.h
222

These look like they will be incorrect for the newly added vector sizes. Same for the SCALABLE ones below.

333

This looks like it might be overflowing now too.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
2308

HALF -> FP8

arsenm added inline comments.Dec 16 2022, 6:06 AM
llvm/include/llvm-c/Core.h
150–151

You can't break the C API. Thees need to go to the end

kushanam updated this revision to Diff 483852.Dec 18 2022, 4:16 PM

Addressing review commnents, adding funtionality to pass the MLIR tests

arsenm requested changes to this revision.Dec 19 2022, 8:51 AM

Tests still missing

This revision now requires changes to proceed.Dec 19 2022, 8:51 AM

Tests still missing

Hi @arsenm, I looked at the existing tests and got an assumption that the existing way to test those, is to have the codegen pipeline supporting the datatype in the "Target" level. That is something I am working on for the follow up patch and thought then I could add the tests. If you could give me a pointer to the existing tests that I might have missed, I would add them. Thank you in advance!

Tests still missing

Hi @arsenm, I looked at the existing tests and got an assumption that the existing way to test those, is to have the codegen pipeline supporting the datatype in the "Target" level. That is something I am working on for the follow up patch and thought then I could add the tests. If you could give me a pointer to the existing tests that I might have missed, I would add them. Thank you in advance!

All of the IR changes are testable without codegen support. Needs some tests in test/Assembler, test/Bitcode, test/Verifier, and some constant folding tests in test/Transforms/InstSimplify. See 8c24f33158d81d5f4b0c5d27c2f07396f0f1484b which added bfloat (although this includes far fewer tests than I would have liked to see. For example it's missing the all important bitcode compatibility tests)

Tests still missing

Hi @arsenm, I looked at the existing tests and got an assumption that the existing way to test those, is to have the codegen pipeline supporting the datatype in the "Target" level. That is something I am working on for the follow up patch and thought then I could add the tests. If you could give me a pointer to the existing tests that I might have missed, I would add them. Thank you in advance!

All of the IR changes are testable without codegen support. Needs some tests in test/Assembler, test/Bitcode, test/Verifier, and some constant folding tests in test/Transforms/InstSimplify. See 8c24f33158d81d5f4b0c5d27c2f07396f0f1484b which added bfloat (although this includes far fewer tests than I would have liked to see. For example it's missing the all important bitcode compatibility tests)

I think I covered all the bases for testing an IR change in D139902. This is a bit broader as it's a type

Tests still missing

Hi @arsenm, I looked at the existing tests and got an assumption that the existing way to test those, is to have the codegen pipeline supporting the datatype in the "Target" level. That is something I am working on for the follow up patch and thought then I could add the tests. If you could give me a pointer to the existing tests that I might have missed, I would add them. Thank you in advance!

All of the IR changes are testable without codegen support. Needs some tests in test/Assembler, test/Bitcode, test/Verifier, and some constant folding tests in test/Transforms/InstSimplify. See 8c24f33158d81d5f4b0c5d27c2f07396f0f1484b which added bfloat (although this includes far fewer tests than I would have liked to see. For example it's missing the all important bitcode compatibility tests)

I think I covered all the bases for testing an IR change in D139902. This is a bit broader as it's a type

Thank you Matt, for the pointers.

kushanam updated this revision to Diff 484407.Dec 20 2022, 3:38 PM

Adding the smoke tests for f8e5m2 and f6e4m3 types

Also needs bitcode compatibility.ll test

llvm/test/Assembler/fp8.ll
2

no opt, especially -O3

28–32

The constant folding tests belong in test/Transforms/InstSimplify, and should cover the full range of floating point classes and operations

61

Should check math intrinsics

arsenm added inline comments.Dec 21 2022, 11:05 AM
llvm/lib/IR/Type.cpp
214–223

This whole function should probably convert to a switch

nikic requested changes to this revision.Dec 24 2022, 1:22 AM
nikic added subscribers: efriedma, nikic.

Is there an RFC for this change? In https://discourse.llvm.org/t/rfc-add-apfloat-and-mlir-type-support-for-fp8-e5m2/65279 the addition of an LLVM IR type was explicitly out of scope. See also in particular the comment by @efriedma here: https://discourse.llvm.org/t/rfc-add-apfloat-and-mlir-type-support-for-fp8-e5m2/65279/3

I'm personally rather skeptical about adding new ad-hoc floating-point types to IR without at least some restructuring of the existing system. At the IR level, my expectation would be that all the different float types get consolidated into a single type that is parameterized over the floating-point semantics.

This revision now requires changes to proceed.Dec 24 2022, 1:22 AM

Is there an RFC for this change? In https://discourse.llvm.org/t/rfc-add-apfloat-and-mlir-type-support-for-fp8-e5m2/65279 the addition of an LLVM IR type was explicitly out of scope. See also in particular the comment by @efriedma here: https://discourse.llvm.org/t/rfc-add-apfloat-and-mlir-type-support-for-fp8-e5m2/65279/3

I'm personally rather skeptical about adding new ad-hoc floating-point types to IR without at least some restructuring of the existing system. At the IR level, my expectation would be that all the different float types get consolidated into a single type that is parameterized over the floating-point semantics.

Hi Nikita et al, Thanks for the feedback. The idea of parametrizing APFloat sounds like a neat one, though also complex given both the amount of overhauling that is needed to get it right, as well as the need for proper design and dealing with a myriad of exceptions (e.g. NaNs, Infs, etc), for which my team doesn't have LLVM competence (or bandwidth) yet to implement on our own. From a pragmatic standpoint, I am afraid this effort would require many months of concerted planning, deliberation, implementation and testing before it would be ready to serve as a basis for FP8 extensions.

As you may know, the introduction of FP8 is one of the main features of NVIDIA's new Hopper GPUs, so we are trying very hard to adopt it across all DL frameworks. We hope LLVM will be a benefit in this process, since its features are inherited by many different frameworks. This patch (which defines the types) and another currently in development (to introduce ptx codegen) is needed as part of supporting native Hopper instructions for FP8 casts. My team has already implemented FP8 support in XLA and we now have end-to-end support in TF (and JAX and others) through XLA. If the changes cannot be provided in LLVM in a reasonable timeframe, we will need to pursue framework specific hacks to enable efficient type conversions, which is something we'd very much like to avoid.

Is there an RFC for this change? In https://discourse.llvm.org/t/rfc-add-apfloat-and-mlir-type-support-for-fp8-e5m2/65279 the addition of an LLVM IR type was explicitly out of scope. See also in particular the comment by @efriedma here: https://discourse.llvm.org/t/rfc-add-apfloat-and-mlir-type-support-for-fp8-e5m2/65279/3

I'm personally rather skeptical about adding new ad-hoc floating-point types to IR without at least some restructuring of the existing system. At the IR level, my expectation would be that all the different float types get consolidated into a single type that is parameterized over the floating-point semantics.

Indeed, the original RFC was scoped to enable a further discussion about adding these types to LLVM IR but did not address that topic itself. I don't work on this level of LLVM, and all I can offer is:

  • On the MLIR side, we concluded that we didn't need to figure out a generalization strategy for this scope (yet) given the existing parameterization on fltSemantics (which is similar to LLVM).
  • Much of this was modeled after patches to add BFLOAT16 support to APFloat/MLIR/LLVM, and that was deemed a straight-forward addition (but we don't just blindly carry forward precedent, so just fyi).

Sounds like an RFC is in order?

nikic added a subscriber: nlopes.Jan 9 2023, 1:38 PM

To clarify, I'm not necessarily saying that we can't move forward with this implementation approach, just that it should go through an RFC to have a wider discussion on the topic. The outcome of that may well be that we should just carry on as before.

I should also clarify that I'm definitely not excepting a fully-generic floating point system in the same vein as we have for integers, but rather some more consolidated handling for floating-point types (as a single floating point type parameterized over semantics, rather than a collection of many separate types). Based on @stellaraccident's comment, that would be similar to what MLIR does? This should be much simpler to do than fully-generic floats.

Finally, I think something very important when it comes to these specific types (as opposed to "more float types" in general) is that these types deviate from IEEE semantics in ways that existing types don't, so need to be careful about how these affect IR semantics and middle-end optimization. I'm pretty sure that just exposing the existing code to these types will cause miscompiles in some cases -- e.g. it looks like creating an infinity value of these types will create a NaN instead, so if we create Inf as a neutral value for min/max, we'll now use NaN instead, which would be incorrect. There's probably more things like this lurking in various transforms.

In order to be reasonably confident about optimization correctness for these types, what we'd want is to land basic LangRef + IR support first, then work with @nlopes for Alive2 support (ironing out and specifying any details about semantics along the way) and then duplicating key bits of existing tests for the new types, and checking that alive2 does not detect miscompiles.

llvm/tools/llvm-c-test/echo.cpp
83

Drive-by note: This is creating the wrong type...

kushanam edited the summary of this revision. (Show Details)Jan 10 2023, 12:59 PM
kushanam edited the summary of this revision. (Show Details)Jan 10 2023, 1:02 PM
kushanam changed the visibility from "Public (No Login Required)" to "All Users".Feb 15 2023, 2:31 PM
efriedma changed the visibility from "All Users" to "Public (No Login Required)".Feb 16 2023, 6:10 PM
hliao added a subscriber: hliao.Apr 24 2023, 12:10 PM