Remove uses of INLINE_YL2X from std.math #3310
Conversation
|
I need the autotester to report feedback, as it works for me locally. @braddr |
|
Due to whitespace and reformatting changes, this is nearly impossible to review. I can't tell what is substantive and what isn't without spending a great deal of time. |
|
@WalterBright Github can hide the whitespace changes from the diff, making review much easier. I don't think it's exposed in the UI anywhere, but you can enable it by adding Link for this pull: https://github.com/D-Programming-Language/phobos/pull/3310/files?w=1 |
|
Regardless of |
|
So, does the non-INLINE_YL2X branch generate the same code as the INLINE_YL2X branch with dmd? |
If you are asking, does the non-INLINE-YL2X branch compile down to the same assembly? Answer no. If you are asking performance? That will need to be tested. A brief look at an instruction reference manual tells me that the INLINE_YL2X branches will execute at least 63 ops and has a latency of at least 158 (clock ticks or cycles is not specified) [edit: could be substantially more depending on the CPU]. As for the non-INLINE_YL2X branch, that is difficult to say. Someone will need to find the most common path and work out if the accululated fsub/fadd/fmul/fdiv's account for less or more than a single fyl2x instruction. But rather than being too bogged down on such matters though, instead, you should be asking about comparison of results. Is non-INLINE_YL2X less or more accurate than INLINE_YL2X? |
|
Autotester can't test these unless this is pulled in: #3309 |
|
Until you fix Phobos builds, this is blocked. |
|
Probably we need relax gamma function unittests? |
|
#3309 was merged, however unittests fails. |
|
I have the same test passing with gdc. Could be that dmd is using an inline assembly code path that is producing subtly a different result from the generic version. But that is just speculation... |
Guess now it's unblocked. @WalterBright |
|
ping @WalterBright |
|
Who's idea was it to add |
|
my |
|
Assuming that the line of failure reported by the auto-tester is correct. https://github.com/D-Programming-Language/phobos/blob/b2f1987cb3ec2d34739a02e55bff19e4443c0b36/std/internal/math/gammafunction.d#L540 GDC does not have this problem, which could indicate an issue with DMD codegen. But then again, maybe not if the inline assembler is involved in producing one of the answers. |
|
Seems like it started to pass |
|
@ibuclaw rebase please |
|
From what I recall, spoke to Don about this and I think we uncovered a problem with the asm implementation of exp2, where it was less accurate than the generic version if it fell into the slow branch. I suspect that it's not helped by y2lx not being a very accurate instruction either. |
|
@ibuclaw what's the status of this? |
|
I'll mark it as blocked until the non-asm implementations become ctfe-able. In the meantime, someone should look at why the asm implementation for exp2 is less accurate. And remove / fix tests that check for this inaccuracy. |
|
Superseded by #7668 |
To initially get std.math bootstrapped onto being
pure @trusted nothrow, yl2x did have a purpose.Now though, its role is meaningless as we have long since had proper
pure @trusted nothrowimplementations. And if anything, it's now harmful since the introduction of dlang/dmd#4012 as it leaves it open to misuse.I'm not against having these functions as CTFE-able. I'm just against library maintainers leveraging (unwittingly, as it's not actually their fault if code seems to work for them) non-portable CTFE builtins.