Codereview.chromium.org won't let me log in, so I'll give the comments by mail only.
On Tue, Jul 14, 2009 at 11:09, <whe...@chromium.org> wrote: > LGTM, with comments. I can make the changes, if you want. It looks > like the test result changes for x64 were mainly made in an earlier > changelist. Yes, this just fixes the syntax, which were slightly off. If you can take over the CL, it would be great. :) > > http://codereview.chromium.org/155346/diff/1/3 > File src/x64/disasm-x64.cc (right): > > http://codereview.chromium.org/155346/diff/1/3#newcode509 > Line 509: int DisassemblerX64::PrintImmediate(byte* data, OperandSize > size) { > I would call the argument immediate_size, since the operand size of the > instruction can be larger than the size of the immediate data. The immediate size is determined from the Operand Size (which is determined by the REX.W and Operand Size prefixes or the opcode itself), but it is not the same. In this case, the argument *is* the operand size. The immediate size inference happens in this functions. > > http://codereview.chromium.org/155346/diff/1/3#newcode622 > Line 622: OperandSize op_size = byte_size_immediate ? BYTE_SIZE : > operand_size(); > immediate_size, not op_size. This is tricky. It's still the operand size if it's not the special case of a byte size immediate with a non-byte size operand size. But yes, in this case immediate_size is probably more correct (except that quadword isn't really the immediate size, it's interpreted to mean a sign extended double-word immediate). > > > http://codereview.chromium.org/155346/diff/1/3#newcode732 > Line 732: if (imm8 > 0) { > This seems wrong. Why aren't we testing the opcode to see if it encodes > using cl as the shift amount? Won't this guarantee that a shift of 0 > (or is that 64?) is disassembled wrong? We may not generate that, but > it may as well be disassembled correctly. Seems like a potential bug, yes. > > http://codereview.chromium.org/155346 > -- Lasse R.H. Nielsen / l...@chromium.org'Faith without judgement merely degrades the spirit divine' --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
