Bug 224403 – [JSC] B3 reduce-double-to-float should reduce only when constant double is canonical one to reduced float value

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

[Yusuke Suzuki]

[JSC] B3 reduce-double-to-float should reduce only when constant double is canonical one to reduced float value

[Yusuke Suzuki]

Created attachment 425678 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/76259599>

[Mark Lam]

Comment on attachment 425678 [details]
Patch

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

r=me with suggestion.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1690
> +            // We cannot convert FloatToDouble(DoubleToFloat(value)) to value, because double-to-float will trancate some range of double values into one float.

/cannot convert/cannot convert some/

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1696
>              if (Value* constant = m_value->child(0)->floatToDoubleConstant(m_proc)) {

Currently, both floatToDoubleConstant() and doubleToFloatConstant() do unconditional conversions.  Since their results are always expected to be null checked before use, I recommend either:
1. Make them only return a non-null result if the conversion is lossless, or
2. Add ASSERTs in them that the conversion is lossless.

[Yusuke Suzuki]

Comment on attachment 425678 [details]
Patch

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

>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1690
>> +            // We cannot convert FloatToDouble(DoubleToFloat(value)) to value, because double-to-float will trancate some range of double values into one float.
> 
> /cannot convert/cannot convert some/

Changed.

>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1696
>>              if (Value* constant = m_value->child(0)->floatToDoubleConstant(m_proc)) {
> 
> Currently, both floatToDoubleConstant() and doubleToFloatConstant() do unconditional conversions.  Since their results are always expected to be null checked before use, I recommend either:
> 1. Make them only return a non-null result if the conversion is lossless, or
> 2. Add ASSERTs in them that the conversion is lossless.

This place is OK to lose bits since `FloatToDouble(constant)` is explicitly offered as an instruction. So, if it loses the bits, that's the intent of the program.
The above conversion's problem was shat replacement removed the original semantics.

static_cast<double>(static_cast<float>(1.1)) != 1.1


But this conversion is not losing the semantics since the original program's intent.

[Yusuke Suzuki]

Committed r275800 (236371@main): <https://commits.webkit.org/236371@main>

[Mark Lam]

(In reply to Yusuke Suzuki from comment #4)
> >> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1696
> >>              if (Value* constant = m_value->child(0)->floatToDoubleConstant(m_proc)) {
> > 
> > Currently, both floatToDoubleConstant() and doubleToFloatConstant() do unconditional conversions.  Since their results are always expected to be null checked before use, I recommend either:
> > 1. Make them only return a non-null result if the conversion is lossless, or
> > 2. Add ASSERTs in them that the conversion is lossless.
> 
> This place is OK to lose bits since `FloatToDouble(constant)` is explicitly
> offered as an instruction. So, if it loses the bits, that's the intent of
> the program.
> The above conversion's problem was shat replacement removed the original
> semantics.
> 
> static_cast<double>(static_cast<float>(1.1)) != 1.1
> 
> 
> But this conversion is not losing the semantics since the original program's
> intent.

Good point.

[Michael Catanzaro]

%llu assumes uint64_t is a long long unsigned int, but on Linux x86_64 it's long unsigned int. Solution is to use the PRIu64 format modifier:

[624/4867] Building CXX object Source/JavaScriptCore/CMak...vedSources/unified-sources/UnifiedSource-23a5fd0e-3.cpp.o
In file included from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-23a5fd0e-3.cpp:1:
../../Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp: In member function ‘virtual void JSC::B3::ConstDoubleValue::dumpMeta(WTF::CommaPrinter&, WTF::PrintStream&) const’:
../../Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:197:24: warning: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘long unsigned int’ [-Wformat=]
  197 |     out.printf("%le(%llu)", m_value, bitwise_cast<uint64_t>(m_value));
      |                     ~~~^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                        |                                   |
      |                        long long unsigned int              long unsigned int
      |                     %lu

[Yusuke Suzuki]

Fixed the warning in https://trac.webkit.org/changeset/275870/webkit