[llvm-commits] [llvm] r139028 - in /llvm/trunk: lib/Target/X86/Disassembler/X86Disassembler.cpp lib/Target/X86/InstPrinter/X86ATTInstPrinter.cpp test/MC/Disassembler/X86/simple-tests.txt
Kevin Enderby
enderby at apple.com
Tue Sep 6 13:11:23 PDT 2011
Hi Evan & Eli,
I'd be happy to back this out and change it to the much better way using a type to distinguish between extending and non-extending immediates. My fear is breaking a bunch of "bad assembly code" that might exist and assembles today but is technically wrong.
As Eli pointed out their are many other instructions that really use an non-extended immediate. The problem is that bad assembly code like this:
pcmpestri $0x181, %xmm2, %xmm1
pcmpestri $-127, %xmm2, %xmm1
currently assembles in many cases. As I found out when I spent a lot of time auditing ALL the x86 instructions with immediates and seeing what the Mac OS X gas(1) assembler and the llvm-mc assembler will accept.
For the two line example above llvm-mc will assembly both of these lines and gas(1) will give a hard error only on the first. And there is also a class of instructions like in/out where the handling of this:
out %al, $0x181
produces only a warning with gas(1) but assembles and assembles with no warning with llvm-mc. Then you get to the class of instructions that are special cased in the patch like:
blendps $0x81, %xmm2, %xmm1
blendps $-127, %xmm2, %xmm1
Where gas will assemble both but llvm-mc will (I feel correctly) refuse to assemble the second. So the patch is a bit if a tight rope act and a workaround trying to only change the disassembler to match what the llvm-mc assembler currently allows.
I would love to tighten up all this in llvm-mc but with all the technically wrong intel assembly code out there being accepted by various assemblers I choose to only change the disassembler to address the specific problem with the lack of sign extension in disassembly the bug rdar://8795217 was asking for.
I welcome any suggestions and guidance in this area,
Kev
On Sep 5, 2011, at 2:52 PM, Evan Cheng wrote:
>
>
> On Sep 4, 2011, at 1:09 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>
>> On Fri, Sep 2, 2011 at 1:01 PM, Kevin Enderby <enderby at apple.com> wrote:
>>> Author: enderby
>>> Date: Fri Sep 2 15:01:23 2011
>>> New Revision: 139028
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=139028&view=rev
>>> Log:
>>> Change X86 disassembly to print immediates values as signed by default. Special
>>> case those instructions that the immediate is not sign-extend. radr://8795217
>>>
>>> Modified:
>>> llvm/trunk/lib/Target/X86/Disassembler/X86Disassembler.cpp
>>> llvm/trunk/lib/Target/X86/InstPrinter/X86ATTInstPrinter.cpp
>>> llvm/trunk/test/MC/Disassembler/X86/simple-tests.txt
>>>
>>> Modified: llvm/trunk/lib/Target/X86/Disassembler/X86Disassembler.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/Disassembler/X86Disassembler.cpp?rev=139028&r1=139027&r2=139028&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/X86/Disassembler/X86Disassembler.cpp (original)
>>> +++ llvm/trunk/lib/Target/X86/Disassembler/X86Disassembler.cpp Fri Sep 2 15:01:23 2011
>>> @@ -28,6 +28,8 @@
>>>
>>> #define GET_REGINFO_ENUM
>>> #include "X86GenRegisterInfo.inc"
>>> +#define GET_INSTRINFO_ENUM
>>> +#include "X86GenInstrInfo.inc"
>>> #include "X86GenEDInfo.inc"
>>>
>>> using namespace llvm;
>>> @@ -184,6 +186,38 @@
>>> break;
>>> }
>>> }
>>> + // By default sign-extend all X86 immediates based on their encoding.
>>> + else if (type == TYPE_IMM8 || type == TYPE_IMM16 || type == TYPE_IMM32 ||
>>> + type == TYPE_IMM64) {
>>> + uint32_t Opcode = mcInst.getOpcode();
>>> + switch (operand.encoding) {
>>> + default:
>>> + break;
>>> + case ENCODING_IB:
>>> + // Special case those X86 instructions that use the imm8 as a set of
>>> + // bits, bit count, etc. and are not sign-extend.
>>> + if (Opcode != X86::BLENDPSrri && Opcode != X86::BLENDPDrri &&
>>> + Opcode != X86::PBLENDWrri && Opcode != X86::MPSADBWrri &&
>>> + Opcode != X86::DPPSrri && Opcode != X86::DPPDrri &&
>>> + Opcode != X86::INSERTPSrr && Opcode != X86::VBLENDPSYrri &&
>>> + Opcode != X86::VBLENDPSYrmi && Opcode != X86::VBLENDPDYrri &&
>>> + Opcode != X86::VBLENDPDYrmi && Opcode != X86::VPBLENDWrri &&
>>> + Opcode != X86::VMPSADBWrri && Opcode != X86::VDPPSYrri &&
>>> + Opcode != X86::VDPPSYrmi && Opcode != X86::VDPPDrri &&
>>> + Opcode != X86::VINSERTPSrr)
>>
>> This can't possibly be the complete list of instructions with a
>> non-extended immediate. PCMPESTRI and friends, SHUFPS and friends,
>> all shift instructions, and IN/OUT (plus possibly others I've missed)
>> all take an imm8 which is clearly not signed.
>
> It's also the wrong way to fix the bug. Kevin, can you add a new type to distinguish between extending and non-extending immediates?
>
> Evan
>
>>
>> -Eli
>>
>>> + type = TYPE_MOFFS8;
>>> + break;
>>> + case ENCODING_IW:
>>> + type = TYPE_MOFFS16;
>>> + break;
>>> + case ENCODING_ID:
>>> + type = TYPE_MOFFS32;
>>> + break;
>>> + case ENCODING_IO:
>>> + type = TYPE_MOFFS64;
>>> + break;
>>> + }
>>> + }
>>>
>>> switch (type) {
>>> case TYPE_MOFFS8:
>>>
>>> Modified: llvm/trunk/lib/Target/X86/InstPrinter/X86ATTInstPrinter.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/InstPrinter/X86ATTInstPrinter.cpp?rev=139028&r1=139027&r2=139028&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/X86/InstPrinter/X86ATTInstPrinter.cpp (original)
>>> +++ llvm/trunk/lib/Target/X86/InstPrinter/X86ATTInstPrinter.cpp Fri Sep 2 15:01:23 2011
>>> @@ -90,7 +90,8 @@
>>> if (Op.isReg()) {
>>> O << '%' << getRegisterName(Op.getReg());
>>> } else if (Op.isImm()) {
>>> - O << '$' << Op.getImm();
>>> + // Print X86 immediates as signed values.
>>> + O << '$' << (int64_t)Op.getImm();
>>>
>>> if (CommentStream && (Op.getImm() > 255 || Op.getImm() < -256))
>>> *CommentStream << format("imm = 0x%llX\n", (long long)Op.getImm());
>>>
>>> Modified: llvm/trunk/test/MC/Disassembler/X86/simple-tests.txt
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/Disassembler/X86/simple-tests.txt?rev=139028&r1=139027&r2=139028&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/test/MC/Disassembler/X86/simple-tests.txt (original)
>>> +++ llvm/trunk/test/MC/Disassembler/X86/simple-tests.txt Fri Sep 2 15:01:23 2011
>>> @@ -102,3 +102,59 @@
>>>
>>> # CHECK: vmovapd %xmm0, %xmm2
>>> 0xc5 0xf9 0x28 0xd0
>>> +
>>> +# Check X86 immediates print as signed values by default. radr://8795217
>>> +# CHECK: andq $-16, %rsp
>>> +0x48 0x83 0xe4 0xf0
>>> +
>>> +# Check these special case instructions that the immediate is not sign-extend.
>>> +# CHECK: blendps $129, %xmm2, %xmm1
>>> +0x66 0x0f 0x3a 0x0c 0xca 0x81
>>> +
>>> +# CHECK: blendpd $129, %xmm2, %xmm1
>>> +0x66 0x0f 0x3a 0x0d 0xca 0x81
>>> +
>>> +# CHECK: pblendw $129, %xmm2, %xmm1
>>> +0x66 0x0f 0x3a 0x0e 0xca 0x81
>>> +
>>> +# CHECK: mpsadbw $129, %xmm2, %xmm1
>>> +0x66 0x0f 0x3a 0x42 0xca 0x81
>>> +
>>> +# CHECK: dpps $129, %xmm2, %xmm1
>>> +0x66 0x0f 0x3a 0x40 0xca 0x81
>>> +
>>> +# CHECK: dppd $129, %xmm2, %xmm1
>>> +0x66 0x0f 0x3a 0x41 0xca 0x81
>>> +
>>> +# CHECK: insertps $129, %xmm2, %xmm1
>>> +0x66 0x0f 0x3a 0x21 0xca 0x81
>>> +
>>> +# CHECK: vblendps $129, %ymm2, %ymm5, %ymm1
>>> +0xc4 0xe3 0x55 0x0c 0xca 0x81
>>> +
>>> +# CHECK: vblendps $129, (%rax), %ymm5, %ymm1
>>> +0xc4 0xe3 0x55 0x0c 0x08 0x81
>>> +
>>> +# CHECK: vblendpd $129, %ymm2, %ymm5, %ymm1
>>> +0xc4 0xe3 0x55 0x0d 0xca 0x81
>>> +
>>> +# CHECK: vblendpd $129, (%rax), %ymm5, %ymm1
>>> +0xc4 0xe3 0x55 0x0d 0x08 0x81
>>> +
>>> +# CHECK: vpblendw $129, %xmm2, %xmm5, %xmm1
>>> +0xc4 0xe3 0x51 0x0e 0xca 0x81
>>> +
>>> +# CHECK: vmpsadbw $129, %xmm2, %xmm5, %xmm1
>>> +0xc4 0xe3 0x51 0x42 0xca 0x81
>>> +
>>> +# CHECK: vdpps $129, %ymm2, %ymm5, %ymm1
>>> +0xc4 0xe3 0x55 0x40 0xca 0x81
>>> +
>>> +# CHECK: vdpps $129, (%rax), %ymm5, %ymm1
>>> +0xc4 0xe3 0x55 0x40 0x08 0x81
>>> +
>>> +# CHECK: vdppd $129, %xmm2, %xmm5, %xmm1
>>> +0xc4 0xe3 0x51 0x41 0xca 0x81
>>> +
>>> +# CHECK: vinsertps $129, %xmm3, %xmm2, %xmm1
>>> +0xc4 0xe3 0x69 0x21 0xcb 0x81
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list