Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduplicate some floating point code #448

Closed
wants to merge 6 commits into from

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Apr 9, 2024

This is an initial effort to deduplicate the floating point code (see #432). Currently all the floating point code is copy/pasted for every size of float (f16, f32, f64). That's very verbose and error prone and is going to get worse if people want f128!

It's also unnecessary because Sail has an amazing type system so it can easily deal with functions that are generic over the size of the float.

Note, this PR is composed of 6 commits which all build and work, so I would recommend a rebase merge rather than a squash merge to preserve the history.

Normally I would make the PRs in series, but 6 PRs is going to take like a year to get through our review system so I'm doing it all in one! I recommend reviewing the individual commits though.

Also note some of the changes look like they are kind of pointless, e.g. F_or_X_H(rs1) to F_or_X(16, rs1), but that's just because this is only part of the solution.

This is the first step of making the float code generic over the float size. There are no functional changes, it just removes code duplication.
Using this alias makes things a bit clearer.
This replaces the separate functions to read/write the floating point registers as smaller widths (which boxes/unboxes them) with single generic functions.

Note in some cases we still need to explicitly pass the width in so it might seem like there is no benefit, but those will be removed once the code is fully generic.
Make `fsplit`/`fmake`, `f_is_...` and `f_negate` generic over the float size. The vector code actually already used functions like this.

This reduces a lot of copy/pasted code and will allow even more of the code to be deduplicated later.
I'm not sure of the exact history here but only the le function is actually used. I left the other two in for reference.

I also changed the flags from functions to global constants, since that's what they are.
Similar to the `F_box` accessor, these can be generic. They just need a special case for xlen == 32 when reading/writing 64-bit floats which are split over two registers when using Zfinx.
@Timmmm
Copy link
Collaborator Author

Timmmm commented Apr 9, 2024

@Incarnation-p-lee you might be interested in some of this code, especially these functions - took me a little while to get the types figured out!

// Split a floating point bitvec up into its sign, exponent, mantissa parts.
val fsplit : forall 'n, 'n in {16, 32, 64}.
  bits('n) -> (
    bits(1),
    bits(if 'n == 16 then 5 else (if 'n == 32 then 8 else 11)),
    bits(if 'n == 16 then 10 else (if 'n == 32 then 23 else 52)),
  )
function fsplit(x) = {
  if      'n == 16 then (x[15..15], x[14..10], x[9..0])
  else if 'n == 32 then (x[31..31], x[30..23], x[22..0])
  else                  (x[63..63], x[62..52], x[51..0])
}

// Join sign, exponent, mantissa parts back into a single bit vector.
val fmake : forall 'e, 'e in {5, 8, 11}.
  (
    bits(1),
    bits('e),
    bits(if 'e == 5 then 10 else (if 'e == 8 then 23 else 52)),
  ) -> bits(if 'e == 5 then 16 else (if 'e == 8 then 32 else 64))
function fmake(sign, exp, mant) = sign @ exp @ mant

But with those you can do generic functions like

// Bit vector type for floating points - restricted to f16, f32, f64.
type fbits = { 'n, 'n in {16, 32, 64}. bits('n) }

function f_is_neg_inf(x : fbits) -> bool = {
  let (sign, exp, mant) = fsplit(x);
  (  (sign == 0b1)
   & (exp  == ones())
   & (mant == zeros()))
}

and it all type checks nicely.

Copy link

github-actions bot commented Apr 9, 2024

Test Results

712 tests  ±0     0 ✅  - 712   0s ⏱️ ±0s
  6 suites ±0     0 💤 ±  0 
  1 files   ±0   712 ❌ +712 

For more details on these failures, see this check.

Results for commit cf7553b. ± Comparison against base commit a2ffa85.

@Incarnation-p-lee
Copy link

Incarnation-p-lee commented Apr 9, 2024

Thanks @Timmmm . That is very cool, I suppose this is what I need shortly. Just complete the API design draft doc, and will work out a POC on the sail side. I bet that will be very helpful.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Apr 9, 2024

I think this probably failed to using an old version of Sail. I will make a PR to update it.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Apr 9, 2024

Oh actually it turns out type level if-then-else was only added to Sail three weeks ago! Very timely. But it means we'll need to wait for the next Sail release for this to compile.

@Incarnation-p-lee
Copy link

Incarnation-p-lee commented Apr 9, 2024

Oh actually it turns out type level if-then-else was only added to Sail three weeks ago! Very timely. But it means we'll need to wait for the next Sail release for this to compile.

Got it, I suppose I can leverage SAIL_DIR to the new built one from the sail repo. Let me have a try later(in the middle of fixing ICE for GCC ...) and keep you posted.

@Incarnation-p-lee
Copy link

Not sure if below parts can transform to

  bits('n) -> (
    bits(1),
    bits(if 'n == 16 then 5 else (if 'n == 32 then 8 else 11)),
    bits(if 'n == 16 then 10 else (if 'n == 32 then 23 else 52)),
  )

=>

  bits('n) -> (
    bits(1),
    bits(match 'n {
       16 => 5,
       32 => 8,
       64 => 11,
    }),
    bits(if 'n == 16 then 10 else (if 'n == 32 then 23 else 52)),
  )

That would be more friendly for reading, specially when we add fp128 support.

@Incarnation-p-lee
Copy link

Incarnation-p-lee commented Apr 10, 2024

Not sure if below parts can transform to

  bits('n) -> (
    bits(1),
    bits(if 'n == 16 then 5 else (if 'n == 32 then 8 else 11)),
    bits(if 'n == 16 then 10 else (if 'n == 32 then 23 else 52)),
  )

=>

  bits('n) -> (
    bits(1),
    bits(match 'n {
       16 => 5,
       32 => 8,
       64 => 11,
    }),
    bits(if 'n == 16 then 10 else (if 'n == 32 then 23 else 52)),
  )

That would be more friendly for reading, specially when we add fp128 support.

Unfortunately not work for matches in bits now.
Another interest found for types, looks we can leverage tuple for arg nums > 1.

val feq_quiet : forall 'n, 'n in {16, 32, 64}. (bits('n), bits('n)) -> (bool, bits_fflags)
function feq_quiet(v1, v2) = { }

=>

type fp_bits_x2 = { 'n, 'n in {16, 32, 64}. (bits('n), bits('n)) }
val      feq_quiet : (fp_bits_x2) -> (bool, bits_fflags)
function feq_quiet (tuple) = {
  let (v1, v2) = tuple;
}

@aamartin0000
Copy link

Hello, switching on width alone may not be adequate for future-proofing. For example, there is already BF16 format along with "standard" FP16, for 16-bit width floats (see Zfbf). AI/ML will also be adding a variety of 8-, and possibly even 6- or 4-bit floats. (These may only be supported in the vector or matrix instruction sets, and may not even be IEEE754 compliant.) The formats we will support, which differ in the allocation of exponent vs. significand bits, has not yet been nailed down.

The point is, if you're changing this code, it may as well be done in a more generic fashion that will work with these future formats.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Apr 12, 2024

Hmm that is a good point. Unfortunately I don't think Sail supports type level enums so supporting multiple 16-bit float formats in this way is going to be a bit tricky.

Perhaps the best solution is to make all the functions generic over the split sizes, instead of the total size. Like instead of

function f_is_pos_zero(x : fbits) -> bool = {
  let (sign, exp, mant) = fsplit(x);
  (  (sign == zeros())
   & (exp  == zeros())
   & (mant == zeros()))
}

have

function f_is_pos_zero(sign : fsign, exp : fexp, mant : fmant) -> bool = {
  (  (sign == zeros())
   & (exp  == zeros())
   & (mant == zeros()))
}

Quite a bit more tedious to use though. Perhaps we should cross that bridge when we come to it. At the very worst those weird formats can copy & paste the implementations as they would have to now.

@aamartin0000
Copy link

Whatever you feel is best. I just wanted to make sure you were aware of the situation, so that any change you make now won't make it really hard to change later on when these new formats come into use.

You still need to figure out how to deal with FP16 vs BF16, and that could provide a template for all the other formats to be added. Rather than the width, maybe you could key off of opcode bits? Hardware decode logic has to do it this way.

@Timmmm Timmmm mentioned this pull request Apr 17, 2024
@Incarnation-p-lee
Copy link

Hi @Timmmm, Just FYI, the sail side accept the float bits representation in structure, more details can be located here.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Sep 16, 2024

Closing this since we can use the code included in Sail instead.

@Timmmm Timmmm closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants