[PATCH] D78190: Add Bfloat IR type

Tue May 5 09:42:23 PDT 2020

stuij added a comment.

In D78190#2018685 <https://reviews.llvm.org/D78190#2018685>, @fpetrogalli wrote:

> 1. Shouldn't we test also that the parser is happy with the following expressions?

Added. Thanks.

> 2. 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.

> 3. 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78190