[JSC] B3 reduce-double-to-float should reduce only when constant double is canonical one to reduced float value
Created attachment 425678 [details] Patch
<rdar://problem/76259599>
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.
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.
Committed r275800 (236371@main): <https://commits.webkit.org/236371@main>
(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.
%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
Fixed the warning in https://trac.webkit.org/changeset/275870/webkit