Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(62)

Issue 93110044: code review 93110044: x86asm: instruction decoder (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 3 months ago by rsc
Modified:
8 years, 3 months ago
Reviewers:
crawshaw
Visibility:
Public.

Description

x86asm: instruction decoder

Patch Set 1 #

Patch Set 2 : diff -r 66b21e781a9a https://code.google.com/p/rsc.x86/ #

Total comments: 11

Patch Set 3 : diff -r 66b21e781a9a https://code.google.com/p/rsc.x86/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13869 lines, -2 lines) Patch
A x86asm/Makefile View 1 1 chunk +2 lines, -0 lines 0 comments Download
A x86asm/decode.go View 1 2 1 chunk +1493 lines, -0 lines 0 comments Download
A x86asm/decode_test.go View 1 1 chunk +63 lines, -0 lines 0 comments Download
M x86asm/inst.go View 1 2 chunks +48 lines, -2 lines 0 comments Download
A x86asm/tables.go View 1 1 chunk +9766 lines, -0 lines 0 comments Download
A x86asm/testdata/Makefile View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A x86asm/testdata/decode.txt View 1 1 chunk +2493 lines, -0 lines 0 comments Download

Messages

Total messages: 10
rsc
Hello crawshaw, I'd like you to review this change to https://code.google.com/p/rsc.x86/
8 years, 3 months ago (2014-05-08 03:34:54 UTC) #1
rsc
Should be obvious but decode.go is the only interesting file.
8 years, 3 months ago (2014-05-08 03:35:38 UTC) #2
crawshaw
LGTM Strangely like programming for a web browser. https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go File x86asm/decode.go (right): https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode247 x86asm/decode.go:247: var ...
8 years, 3 months ago (2014-05-08 11:39:48 UTC) #3
rsc
*** Submitted as https://code.google.com/p/rsc/source/detail?r=074e342086e6&repo=x86 *** x86asm: instruction decoder LGTM=crawshaw R=crawshaw https://codereview.appspot.com/93110044
8 years, 3 months ago (2014-05-08 13:55:24 UTC) #4
rsc
https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go File x86asm/decode.go (right): https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode247 x86asm/decode.go:247: var ( On 2014/05/08 11:39:48, crawshaw wrote: > If ...
8 years, 3 months ago (2014-05-08 14:00:16 UTC) #5
rsc
On Thu, May 8, 2014 at 7:39 AM, <crawshaw@golang.org> wrote: > Strangely like programming for ...
8 years, 3 months ago (2014-05-08 14:00:39 UTC) #6
rsc
https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go File x86asm/decode.go (right): https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373 x86asm/decode.go:373: case 0x67: On 2014/05/08 11:39:48, crawshaw wrote: > I ...
8 years, 3 months ago (2014-05-08 14:18:56 UTC) #7
crawshaw
https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go File x86asm/decode.go (right): https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373 x86asm/decode.go:373: case 0x67: On 2014/05/08 14:18:56, rsc wrote: > On ...
8 years, 3 months ago (2014-05-08 14:26:38 UTC) #8
rsc
https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go File x86asm/decode.go (right): https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373 x86asm/decode.go:373: case 0x67: At least on the copy I'm reading, ...
8 years, 3 months ago (2014-05-08 16:47:26 UTC) #9
crawshaw
8 years, 3 months ago (2014-05-08 16:51:22 UTC) #10
Message was sent while issue was closed.
https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go
File x86asm/decode.go (right):

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373
x86asm/decode.go:373: case 0x67:
On 2014/05/08 16:47:26, rsc wrote:
> At least on the copy I'm reading, the row for address size is
> 64,32,64,32,64,32,64,32. The first four are REX.W=0 and the last are REX.W=1.
> Both are 64,32,64,32, so REX.W doesn't actually influence address size.

Ack, I've been misreading the table. Phew.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b