Fix #4034: Encode -x as -1.0 * x instead of 0.0 - x. #4040
Conversation
|
@gzm0 Please pay particular attention to the last paragraph of the PR description. WDYT, should we use the alternative instead? (that would break code doing |
There was a problem hiding this comment.
Please pay particular attention to the last paragraph of the PR description. WDYT, should we use the alternative instead? (that would break code doing 0.0 - x in source code instead of breaking code doing -x in source code)
My understanding is that because the linker (optimizer and emitter) freely translated between 0.0 - x and -x likely we can find instances of both being wrong with SJS 1.0.1 and maybe even the exact semantic depending on whether the optimizer was on or not. So breaking both of them differently in 1.1.0 is IMO not a problem.
Therefore, I think we should do what is simplest, which is this PR.
| case (PreTransLit(FloatLiteral(0)), _) => | ||
| rhs | ||
| case (_, PreTransLit(FloatLiteral(_))) => | ||
| foldBinaryOp(Float_+, rhs, lhs) |
There was a problem hiding this comment.
These two are also incorrect? Or just not worth it anymore? (similar comments below).
There was a problem hiding this comment.
The first one is incorrect : +0.0f + -0.0f gives +0.0f, not rhs which is -0.0f.
The second one is not worth it anymore.
AFAICT, the previous linker always treated So at least I'm pretty sure that all uses of |
Hmmm... But then why is it observable (as stated in the bug report)? At least when constant folding, it seems the linker didn't treat Or am I misunderstanding what is happening here completely? |
|
Oh, you're right. The optimizer could indeed incorrectly constant-fold So yes, you're right, the optimizer has an impact on the validity of |
The result of `-x` is not the same as `0.0 - x` when `x` is `0.0`: it is -0.0 instead of +0.0. Therefore, encoding `-x` as `0.0 - x` was incorrect, as well as link-time optimizations that interpreted `0.0 - x` as `-x`. We now encode `-x` as `-1.0 * x` instead, which always give the correct result. We adapt the rules in the optimizer and in the emitter to recognize `-1.0 * x` as `-x`, instead of `0.0 - x`. In addition, we remove some wrong simplifications in the optimizer, that were assuming that `+` is associative for doubles, whereas it is not. All of the above also applies to floats. Only code compiled with the newer version of the compiler will be fixed. Attention! Code doing `-x` in source code and compiled with an earlier version of the compiler will be *broken* by this change, since it was encoded as `0.0 - x` in the IR, which used to be emitted as `-x` (fixing the wrong encoding) and now emitted as `0.0 - x` (which is correct given the IR, but incorrect given the original source code). An alternative would be to add a deserialization hack to deserialize `0.0 - x` as `-1.0 * x`.
sjrd
force-pushed
the
fix-double-negate-zero
branch
from
0d1aff3 to
40e323a
Compare
|
Added an |
|
Why |
|
Well, that would work too, apparently. It's clearly non-obvious, though (much less so than |
|
Usually, FADD/FSUB have fewer latency on contemporary CPUs than FMUL: https://www.agner.org/optimize/instruction_tables.pdf |
|
There's no need to care about that. It is compiled into |
Copy fix for scala-js/scala-js#4043 to successfully execute the tests. JSCodeGen was changed as in scala-js/scala-js#4040
Copy fix for scala-js/scala-js#4043 to successfully execute the tests. JSCodeGen was changed as in scala-js/scala-js#4040
JSCodeGen was changed according to scala-js/scala-js#4040
JSCodeGen was changed according to scala-js/scala-js#4040
The result of
-xis not the same as0.0 - xwhenxis0.0: it is -0.0 instead of +0.0. Therefore, encoding-xas0.0 - xwas incorrect, as well as link-time optimizations that interpreted0.0 - xas-x.We now encode
-xas-1.0 * xinstead, which always give the correct result. We adapt the rules in the optimizer and in the emitter to recognize-1.0 * xas-x, instead of0.0 - x.In addition, we remove some wrong simplifications in the optimizer, that were assuming that
+is associative for doubles, whereas it is not.All of the above also applies to floats.
Only code compiled with the newer version of the compiler will be fixed.
Attention! Code doing
-xin source code and compiled with an earlier version of the compiler will be broken by this change, since it was encoded as0.0 - xin the IR, which used to be emitted as-x(fixing the wrong encoding) and now emitted as0.0 - x(which is correct given the IR, but incorrect given the original source code). An alternative would be to add a deserialization hack to deserialize0.0 - xas-1.0 * x.