Bug 151903 – [JSC] Add Float-Double conversion to B3, use it to implement fround()

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

[Benjamin Poulain]

[JSC] Add Float-Double conversion to B3, use it to implement fround()

[Benjamin Poulain]

Created attachment 266696 [details]
Patch

[Benjamin Poulain]

There were surprisingly few changes require to add Float :)

[Filip Pizlo]

Comment on attachment 266696 [details]
Patch

I like it!

[Filip Pizlo]

Comment on attachment 266696 [details]
Patch

Wait, part of what is going on here is that you're deferring implementing float opcodes in the macro assembler. This puts us in a weird state since it increases the amount of macro assembler work we will have to do in order to declare feature complete. Your patch is easy but you haven't established how much work the macro assembler will be. 

Its important for us to quickly get to the point where FTL and b3 are complete, and I fear like this could actually make it take longer to get there. One way to get around this is to have your patch implement some of the basic float operations to illustrate that it's not hard. 

Clearing r? while we consider this a bit.

[Benjamin Poulain]

(In reply to comment #4)
> Comment on attachment 266696 [details]
> Patch
> 
> Wait, part of what is going on here is that you're deferring implementing
> float opcodes in the macro assembler. This puts us in a weird state since it
> increases the amount of macro assembler work we will have to do in order to
> declare feature complete. Your patch is easy but you haven't established how
> much work the macro assembler will be. 
> 
> Its important for us to quickly get to the point where FTL and b3 are
> complete, and I fear like this could actually make it take longer to get
> there. One way to get around this is to have your patch implement some of
> the basic float operations to illustrate that it's not hard. 
> 
> Clearing r? while we consider this a bit.

I was not planning on adding float support on any other operation than those two right now.

Eventually we'll extend the MacroAssembler. Right now, the only opcode that can use the Float is FloatToDouble.

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 266696 [details]
> > Patch
> > 
> > Wait, part of what is going on here is that you're deferring implementing
> > float opcodes in the macro assembler. This puts us in a weird state since it
> > increases the amount of macro assembler work we will have to do in order to
> > declare feature complete. Your patch is easy but you haven't established how
> > much work the macro assembler will be. 
> > 
> > Its important for us to quickly get to the point where FTL and b3 are
> > complete, and I fear like this could actually make it take longer to get
> > there. One way to get around this is to have your patch implement some of
> > the basic float operations to illustrate that it's not hard. 
> > 
> > Clearing r? while we consider this a bit.
> 
> I was not planning on adding float support on any other operation than those
> two right now.
> 
> Eventually we'll extend the MacroAssembler. Right now, the only opcode that
> can use the Float is FloatToDouble.

Do you intend to add validation rules for that?  If not then we are increasing the number of things we will have to do before we finish B3. I don't like the idea of B3 being indefinitely incomplete, and so long as there are B3 constructs that pass validation but assert in lowering, we are in an incomplete state. 

I recommend either addibg lowering support for floats everywhere that we lower doubles, or just implementing Fround. Addibg float support will probably take some work - for exemple the compare operations will be super annoying - but it's better than making B3 appear to support something that we never actually use and can't really lower.

[Benjamin Poulain]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Comment on attachment 266696 [details]
> > > Patch
> > > 
> > > Wait, part of what is going on here is that you're deferring implementing
> > > float opcodes in the macro assembler. This puts us in a weird state since it
> > > increases the amount of macro assembler work we will have to do in order to
> > > declare feature complete. Your patch is easy but you haven't established how
> > > much work the macro assembler will be. 
> > > 
> > > Its important for us to quickly get to the point where FTL and b3 are
> > > complete, and I fear like this could actually make it take longer to get
> > > there. One way to get around this is to have your patch implement some of
> > > the basic float operations to illustrate that it's not hard. 
> > > 
> > > Clearing r? while we consider this a bit.
> > 
> > I was not planning on adding float support on any other operation than those
> > two right now.
> > 
> > Eventually we'll extend the MacroAssembler. Right now, the only opcode that
> > can use the Float is FloatToDouble.
> 
> Do you intend to add validation rules for that?  If not then we are
> increasing the number of things we will have to do before we finish B3. I
> don't like the idea of B3 being indefinitely incomplete, and so long as
> there are B3 constructs that pass validation but assert in lowering, we are
> in an incomplete state. 
> 
> I recommend either addibg lowering support for floats everywhere that we
> lower doubles, or just implementing Fround. Addibg float support will
> probably take some work - for exemple the compare operations will be super
> annoying - but it's better than making B3 appear to support something that
> we never actually use and can't really lower.

For now, the only opcode that should pass validation with a Float as input is "FloatToDouble".

I should change the few isFloat() to " == Double". It is an oversight, but I did not change isFloat() to return true for Float.

It is better to add Float early so that we can think about it when we make other constructs. It seems less painful than adding it later.
We don't have to add any support not required for FTL.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > Comment on attachment 266696 [details]
> > > > Patch
> > > > 
> > > > Wait, part of what is going on here is that you're deferring implementing
> > > > float opcodes in the macro assembler. This puts us in a weird state since it
> > > > increases the amount of macro assembler work we will have to do in order to
> > > > declare feature complete. Your patch is easy but you haven't established how
> > > > much work the macro assembler will be. 
> > > > 
> > > > Its important for us to quickly get to the point where FTL and b3 are
> > > > complete, and I fear like this could actually make it take longer to get
> > > > there. One way to get around this is to have your patch implement some of
> > > > the basic float operations to illustrate that it's not hard. 
> > > > 
> > > > Clearing r? while we consider this a bit.
> > > 
> > > I was not planning on adding float support on any other operation than those
> > > two right now.
> > > 
> > > Eventually we'll extend the MacroAssembler. Right now, the only opcode that
> > > can use the Float is FloatToDouble.
> > 
> > Do you intend to add validation rules for that?  If not then we are
> > increasing the number of things we will have to do before we finish B3. I
> > don't like the idea of B3 being indefinitely incomplete, and so long as
> > there are B3 constructs that pass validation but assert in lowering, we are
> > in an incomplete state. 
> > 
> > I recommend either addibg lowering support for floats everywhere that we
> > lower doubles, or just implementing Fround. Addibg float support will
> > probably take some work - for exemple the compare operations will be super
> > annoying - but it's better than making B3 appear to support something that
> > we never actually use and can't really lower.
> 
> For now, the only opcode that should pass validation with a Float as input
> is "FloatToDouble".

That's probably ok, but I don't see how that's better than implementing FRound and adding Float when we are ready. 

> 
> I should change the few isFloat() to " == Double". It is an oversight, but I
> did not change isFloat() to return true for Float.

That's a problem. I think that at a minimum the patch should be complete in this regard. 

> 
> It is better to add Float early so that we can think about it when we make
> other constructs. It seems less painful than adding it later.
> We don't have to add any support not required for FTL.

But we can always fix constructs later. I don't think it would be painful if we added them all later. 

I don't like the approach of having a type that is essentially unused except for FRound, since this feels like the sort of thing that we wouldn't want to make a habit of. In this case, either the MacroAssemlber support is easy in which case you can add it in this patch, or it's hard enough that it requires more thought in which case we should add it once we know how to. In particular I don't like having this Oops on each binop and unop since it's not actually enough to support floats (due to comparisons) and it is enough to be annoying when adding non-Float code.

[Filip Pizlo]

Another thought: this patch doesn't get rid of the Float-specific opcodes that we have only because of the lack of the Float type.  This patch still leaves StoreFloat and LoadFloat.

I think that if we add Float to the compiler, we should do it in one go.  We could do this at any time, even after adding more optimizations and functionality to the compiler.  For now, what we need is FRound.  We could add FRound directly without putting the IR in an inconsistent state.

[Benjamin Poulain]

(In reply to comment #9)
> Another thought: this patch doesn't get rid of the Float-specific opcodes
> that we have only because of the lack of the Float type.  This patch still
> leaves StoreFloat and LoadFloat.

This convinces me to extend this patch :)

We don't have LoadStore nor StoreFloat yet. This is gonna be so much easier with this. I'll add them too!

[Saam Barati]

Comment on attachment 266696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266696&action=review

> Source/JavaScriptCore/b3/B3Type.cpp:50
> +        out.print("Double");

Should be "Float"

[Saam Barati]

(In reply to comment #11)
> Comment on attachment 266696 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266696&action=review
> 
> > Source/JavaScriptCore/b3/B3Type.cpp:50
> > +        out.print("Double");
> 
> Should be "Float"

Oops for some reason I thought this landed already.

[Filip Pizlo]

(In reply to comment #10)
> (In reply to comment #9)
> > Another thought: this patch doesn't get rid of the Float-specific opcodes
> > that we have only because of the lack of the Float type.  This patch still
> > leaves StoreFloat and LoadFloat.
> 
> This convinces me to extend this patch :)
> 
> We don't have LoadStore nor StoreFloat yet. This is gonna be so much easier
> with this. I'll add them too!

Please look at the situation we have with comparisons, branches, and conditional moves. I think that adding he floats is a significant chunk of work, and it is not something we have to do anytime soon. JavaScript definitely doesn't need floats and WebAssembly is not something we are committed to doing in the near future. I think it's a higher priority to add things that the FTL needs. I also think that to really declare victory on FTL B3, we should have a complete story for B3. I don't want to ship B3 in a state where it has some types that are half-usable - like you'll crash if you use them in the wrong kind of comparison or math op. Now that B3 is starting to look finished, I think it's best if we don't add incomplete features.

[Filip Pizlo]

Thought about this more.  I think there are two good paths forward:

1) Elaborate on this patch and just add full Float support.  This might be the best path since you've already done probably about half the work.  The MacroAssembler support for floats is annoying to add, but it shouldn't be that much code.

2) Just implement FRound and leave Float for later.

Adding a complete implementation of Float will cover all of my concerns.  I want B3 to get to a point where it is complete, in the sense that all types are usable with all of the things that people would expect - like, I would expect that if there is a Float type then I can add, subtract, compare, etc. on Floats.  Right now we are almost there for the types that we support in trunk.  Adding an incomplete Float feature means that we are further from the goal of being complete.

[Benjamin Poulain]

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