[PATCH] D133823: Add APFloat and MLIR type support for fp8 (e5m2).

[Renato Golin via Phabricator via llvm-commits]

rengolin accepted this revision.
rengolin added a comment.

Honestly, there isn't much meat here, other than the decision to support and the name bikeshedding, both of which I think are well covered in the RFC.

Most of the code is mechanical changes to adding a new type and in that are perfectly fine.

The main questions for me are:

1. Do we want float8? YES!
2. Do we want this specific variant? Yes, IMO.
3. Is adding a single variant (amongst so many) worth it? Yes, at the very least for initial support. Adding others or changing this should be easy.
4. Do we want a more generic implementation which we derive specific cases from? Maybe. Regardless, this is an initial implementation and we can generalise later, if desired.
5. Is the end goal to have only concrete cases listed? Strong YES from me. This should be less than 1/2 dozen and we can cope with that, even if we don't have a generic implementation.
6. Does this patch satisfy all the constraints above? Yes, IMO.

The only issue that could be raised for adding a specific one is that of backwards compatibility. As long as we're sure what we want before forking the next version (Jan-ish), it should be fine to experiment and gather opinions.

So, my approval of it is exactly that: my support for this change. As usual, please wait for more approvals to make sure we have enough support for the specific way you implemented it.

Renato


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133823/new/

https://reviews.llvm.org/D133823