Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add core.math.toPrec intrinsics #10654

Merged
merged 2 commits into from
Dec 20, 2019
Merged

Add core.math.toPrec intrinsics #10654

merged 2 commits into from
Dec 20, 2019

Conversation

ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Dec 10, 2019

Rebase of #7219.

Requires dlang/druntime#2864.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

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 fetch digger
dub run digger -- build "master + dmd#10654"

@thewilsonator
Copy link
Contributor

Dependant druntime PR merged.

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 11, 2019

Added back-end support for precision truncating/extending, based off CastExp. Let's see if it fixes the testsuite agrees with it...

@thewilsonator
Copy link
Contributor

Please also see #10656 which is a much better way to do intrinsics.

@ibuclaw ibuclaw force-pushed the to_prec branch 3 times, most recently from 9614e2d to b684c37 Compare December 11, 2019 07:41
@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 11, 2019

Apparently SIMD is broken, which means I must have messed up the intrinsic name array somehow...

src/dmd/builtin.d Outdated Show resolved Hide resolved
@ibuclaw ibuclaw force-pushed the to_prec branch 2 times, most recently from 6a0812a to 9074f1a Compare December 11, 2019 09:40
@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 11, 2019

Apparently SIMD is broken, which means I must have messed up the intrinsic name array somehow...

Fixed by moving OPtoPrec to the bottom of the OPER enum... which seems to suggest there's something fishing going on in the dmd backend (bad relative comparison somewhere?).

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 11, 2019

@thewilsonator - I'd prefer to give this a quick test/port on gdc (ppc64 more specifically) before committing, when/if this pull ever goes green.

@thewilsonator
Copy link
Contributor

Don't worry I won't pull this until at least #10654 (comment) is addressed ;)

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 11, 2019

OK, it works perfect with GDC. :-P

@ibuclaw ibuclaw force-pushed the to_prec branch 2 times, most recently from c89d594 to bcc4455 Compare December 12, 2019 15:19
@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 12, 2019

The azure failures likely down to casting from longdouble (soft 80-bit) to real (native 64-bit). There's practically zero need for toPrec!real to do anything really, I'm not even sure returning a new RealExp even makes sense.

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 12, 2019

@thewilsonator - I think we're all green now.

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 12, 2019

@don-clugston-sociomantic - does this sufficiently satisfy https://issues.dlang.org/show_bug.cgi?id=9937 ?

Example usage of it, where double precision is forced at specific checkpoints in the CTFE code is found here.
https://github.com/dlang/druntime/blob/f7d2bbd2952ec644bf0ce7855195dc39760205aa/src/core/internal/convert.d#L74-L105

@don-clugston-sociomantic
Copy link
Contributor

Finally! This is long overdue!
The corresponding druntime PR says it "forces rounding of the argument f to the precision of the specified floating point type T"

It does not say which form of rounding is used. Since the purpose of this to eradicate machine-specific bits, the compiler can do it however it likes,
Suggested wording:
"The rounding is inevitably machine-dependent, but will be done in a way to maximize accuracy. In most cases round-to-nearest will be used."

Not sure if 'machine-dependent' is the right word, but it's more than just CPU-dependent, it may depend on the OS as well.

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 13, 2019

It does not say which form of rounding is used. Since the purpose of this to eradicate machine-specific bits, the compiler can do it however it likes,

At runtime, the codegen is indistinguishable from a cast().

During CTFE, dmd stores the value as the requested type, which I imagine would be just loading it in the appropriate register. GDC and probably LDC will truncate or extend the representation so that it fits the target type.

Suggested wording:
"The rounding is inevitably machine-dependent, but will be done in a way to maximize accuracy. In most cases round-to-nearest will be used."

Not sure if 'machine-dependent' is the right word, but it's more than just CPU-dependent, it may depend on the OS as well.

It would depend on the default rounding mode for the type. Looking at the list of possible formats, there's only one that instead rounds to zero.

If the PR hasn't been merged, I can bundle in the comment in the second toPrec PR I made.

@thewilsonator
Copy link
Contributor

Is this good to go?

@don-clugston-sociomantic
Copy link
Contributor

Looks good to me.

@dlang-bot dlang-bot merged commit 1b0302c into dlang:master Dec 20, 2019
@ibuclaw ibuclaw deleted the to_prec branch December 21, 2019 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants