Differential D140088
Add LLVM type support for fp8 Authored by kushanam on Dec 14 2022, 9:04 PM.
Details (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 TimelineComment Actions Would it be possible to qualify the E4M3 type similarly to the APFloat semantics? Comment Actions Make sure you add a note to the changelog, so people know you've changed all the enums involved. Comment Actions 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)?
Comment Actions addressing the "Comments mention f16 vectors (should be f8)" comment
Comment Actions 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! Comment Actions 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) Comment Actions I think I covered all the bases for testing an IR change in D139902. This is a bit broader as it's a type
Comment Actions 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. Comment Actions 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. Comment Actions 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:
Sounds like an RFC is in order? Comment Actions 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.
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||