-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: UInt256: Divide, Lsh, Rsh, Exp, ExpMod, SubtractMod; Int256: Multiply - when in and out params are referenced to the same struct #33
base: master
Are you sure you want to change the base?
Conversation
Could you add test; to ensure doesn't regress? |
…56: Divide, Lsh, Exp, ExpMod, SubtractMod; Int256: Multiply
Added some tests - found much more bugs. I checked Nethermind - it is not affected |
@@ -933,7 +933,7 @@ private static bool SubtractImpl(in UInt256 a, in UInt256 b, out UInt256 res) | |||
|
|||
public void Subtract(in UInt256 b, out UInt256 res) => Subtract(this, b, out res); | |||
|
|||
public static void SubtractMod(in UInt256 a, in UInt256 b, in UInt256 m, out UInt256 res) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to remove in
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I made it intentionaly.
This will create a copy of a
.
So when you use it like
UInt256.SubtractMod(a, in b, out a)
and inside the method you reach
if (SubtractUnderflow(a, b, out res))
this line will not override a
(as a
and res
are the same) (and we need original value of a
later)
I could just made a copy of a
inside the method instead, but why do we need to pass it as reference and then just copy it?
Same situation you can find in
static void Exp(in UInt256 b, in UInt256 e, out UInt256 result)
where I removed a line UInt256 bs = b;
and change signature to
public static void Exp(UInt256 b, in UInt256 e, out UInt256 result)
Same in ExpMod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apple M1 Pro, .NET 7.0 : .NET 7.0.5 (7.0.523.17405), Arm64 RyuJIT AdvSIMD
Method | Mean | Error | StdDev | Ratio |
---|---|---|---|---|
SubtractMod_UInt256 | 5.878 ns | 0.0035 ns | 0.0029 ns | 1.00 |
SubtractMod_UInt256_withoutIn | 5.741 ns | 0.0039 ns | 0.0032 ns | 0.98 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if I will change
public static void Multiply(in Int256 a, in Int256 b, out Int256 res)
{
Int256 av = a, bv = b;
if (a.Sign < 0)
{
a.Neg(out av);
}
if (b.Sign < 0)
{
b.Neg(out bv);
}
UInt256.Multiply(av._value, bv._value, out UInt256 ures);
int aSign = a.Sign;
int bSign = b.Sign;
res = new Int256(ures);
...
to
public static void MultiplyWithoutIn(Int256 a, Int256 b, out Int256 res)
{
int aSign = a.Sign;
int bSign = b.Sign;
if (aSign < 0) a.Neg(out a);
if (bSign < 0) b.Neg(out b);
UInt256.Multiply(a._value, b._value, out UInt256 ures);
res = new Int256(ures);
...
(remove in
and Int256 av = a, bv = b;
)
this will give improvement too
Apple M1 Pro, .NET 7.0 : .NET 7.0.5 (7.0.523.17405), Arm64 RyuJIT AdvSIMD
Method | EnvironmentVariables | Mean | Ratio |
---|---|---|---|
Multiply_Int256 | Empty | 9.861 ns | 1.00 |
Multiply_Int256_withoutIn | Empty | 9.463 ns | 0.96 |
Multiply_Int256 | DOTNET_EnableHWIntrinsic=0 | 19.376 ns | 1.96 |
Multiply_Int256_withoutIn | DOTNET_EnableHWIntrinsic=0 | 18.132 ns | 1.84 |
last commit failed because this code gives different output in different OS
On Linux 22.04 (7.0.305) this will result in:
And on MacOS (7.0.203)
Is it expected behaviour? |
That sounds very weird |
fixes #32