fix issue 14767 - Support CTFE of BigInt under x86 #6029
Conversation
|
Thanks for your pull request, @bbasile! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla references
|
std/internal/math/biguintcore.d
Outdated
| @@ -52,6 +52,130 @@ import std.range.primitives; | |||
| import std.traits; | |||
|
|
|||
| private: | |||
|
|
|||
| // compile-time & run-time dipatchers to allows BigInt CTFE for 32 bit OS. | |||
There was a problem hiding this comment.
/for 32bit OS/d - it should work for all OS at CTFE?
There was a problem hiding this comment.
No but now it will likely be the case. Even if CTFE isn't a requirement i think that it's a problem when it works for an arch and not the other
| } | ||
| static import std.internal.math.biguintnoasm; |
There was a problem hiding this comment.
This now will lead to a bigger object size on x86 as this file will always be imported, but dmd isn't smart enough to lazy load local imports anyways. Besides if (__ctfe) isn't even static if, so imports within will always be loaded.
There was a problem hiding this comment.
Yeah i guess this is the counterpart of the trick used to avoid symbol conflicts.
There was a problem hiding this comment.
I was trying to think of various ways to work around this, but could not.
I wonder if there's a way to introduce a CTFE-only template, basically one that is not instantiated even if it's referred to, until the CTFE engine tries to execute it.
std/internal/math/biguintcore.d
Outdated
| pragma(inline, true) | ||
| uint multibyteAddSub(char op)(uint[] dest, const(uint)[] src1, const (uint)[] src2, uint carry) pure | ||
| { | ||
| if (__ctfe) |
There was a problem hiding this comment.
Does are the times where CTFE + static if would be really cool:
static if(__ctfe || isNotX86)
There was a problem hiding this comment.
Yes. I remember that Stefan said that it could be possible, so it may be true in the future.
std/internal/math/biguintcore.d
Outdated
| // compile-time & run-time dipatchers to allows BigInt CTFE for 32 bit OS. | ||
|
|
||
| pragma(inline, true) | ||
| uint multibyteAddSub(char op)(uint[] dest, const(uint)[] src1, const (uint)[] src2, uint carry) pure |
There was a problem hiding this comment.
Why did you specify the attributes manually? It's a template and DMD should infer the attributes automatically.
The best practice is to avoid specifying the attributes manually, because then future improvements will propagate.
ghost
closed this
ghost
reopened this
ghost
closed this
ghost
reopened this
|
I authorized CircleCi for my own stuff again (revoked last week because i didn't use them) and after reopening the status is reported. They have a bug at CircleCi, it seems that they don't make the difference between people own stuff and their PRs in foreign repo. |
Not for some reason. There are a couple of PRs with "auto-merge" in the queue. They receive a higher priority: Windows + MacOS build hosts are typically quite busy:
I still don't know why that's just happening with your account, but it seems like it works now. |
|
It works now because see my other comment., we've posted more or less at the same time. |
|
@wilzbach, can you accept as you stated before that if i
The pragma for inline guarantee that there's no performance regressions and they don't fail locally. |
std/internal/math/biguintcore.d
Outdated
| @@ -52,6 +52,130 @@ import std.range.primitives; | |||
| import std.traits; | |||
|
|
|||
| private: | |||
|
|
|||
| // compile-time & run-time dipatchers to allows BigInt CTFE for 32 bit systems. | |||
There was a problem hiding this comment.
typo: "to allow:
Also as I mentioned earlier, the dispatchers are more general than 32 bit, so either X86 or remove the last part of the sentence.
There was a problem hiding this comment.
Should be much better now, in no way someone will ever be confused about the role of the new functions.
|
Good news, i can make a direct path for X86_64, saving the time took to inline. |
|
And bad news, i have to remove the pragma for inlining. Actually not all the primitives can be inlined, meaning that the X86 version can be slightly slower with dmd-produced code (but not always): prologue + CALL + epilogue, maybe a few stack copies for the arrays passed as param (2*4 or 2*2*4 bytes, depending on the number of array used by the primitive). |
|
For performance-related issues, I'm tempted to say check with ldc/gdc where possible. Dmd's optimizer leaves much to be desired. |
|
Okay so this is still accepted. |
|
The latest version is better. It doesn't affect X86_64, which is widely used, it only slightly affects X86, less and less used. |
|
DAutoTest fails due to dlang/dub#1336 |
|
Thanks for the information buddy. |
ghost
deleted the
This allows to declare very big BigInt constants but has the same limitation as x86_64 BigInt CTFE.