[Refactoring][stdlib] Replace `_UInt128` with `UInt128` in `stdlib/builtin/_format_float.mojo`

[JoeLoser]

Review Mojo's priorities

• [x] I have read the roadmap and priorities and I believe this request falls within the priorities.

What is your request?

Replace _UInt128 type in stdlib/builtin/_format_float.mojo with the new UInt128 type from stdlib/builtin/simd.mojo.

What is your motivation for this change?

Simplifying/consolidating on existing types.

Any other details?

No response

[kasmith11]

If I understand this correctly, we want to import UInt128 from stdlib/builtin/simd.mojo and change all of the references to _UInt128 in stdlib/builtin/_format_float.mojo to UInt128 while also removing the _UInt128 struct that was created? @JoeLoser

[JoeLoser]

If I understand this correctly, we want to import UInt128 from stdlib/builtin/simd.mojo and change all of the references to _UInt128 in stdlib/builtin/_format_float.mojo to UInt128 while also removing the _UInt128 struct that was created? @JoeLoser

Yep, that's correct. Note you'll need to rework some client code I suspect that is using the existing _UInt128.__iadd__ method.

[kasmith11]

If I understand this correctly, we want to import UInt128 from stdlib/builtin/simd.mojo and change all of the references to _UInt128 in stdlib/builtin/_format_float.mojo to UInt128 while also removing the _UInt128 struct that was created? @JoeLoser

Yep, that's correct. Note you'll need to rework some client code I suspect that is using the existing _UInt128.__iadd__ method.

Perfect. I'll work through this and reach out if I have any questions.

[kasmith11]

@JoeLoser I've created a pull request for this here, but there needed to be a couple of adjustments to functions in order to account for the change to UInt128. With this in mind there may be a couple of considerations that I might have missed such as adjustments to test, coding standards, etc...

[soraros]

Hi @kasmith11, thanks for tackling this issue. I think it will take more than the "mechanical" changes you made in #4680 to resolve it. I don’t believe the tests need to be changed, and some of the existing modifications don't look entirely correct. Here's how I'd suggest we proceed:

• Implement a lossless conversion between _UInt128 and UInt128. This should be easy—just a memory.bitcast.
• Add functions on UInt128 to retrieve the high and low 64 bits. This is also straightforward: use .cast[uint64] or right-shift and cast.
• Swap in the new implementation in _umul128 and friends.

For now, I recommend (1) making each change in a separate commit, and (2) marking #4680 as a draft. Feel free to ping me if you need any help.

[kasmith11]

@soraros thank you for being willing to answer questions!

For the first bullet point, with _UInt128 being a struct that holds two UInt64 values named high and low would I be bitcasting the high and low values or casting the high and low into one UInt128 value.

Is it possible that I'd just need to add a function that returns high and low on UInt128 as suggested in bullet point two?

[soraros]

Answer to both questions: yes.

[kasmith11]

@soraros Hello again, after scoping this I have a couple of questions:

1. Being that UInt128 is an alias for a SIMD scalar SIMD[uint128, 1], in order to add a function that returns high and low bits I believe we would need to add a function to simd.mojo. Do you have any thoughts here? As this may have implications for other SIMD types?
2. Staying on the topic of high and low bits, functions that return UInt64 values do not require high and low 32 bits. Do you know if there is any reason behind continuing to utilize high and low 64 bits for UInt128?

[soraros]

1. You could introduce two private free functions, _uint128_low and _uint128_high, in _format.mojo, right?
2. I haven't studied the implementation fully, but we want the first pass of the migration to be as mechanical as possible. If you see any room for improvement or refactoring, let's handle it in a separate PR.

[kasmith11]

@soraros I think the idea for _uint128_low and _uint128_high are great! I'll take a first pass at those and push to my PR branch.

[kasmith11]

Hello @soraros, I've converted https://github.com/modular/modular/pull/4680 to a draft, reverted the previous changes, and took an initial attempt at _utin128_low and _uint128_high.

I haven't worried about implementing yet, so the commits only contains the two new functions.

[soraros]

Looks good. The next step would be to write a free function that constructs a proper UInt128 from a pair of UInt64s and migrate the cache_f64 table to use it.

[kasmith11]

@soraros cache_f64 now has _uint64_to_uint128 that takes a pair of UInt64's to construct a UInt128. We should be in a place where we can replace .high and .low with _uint128_low and _uint128_high? #4680

[soraros]

We should be in a place where we can replace .high and .low with _uint128_low and _uint128_high? #4680

I hope so, yeah. Please go ahead and try it.

[kasmith11]

@soraros I've replaced .low and .high, but I'm having some trouble understanding how our _uint128_low and _uint128_high will work with with _umul192_upper128. Any initial thoughts upon looking at it?

[soraros]

I think you can inline _UInt128.__iadd__.

[kasmith11]

I think you can inline _UInt128.__iadd__.

Done! #4680

[soraros]

@JoeLoser Since both PRs on this are stale, I'll take it.

[JoeLoser]

That would be great, thanks Sora!