-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Spanify parts of IL reading in ILCompiler #97794
Conversation
* Make ILReader ref struct, only holding IL body bytes and a offset * Use BinaryPrimitives instead of manual bit-shifting * Remove unused ILStreamReader.cs * Make ReadMIbcGroup return List instead of using enumerator
* Remove ReaderExtensions.cs from ILCompiler.Ryujit
* Use ILReader.ReadBranchDestination to merge two switch branches
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, looks great!
Cc @jakobbotsch for IBC changes.
default: | ||
Debug.Fail("Unknown instruction"); | ||
break; | ||
} | ||
|
||
if (!isVariableSize) | ||
currentOffset += opcode.GetSize(); | ||
if (!hasReadOperand) |
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.
Nit: wasn't isVariableSize
better? We defaulted to false and had to set to true in one case. Now we still default to false, but need to set it to true in two cases.
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 thought about name of this for a while. I think the meaning of the variable changed because it's using ILReader
now.
Before we manually kept track of the currentOffset
where isVariableSize
was used to skip the ILOpcode.switch_
case in which the currentOffset
was calculated manually.
Now by using ILReader.Skip(opcode)
(which essentially is operandSize - op.GetSize()
+ switch handling) we can avoid this duplicated logic, however because we want to read the operand in two cases for stack height tracking purposes (branches and calls/newobj) we need to flip the bit to avoid "reading" the operand twice in ILReader
. That was why I called it hasReadOperand
.
tldr: the current branches that use the bool are not variable sized, switch was the only special case and we now handle it in ILReader.Skip(op)
This reverts commit f40fcac.
Came across some low-hanging fruits while browsing through ILCompiler and decided to try utilize some newer .NET APIs to make the code a bit more "modern". Shouldn't be much of a performance optimization though.
Commits
ILReader
ILReader
ref struct, only holding IL body bytes and a offsetBinaryPrimitives
instead of manual bit-shiftingILStreamReader.cs
ReadMIbcGroup
returnList<MethodProfileData>
instead of using enumerator, which prevented the use ofILReader
(now a ref struct)ILStackHelper
ILReader
instead to deduplicate reading logicILReader.ReadBranchDestination
Read7BitEncodedInt
ReaderExtensions.cs
fromILCompiler.Ryujit
. It wasn't used in the project.Had trouble running NativeAOT smoketests locally because my hardware doesn't support AVX512, which caused
nativeaot.SmokeTests\HardwareIntrinsics\X64Avx512
to fail.