Bug 151921 – [JSC] Add minimal float support to B3

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Benjamin Poulain]

[JSC] Add minimal float support to B3

[Benjamin Poulain]

Created attachment 266727 [details]
Patch

[Benjamin Poulain]

First cut.

I disagree that arithmetic are a must have in the first patch. If that's a blocker for you, I'll add them. Most of the work will be writing the tests, not the operators.

[Filip Pizlo]

(In reply to comment #2)
> First cut.
> 
> I disagree that arithmetic are a must have in the first patch. If that's a
> blocker for you, I'll add them. Most of the work will be writing the tests,
> not the operators.

Comparisons, too. If it's done now then we will have fewer fixmes for later.

[Filip Pizlo]

Comment on attachment 266727 [details]
Patch

I think that this patch is a regression, because it introduces a type but implements only a small fraction of that type's intended functionality.  Worse, this is a type that we don't need right now or probably anytime soon.  The B3 is sufficiently complete that at this point, adding new types or new opcodes should be done in complete steps.  This is a good precedent to set: that way what is in trunk is always representative of what we think is a good compiler.

In this case, the only part of the patch that helps us reach a complete state is that it gives a replacement for FRound.  But you could have implemented that in a tiny standalone patch.

But what your patch implements is a partial implementation of Floats.  There is a risk that you will abandon this after landing the patch, since none of the remaining Float support is on anyone's critical path.  So, we're going to have to live with B3 having a half-baked Float type.  That will send a strong message to other contributors that it's OK to add half of something to B3 and then abandon it.  I'd like to avoid that.

I'm fine with this patch being extended to support all Float operations.  It will be a big patch, but that's OK.  If you think that full Float support should be a priority right now, then I recommend just finishing this patch.  The complete patch should include all float math ops that we support for doubles and all float comparisons, including Float = Select(Int, Float, Float).  I'm also fine with a patch that implements Floats by lowering them to Doubles in LowerMacros, but I bet that such a patch would actually be bigger due to all of the weird things it entails, like the weird casts you'd have to do to support Float = Select(Int, Float, Float).

To summarize, the B3 is sufficiently close to done that we should not regress its completeness.  I want B3 to quickly reach a state where everything makes sense.  Adding a type that can only be used for a subset of its intended operations means that we're further from that state.

[Benjamin Poulain]

We'll need this short-ish term anyway. It is not a time for cutting corners. I'll take next week to do the right thing and implement full support then.

[Benjamin Poulain]

*** This bug has been marked as a duplicate of bug 151974 ***