refactor Add()

[WalterBright]

I decided to separate out the refactoring of #14694 into this PR to make 14694 easier to deal with.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14695"

[RazvanN7]

Why do we wee need two identical functions with different names?

[WalterBright]

Because they become different in #14694.

[RazvanN7]

I don't see the point for these functions. e.toReal is much nicer to read, but if you insist on doing this you could just have 2 functions re and im that receive the expression as a parameter. Then you could simply do e1.re, e2.im etc.

[ibuclaw]

Walter's intention is to force round the result to double. However the approach is unclear, and I have the impression that the other PR will have to change rendering this refactor useless.

[WalterBright]

I tried several different ways, and this way made the use of them the easiest to read when used. The uses are all different and have to be carefully checked for being correct.

[ibuclaw]

Let's wait until there is a clear path in #14636 first.

For instance

• This does not appear that it will scale well to the other operations (are you really going to copy paste all of this boilerplate to sub/mul/div/pow?)
• Only touches CTFE (what about differences in run-time?)
• The other change affects current D semantics for evaluating floating point operations (can't see any separation between C11 and D2)
• Doesn't properly handle excess precision (first we need to ask the target what precision we should be evaluating the operation at before doing it - that code would look substantially different to what's going on here)
• The actual fix might even be something completely different (rounding CTFE values at this point during computation seems extraordinarily late, when assumedly there is a cast somewhere that we know gets ignored by CTFE - because enum d = cast(double)r; doesn't actually store d at double precision - the AST should look like cast(double)r1 + cast(double)r2, but knowing D it's probably cast(real)cast(double)r1 + cast(real)cast(double)r2).

[WalterBright]

It's a lot more readable than the previous version. It took me a while to figure out what was going on in that, this one is much easier to read and verify correctness.

This is about CTFE and has nothing to do with code gen. The code gen was fixed a while ago to round to float, the CTFE is a bit behind.

There shouldn't be separation between C11 and D, or at least it should be minimized.

This PR does not change the semantics at all. What it provides is an easy platform to focus on how to make addf and addd round the way we need it to without concern for the rest of the code.

I know it doesn't properly handle excess precision. This is a refactoring, not a fix. It produces identical results.

I don't know how the actual fix could be anything other than a rewrite of addf and addd. Which is most of the point of this refactoring - the problem gets isolated to two functions that are easy to understand.

I know that the front end never stores floats in any format other than real_t. This is not a problem since floats and doubles are exact subsets of real_t.

[WalterBright]

This does not appear that it will scale well to the other operations

One thing at a time. I don't like making gigantic PRs. This is a nice, self-contained PR, easy to review.

[WalterBright]

@ibuclaw you requested changes, but I don't know what those are.

[ibuclaw]

It's a lot more readable than the previous version. It took me a while to figure out what was going on in that, this one is much easier to read and verify correctness.

This is about CTFE and has nothing to do with code gen. The code gen was fixed a while ago to round to float, the CTFE is a bit behind.

I can see that, but I also notice x86-32 produces wrong code too - this would be related to the loss of excess precision (32-bit evaluates to precision of 80-bit real except on OSX).

There shouldn't be separation between C11 and D, or at least it should be minimized.

The spec couldn't be any more clear that it's different.

C11

Floating constants are evaluated to a format whose range and precision may be greater than required by the type. The use of evaluation formats is characterized by the implementation-defined value of # FLT_EVAL_METHOD.

D2

Regardless of the type of the operands, floating-point constant folding is done in real or greater precision.

Something needs to change, either keep with the spec, or change the spec - changing the spec would be a huge departure from your rejection of CTFE rounding at Dconf 2012/13 when Don demanded it.

Changing the behaviour now needs a clear deprecation path.

I know it doesn't properly handle excess precision. This is a refactoring, not a fix. It produces identical results.

I don't know how the actual fix could be anything other than a rewrite of addf and addd. Which is most of the point of this refactoring - the problem gets isolated to two functions that are easy to understand.

I would imagine:

1. Query target for the excess precision type of left and right hand side expression.
2. Round intermediate values to that type if less than current precision.
3. Perform operation.
4. Round the result (though maybe we can get away without doing this).

At no point do I forsee addf/addd being needed during any of those steps. I am more than willing to be proven wrong! :-)

[WalterBright]

This PR addresses none of those issues because it is a refactoring, with zero change in behavior.

at no point do I forsee addf/addd being needed

The rounding issue is 100% how addf and addd behave. It has nothing to do with the semantics of complex numbers. What happens with compile time (float + float), rounding and all, then becomes 100% how addf is implemented.

[ibuclaw]

This PR addresses none of those issues because it is a refactoring, with zero change in behavior.

at no point do I forsee addf/addd being needed

The rounding issue is 100% how addf and addd behave. It has nothing to do with the semantics of complex numbers. What happens with compile time (float + float), rounding and all, then becomes 100% how addf is implemented.

No, it's 100% how floating point is implemented, as e1.toReal() + e2.toReal() is always done at highest precision at compile-time (192-bits in gdc) regardless of what type the expression is fixed to (x + y -> x.opBinary!"+"(y)), so the general approach has to be on what rounding occurs around the operation, which doesn't care about whether it's an add/sub/mul/div/pow.

Focusing on the individual addf/addd looks to be a great way to get x86-centric results (and only 64-bit/SSE, not 32-bit/387!), but that ignores every other kind of system out there that might have different precision requirements for float op float or double op double.

[WalterBright]

it's 100% how floating point is implemented, as e1.toReal() + e2.toReal() is always done at highest precision at compile-time

Yes, because this is a refactoring and that is how it is done before and after this PR.

Focusing on the individual addf/addd looks to be a great way to get x86-centric results (and only 64-bit/SSE, not 32-bit/387!), but that ignores every other kind of system out there that might have different precision requirements for float op float or double op double.

This is a refactoring to put the dependency on floating point rounding into one function. It does not change how dmd rounds. Changing how dmd rounds add for float is then encapsulated in one function rather that being spread around the compiler.