-
Notifications
You must be signed in to change notification settings - Fork 317
tpbase: cgo-less amd routine decoders #602
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
Conversation
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 so much for working on this! Looks mostly good. Some comments. I realize the codeql seems to be just due to moved code, but perhaps you can fix those while doing this PR?
3f8fbb2
to
db3d452
Compare
ok, I will try to handle codeql complains |
7a4b096
to
ee2ca7d
Compare
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.
LGTM
tpbase/assembly_decode_x86.go
Outdated
offset := e.NewImmediateCapture("offset") | ||
expected := e.Mem8( | ||
e.Add( | ||
e.MemWithSegment8(x86asm.GS, e.NewImmediateCapture("")), | ||
offset, | ||
), | ||
) |
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: Same as the other comment: move offset
and expected
outside the for
block to reduce GC pressure by creating new variable instances for each step. Obviously the actual
and Match
call cannot move.
But is there any reason why expected
definition could not be moved and reused for each step?
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.
ah yes, now I understand. will do
This PR is the continuation of #447
Use the new
amd.Interpreter
instead of cgo decoders in thetpbase
package.