std.math: Move pow function into submodule. by berni44 · Pull Request #7942 · dlang/phobos

berni44 · 2021-04-07T12:28:33Z

Moving pow to submodule seems to be a little bit more problematic, so I'm doing this in a single step. Let's see if it works...

dlang-bot · 2021-04-07T12:28:34Z

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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 + phobos#7942"

berni44 · 2021-04-07T13:17:25Z

Some windows computers fail with a unittest in std.internal.math.gammafunction.d. Here the log of the x64 version, which is more verbose:

core.exception.AssertError@std\internal\math\gammafunction.d(1053): unittest failure
----------------
0x00007FF71340BB12 in d_unittestp
0x00007FF7131D508B in std.internal.math.gammafunction.__unittest_L1024_C7 at D:\a\1\phobos\std\internal\math\gammafunction.d(1054)
0x00007FF7131D8F27 in void std.internal.math.gammafunction.__modtest()
0x00007FF71342F139 in int core.runtime.runModuleUnitTests().__foreachbody2(object.ModuleInfo*)
0x00007FF71341B9A7 in int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*))
0x00007FF713432794 in int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))).__foreachbody2(ref rt.sections_win64.SectionGroup)
0x00007FF7134326FF in int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*)))
0x00007FF71341B967 in int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*))
0x00007FF71342EF87 in runModuleUnitTests
0x00007FF713418D25 in void rt.dmain2._d_run_main2(char[][], ulong, extern (C) int function(char[][])*).runAll()
0x00007FF713418C7F in void rt.dmain2._d_run_main2(char[][], ulong, extern (C) int function(char[][])*).tryExec(scope void delegate())
0x00007FF713418A86 in d_run_main2
0x00007FF713404FB3 in d_run_main
0x00007FF7110F9122 in unittest._d_cmain!().main at D:\a\1\phobos\../druntime\import\core\internal\entrypoint.d(30)
0x00007FF7134F1378 in __scrt_common_main_seh at d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl(288)
0x00007FFAD01E84D4 in BaseThreadInitThunk
0x00007FFAD2221821 in RtlUserThreadStart
1/130 modules FAILED unittests
std\internal\math\gammafunction.d(1053): [unittest] unittest failure

Anyone an idea, what's the reason here?

thewilsonator · 2021-04-07T14:04:36Z

MS win has double == real and the gamma function tests in particular test more precision than they really should.
I take it this PR is a cut and paste job for the most part?

evaluate the failing assertion
feqrel(betaIncompleteInv(5, 10, 0.2000002972865658842L), 0.229121208190918L) >= real.mant_dig - 3 and see how far off it is.

berni44 · 2021-04-07T14:49:57Z

I take it this PR is a cut and paste job for the most part?

Aside from a few needed imports it's cut and paste...

berni44 · 2021-04-07T15:58:29Z

It's only one digit: .0...799 instead of .0...800.

Anyway I wonder, why the calculation changed. It shouldn't.

berni44 · 2021-04-07T16:54:09Z

eBay/tsv-utils fails too, due to the same problems as encountered with the gamma function.

IMHO, we should find out, why dmd produces different code here and fix that...

RazvanN7 · 2021-04-08T10:21:57Z

I wonder if might be possible that the compiler searches for pow in both druntime and std.math. Maybe these have different implementations and now since pow is moved from std.math, the compiler picks the one in druntime?

Also, there's the style checker:

std/internal/math/gammafunction.d(26:1)[warn]: A unittest should be annotated with at least @safe or @system
`

berni44 · 2021-04-08T11:28:23Z

Also, there's the style checker:

I'm aware of that, but I see no merit in starting the testers again, when the main issue is not fixed... ;-) (Ideally, the gammafunction should not be touched at all.)

RazvanN7 · 2021-04-08T11:35:29Z

Ok, I was just pointing that out :-"

ibuclaw · 2021-04-09T04:51:48Z

The compiler lowers ^^ operator to

static import std.math;
std.math.pow(x, y);

Hazarding a guess, but you might need to force that the std version takes precedent with:

alias pow = std.math.exponential.pow;

In the package module.

Ideally pow should really be in druntime though because it is doubles as a compiler-runtime interface, however the non-trivial template implementation makes it awkward to move without also privately copying a good chunk of meta and traits along with it.

berni44 · 2021-04-09T06:20:32Z

Hazarding a guess, but you might need to force that the std version takes precedent with:
alias pow = std.math.exponential.pow;
In the package module.

For round and trunc this didn't work. But maybe here it does. Let's try...

berni44 · 2021-04-09T07:00:11Z

For round and trunc this didn't work. But maybe here it does. Let's try...

No, unfortunately not...

ibuclaw · 2021-04-10T05:44:31Z

For round and trunc this didn't work. But maybe here it does. Let's try...

No, unfortunately not...

Another shot in the dark. Remove the public import of core.math from the std.math package.

In its place, use public selective imports in the appropriate submodules.

// e.g in std.math.exponential
public import core.math : pow;

An intentional side effect being future core.math intrinsics won't be implicitly pulled into std.math.

berni44 · 2021-04-10T06:10:23Z

Another shot in the dark. Remove the public import of core.math from the std.math package.

core.math isn't publically imported, only static. I moved it into the functions anyway to see, if this has any effect...

ibuclaw · 2021-04-10T06:56:59Z

Another shot in the dark. Remove the public import of core.math from the std.math package.

core.math isn't publically imported, only static. I moved it into the functions anyway to see, if this has any effect...

Oh, then I suppose it'll likely have no effect at all them, except if there're bugs in the import visibility system.

ibuclaw · 2021-04-10T08:02:48Z

core.math isn't publically imported, only static. I moved it into the functions anyway to see, if this has any effect...

Oh, then I suppose it'll likely have no effect at all them, except if there're bugs in the import visibility system.

Saying that though, it all seems to be green?

berni44 · 2021-04-10T10:31:38Z

Saying that though, it all seems to be green?

Unfortunately not. Still the same problem.

ibuclaw · 2021-04-10T13:41:02Z

@berni44 - I've managed to find some time to check this proper.

Compiling gammafunction.d on x86_64-linux using -vcg-ast and diff'd the generated dump before and after this patch. This difference in codegen has no relation to a change in this PR.

@@ -5473,7 +5474,8 @@ pow!(real, real)
 				}
 				x = -x;
 			}
-			return sign * exp2(yl2x(x, y));
+			real w = exp2(y * log2(x));
+			return sign * w;
 		}
 		return impl(x, y);
 	}

Looks like the culprit is that you forgot to define version = INLINE_YL2X; for DigitalMars platforms.

std/math/exponential.d

std/math/package.d

ibuclaw · 2021-04-10T14:02:28Z

Just realised I can commit the suggestions myself.

ibuclaw · 2021-04-10T15:59:28Z

New error:

D:\a\1\dmd\test\..\..\phobos\std\math\exponential.d(667): Error: `yl2x` cannot be interpreted at compile time, because it has no available source code
compilable\test5227.d(120): Error: cannot interpret `<error>` at compile time
compilable\test5227.d(120):        while evaluating `pragma(msg, powf)`

Which is correct, when LDC/GDC is building DMD, the resultant compiler can not interpret yl2x at compile-time.

ibuclaw · 2021-04-10T16:20:01Z

So this is blocked by the compiler now.
https://github.com/dlang/dmd/blob/e658df1cc3a1622dea8e3614a5c3d94742ebbf52/src/dmd/builtin.d#L89-L93

It looks only for module names ending with math (i.e: core.math, std.math), so it returns BUILTIN.unimpl so tries to interpret the body.

berni44 · 2021-04-10T16:27:46Z

So this is blocked by the compiler now.
https://github.com/dlang/dmd/blob/e658df1cc3a1622dea8e3614a5c3d94742ebbf52/src/dmd/builtin.d#L89-L93

It looks only for module names ending with math (i.e: core.math, std.math), so it returns BUILTIN.unimpl so tries to interpret the body.

Wilsonator pointed me to the same line in PR #7943.

ibuclaw · 2021-04-10T16:41:51Z

So this is blocked by the compiler now.
https://github.com/dlang/dmd/blob/e658df1cc3a1622dea8e3614a5c3d94742ebbf52/src/dmd/builtin.d#L89-L93
It looks only for module names ending with math (i.e: core.math, std.math), so it returns BUILTIN.unimpl so tries to interpret the body.

Wilsonator pointed me to the same line in PR #7943.

Validating a fix just now with compilable/test5227.d, it looks like fabs stopped being matched in #7941, but this wasn't noticed because the body just forwards to core.math.

ibuclaw · 2021-04-11T05:12:16Z

No longer blocked by compiler built-ins support. Looks like one remaining failure is due to missing symbols.

std/math/package.d

ibuclaw

One last nit, and this is good to go.

std/math/package.d

berni44 · 2021-04-11T17:14:18Z

Many thanks for your help!

berni44 requested a review from ibuclaw as a code owner April 7, 2021 12:28

berni44 force-pushed the math_move_pow branch from ee9aa35 to a236104 Compare April 10, 2021 06:06

ibuclaw reviewed Apr 10, 2021

View reviewed changes

std/math/exponential.d Outdated Show resolved Hide resolved

ibuclaw reviewed Apr 10, 2021

View reviewed changes

std/math/exponential.d Show resolved Hide resolved

ibuclaw reviewed Apr 10, 2021

View reviewed changes

std/math/package.d Show resolved Hide resolved

ibuclaw reviewed Apr 10, 2021

View reviewed changes

std/math/package.d Outdated Show resolved Hide resolved

ibuclaw force-pushed the math_move_pow branch from 408fa75 to d90d070 Compare April 10, 2021 14:02

ibuclaw added Merge:Blocked math labels Apr 10, 2021

ibuclaw mentioned this pull request Apr 10, 2021

dmd.builtin: Match any submodule of the std.math package dlang/dmd#12420

Merged

ibuclaw removed the Merge:Blocked label Apr 11, 2021

berni44 commented Apr 11, 2021

View reviewed changes

std/math/package.d Outdated Show resolved Hide resolved

berni44 force-pushed the math_move_pow branch from d90d070 to 7c23317 Compare April 11, 2021 10:38

ibuclaw requested changes Apr 11, 2021

View reviewed changes

std/math/package.d Outdated Show resolved Hide resolved

std.math: Move pow function into submodule.

2ed0a00

berni44 force-pushed the math_move_pow branch from 7c23317 to 2ed0a00 Compare April 11, 2021 17:10

ibuclaw approved these changes Apr 11, 2021

View reviewed changes

ibuclaw added the Merge:auto-merge label Apr 11, 2021

dlang-bot merged commit 59f78a2 into dlang:master Apr 12, 2021

ibuclaw mentioned this pull request Apr 15, 2021

remove use of INLINE_YL2X from std.math #7668

Open

berni44 mentioned this pull request Apr 15, 2021

Replace equalsDigit with isClose #7968

Merged

berni44 deleted the math_move_pow branch April 28, 2021 17:38

Uh oh!

Conversation

berni44 commented Apr 7, 2021

Uh oh!

dlang-bot commented Apr 7, 2021

Bugzilla references

Testing this PR locally

Uh oh!

berni44 commented Apr 7, 2021

Uh oh!

thewilsonator commented Apr 7, 2021

Uh oh!

berni44 commented Apr 7, 2021

Uh oh!

berni44 commented Apr 7, 2021

Uh oh!

berni44 commented Apr 7, 2021

Uh oh!

RazvanN7 commented Apr 8, 2021

Uh oh!

berni44 commented Apr 8, 2021

Uh oh!

RazvanN7 commented Apr 8, 2021

Uh oh!

ibuclaw commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berni44 commented Apr 9, 2021

Uh oh!

berni44 commented Apr 9, 2021

Uh oh!

ibuclaw commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berni44 commented Apr 10, 2021

Uh oh!

ibuclaw commented Apr 10, 2021

Uh oh!

ibuclaw commented Apr 10, 2021

Uh oh!

berni44 commented Apr 10, 2021

Uh oh!

ibuclaw commented Apr 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ibuclaw commented Apr 10, 2021

Uh oh!

ibuclaw commented Apr 10, 2021

Uh oh!

ibuclaw commented Apr 10, 2021

Uh oh!

berni44 commented Apr 10, 2021

Uh oh!

ibuclaw commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Apr 11, 2021

Uh oh!

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

berni44 commented Apr 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ibuclaw commented Apr 9, 2021 •

edited

Loading

ibuclaw commented Apr 10, 2021 •

edited

Loading

ibuclaw commented Apr 10, 2021 •

edited

Loading