Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Implement IL codes for #91, #93, fix #105 #104
Changes from 40 commits
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
There are no files selected for viewing
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 The
IsInt32StackFriendlyType
property means "Can place evaluation stack these types, that's compatible int32 type on CLR".Therefore use
IsInt32Type
,IsInt64Type
,IsIntPtrType
andIsUIntPtrType
. By the way, can useTargetType
directly onto decodeContext.PushStack(...) argument.We need regression test for not converter, require these topics:
int32
int64
andIntPtr
.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.
Thank you! This way, it will be much nicer. I am getting to know more and more of your code, and how parts are interacting with each other. I need to note, when I uncomment IntPtr unit tests, and also insert those into the IL, a UnitTestFramework fails to discover any tests, even though the produced DLL can be verified to be OK with DotPeek.
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 Likely
not
opcode's integer type limitation.ECMA-335: III.3.58 shl - shift integer left
section is written:shiftAmount (
si1
) tricky specification, it's exceptedint64
.We need regression test for
shl
andshr
converter, require these topics:shl
/shr
opcode each types int32 int64 and IntPtr combination with shiftAmount by int32 and IntPtr.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 have already added quite an amount of tests especially for shr & shr.un, as they were not that intuitive when they sign extend, and when not. I found this:
// Arithmetic Shift right (with sign)
// Warning!!! MSIL always operates for bit storage on Int32, or Int64
// and as it seems, smaller unsigned values (uint8 & uint16) will never
// be able to set the int32 highest bit => they will operate as logical shift right.
// On the other hand: uint32 and uint64 are still operated as arithmetic shift.
Will try to add the native int shift parameter as test, good point, thank you!
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.
Unfortunately I needed to comment them out again.
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.
OK, I will take a look on my environment, could you tell me explicit commit id reproducible?
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.
Is it valid on newest commit on branch
feature/implement-ilcodes
(ca5706c) ?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.
Commented: #104 (comment)
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...
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 The 'neg' opcode calculus is serious with floating point type...
ECMA-335 III.3.50 neg - negate
: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
NaN
value at all, so it seems that it's time to think about it....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 comment
The 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 comment
The 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)