search

StanfordPL / x64asm

x86-64 assembler library
Apache License 2.0
465 stars 60 forks source link

Should `cvtsd2si` be `cvtsd2sil` and `cvtsd2siq` #206

Closed stefanheule closed 8 years ago

stefanheule commented 8 years ago

The following entries in opcodes.names seem wrong:

, "cvtsd2si_r32_m64" // CVTSD2SI r32, m64
, "cvtsd2si_r32_xmm" // CVTSD2SI r32, xmm
, "cvtsd2si_r64_m64" // CVTSD2SI r64, m64
, "cvtsd2si_r64_xmm" // CVTSD2SI r64, xmm

I think the l and q postfixes are missing. For comparison, the v version of the instruction (and all other similar si2* or *2si instructions) have those:

, "vcvtsd2sil_r32_m64" // VCVTSD2SI r32, m64
, "vcvtsd2sil_r32_xmm" // VCVTSD2SI r32, xmm
, "vcvtsd2siq_r64_m64" // VCVTSD2SI r64, m64
, "vcvtsd2siq_r64_xmm" // VCVTSD2SI r64, xmm

Is this a bug in the spreadsheet, or just a weird inconsistency by whoever came up with the names?

stefanheule commented 8 years ago

Similarly, the following have a q postfix, but not a l postfix. Is this correct? Where would I look this up?

cvttsd2si
cvttsd2siq
vcvttsd2si
vcvttsd2siq

cvttss2si
cvttss2siq
vcvttss2si
vcvttss2siq
bchurchill commented 8 years ago

idk what the situation is. It could be a spreadsheet issue or a codegen issue, but I'm not sure which. I'm also not sure if there's a reason they are/aren't this way. I believe the names should match the att memonic. Can you try assembling these was gas, disassembling with objdump and seeing if these names work or if they need to be changed?

On 10/10/2015 10:57 AM, Stefan Heule wrote:

Similarly, the following have a |q| postfix, but not a |l| postfix. Is this correct? Where would I look this up?

|cvttsd2si cvttsd2siq vcvttsd2si vcvttsd2siq

cvttss2si cvttss2siq vcvttss2si vcvttss2siq |

— Reply to this email directly or view it on GitHub https://github.com/StanfordPL/x64asm/issues/206#issuecomment-147112826.

stefanheule commented 8 years ago

Yes. GCC assembles all of these: cvtsd2si, cvtsd2sil, and cvtsd2siq (but only if the operands match the suffix, so cvtsd2siq %xmm0, %eax fails). If no suffix is used, then the operand is used to select the right opcode.

objdump spits out cvtsd2si %xmm0,%eax and cvtsd2siq %xmm0,%rax, i.e. it uses the suffix only for 64bits, and no suffix for 32 bits.

So, to me this sounds like we don't have a bug, but our spreadsheet is a bit inconsistent. If there are no objections, I will add the suffix to all instructions to make it consistent.

bchurchill commented 8 years ago

That may or may not be the right thing to do -- I'm honestly not sure. It could also be a bug in codegen where suffixes are added. I think the spreadsheet is more likely.

Berkeley

On 10/10/2015 06:10 PM, Stefan Heule wrote:

Yes. GCC assembles all of these: |cvtsd2si|, |cvtsd2sil|, and |cvtsd2siq| (but only if the operands match the suffix, so |cvtsd2siq %xmm0, %eax| fails). If no suffix is used, then the operand is used to select the right opcode.

objdump spits out |cvtsd2si %xmm0,%eax| and |cvtsd2siq %xmm0,%rax|, i.e. it uses the suffix only for 64bits, and no suffix for 32 bits.

So, to me this sounds like we don't have a bug, but our spreadsheet is a bit inconsistent. If there are no objections, I will add the suffix to all instructions to make it consistent.

— Reply to this email directly or view it on GitHub https://github.com/StanfordPL/x64asm/issues/206#issuecomment-147142775.

stefanheule commented 8 years ago

Yeah, the "bug" (really, it's more of an inconsistency than a bug) is in the spreadsheet. I will fix it.