-
Notifications
You must be signed in to change notification settings - Fork 36
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
Implement IL codes for #91, #93, fix #105 #104
base: devel
Are you sure you want to change the base?
Changes from 1 commit
7ac3be8
a2c5f30
69cf5f0
e29d09e
e7fef2b
69e814f
663563c
4234755
19c92d9
f6bb406
9091c5b
bab32a0
80e54b5
c5ac124
e01dc9a
4833ba2
90bd0a3
05ececc
f98e142
df35a87
1a9cfe8
2a73440
e8029c1
e1b6cc4
30c0889
08ea06a
ff75c55
5453d39
8e3bac8
e0d75e0
1153f48
b549284
e43b30c
3c3a734
796f113
dd83236
73afbfb
a28587a
92ed390
76eeee7
2b7b595
3cf87a5
bc1476f
9753e44
4790411
ca5706c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -310,4 +310,46 @@ internal sealed class ShiftLeftConverter : ShiftConverter | |
|
||
public override ShiftDirection Direction => ShiftDirection.Left; | ||
} | ||
|
||
internal sealed class ShiftRightUnConverter : ShiftConverter | ||
{ | ||
public override OpCode OpCode => OpCodes.Shr_Un; | ||
|
||
public override ShiftDirection Direction => ShiftDirection.Right; | ||
} | ||
|
||
internal sealed class NegConverter : InlineNoneConverter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cyborgyn The 'neg' opcode calculus is serious with floating point type...
If we write the same expression in C language, I feel that the result may not be the same between C and IL (C#). Also, the current IL2C doesn't consider At this point, I think it's okay to include TODO comments and write with comment out regression test case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wrote ckfinite(float/double) opcode implementation in C to conform with these tests https://github.com/dotnet/runtime/blob/f9a2449/src/tests/JIT/IL_Conformance/Old/Base/ckfinite.il#L52. Here is the C version: https://godbolt.org/z/vxMYrvb8d passing all conformance tests. coreclr uses the same approach by checking if all exponent bits are 1 to find out if float/double number is infinite or NaN, and emits ArithmeticException. Was looking at the place to implement it in IL2C. Note: in this case, we need the raw single/double number in binary storage variable (e.g. uint32_t and uint64_t respectively) as it is represented in IL (IEEE 754 floating-point); without converting it to C float where it may or may not conform to IEEE 754 fp (ISO C99 is relaxed on the format...). Feel free to implement ckfinite too, as this one is pretty simple. Otherwise, I'll take a stab at it; once this PR is merged. 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good news! I will include NaN handler and ckfinite implementation when after merged this PR. (Memoized: #112) |
||
{ | ||
public override OpCode OpCode => OpCodes.Neg; | ||
|
||
public override ExpressionEmitter Prepare(DecodeContext decodeContext) | ||
{ | ||
var si0 = decodeContext.PopStack(); | ||
Metadata.ILocalVariableInformation result; | ||
|
||
if (si0.TargetType.IsByReference) | ||
throw new InvalidProgramSequenceException( | ||
"Invalid arithmetical NEG operation: Location={0}, Type0={1}", | ||
decodeContext.CurrentCode.RawLocation, | ||
si0.TargetType.FriendlyName); | ||
|
||
if (si0.TargetType.IsInt32StackFriendlyType) | ||
{ | ||
result = decodeContext.PushStack(decodeContext.PrepareContext.MetadataContext.Int32Type); | ||
} | ||
else if (si0.TargetType.IsInt64StackFriendlyType) | ||
{ // Int64 = -(Int64) | ||
result = decodeContext.PushStack(decodeContext.PrepareContext.MetadataContext.Int64Type); | ||
} | ||
else | ||
{ // double = -(double) | ||
result = decodeContext.PushStack(decodeContext.PrepareContext.MetadataContext.DoubleType); | ||
} | ||
|
||
return (extractContext, _) => new[] { string.Format( | ||
"{0} = -{1}", | ||
extractContext.GetSymbolName(result), | ||
extractContext.GetSymbolName(si0)) }; | ||
} | ||
} | ||
} |
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.
@cyborgyn Ouch, I forgot signed integer flag :)
shr
opcode will understand signed bit on MSB:It case, we can use C language shift operator with signed integer type (int32_t, int64_t and intptr_t).
But
shr.un
opcode:Strictly ignores MSB bit and insert zero.
ECMA-335 III.3.60 shr.un - shift integer right, unsigned
section:On IL2C side, The C language expression is having to write temporary casting unsigned integers explicitly, like:
We need regression test for shr.un (and shr) converter, require these topics:
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.
I did the temporary casting, if you look closely, and the tests are also walking through all of those tricky cases. One thing I forget, is negative shift (I never tried it, never even thought about it on any platform/language before). :) For the shift left, no magic happens, this is why it is omitted there.
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.
Sorry, I found it on branch latest commit. (I took and read commit from bottom of branch...)
I approved it!
Bit off topic: The CLR (.NET Framework CLR and Core CLR) behavior is funny. Because
shr
andshr.un
opcode behavior isn't defined with anyint32
,int64
andIntPtr
on ECMA-335.But real regression test on CLR is passed...