Re: [PATCH] AMD bdver2 processors 1/2 - BMI

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Jan 5, 2011 at 8:45 AM, Quentin Neill
<quentin.neill.gnu@gmail.com> wrote:
> On Tue, Jan 4, 2011 at 6:23 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Jan 4, 2011 at 12:58 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Tue, Jan 4, 2011 at 12:23 PM, Quentin Neill
>>> <quentin.neill.gnu@gmail.com> wrote:
>>>> On Tue, Dec 28, 2010 at 8:10 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Tue, Dec 28, 2010 at 5:53 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>> On Mon, Dec 20, 2010 at 2:55 PM, Quentin Neill
>>>>>> <quentin.neill.gnu@gmail.com> wrote:
>>>>>>> On Mon, Dec 20, 2010 at 4:39 PM, Sebastian Pop <sebpop@gmail.com> wrote:
>>>>>>>> On Mon, Dec 20, 2010 at 16:32, Quentin Neill
>>>>>>>> <quentin.neill.gnu@gmail.com> wrote:
>>>>>>>>> These two patches add support for BMI and TBM ISAs to be introduced in
>>>>>>>>> AMD bdver2 processors.
>>>>>>>>>
>>>>>>>>> The full encoding specification is delayed, however I have posted
>>>>>>>>> abbreviated specs on the gcc mailing list:
>>>>>>>>> BMI: http://gcc.gnu.org/ml/gcc-patches/2010-10/msg01766.html
>>>>>>>>> TBM: http://gcc.gnu.org/ml/gcc-patches/2010-10/msg01767.html
>>>>>>>>>
>>>>>>>>
>>>>>>>> Looks like your patch is reversed. ?Could you please send another one
>>>>>>>> that you get from git format-patch -1
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Sebastian
>>>>>>>
>>>>>>> Oops. ?Reposting with reversed patch. ?Thanks for reviewing Sebastian.
>>>>>>> --
>>>>>>> Quentin
>>>>>>>
>>>>>>
>>>>>> Please don't add ModrmRegExt, There are many examples in i386-opt.tbl
>>>>>> without ModrmRegExt.
>>>>>>
>>>>>>
>>>>>
>>>>> You should check i.tm.extension_opcode != None instead.
>>>>>
>>>>> --
>>>>> H.J.
>>>>
>>>> Fixed with the attached.
>>>> Tested and passes with "make check RUNTESTFLAGS=i386.exp".
>>>> Okay to commit?
>>>> --
>>>
>>> Comments on i386-dis.c:
>>>
>>> 1. For insns with VEX encoding, use XXX_VEX_0FXXXXX and sort them.
>>> 2. Use vex_len_table to handle invalid vector length.
>>> 3. Use VexGdq I just added instead of "{ OP_LWP_E, 0 }"
>>> 4. Properly add suffix with
>>>
>>> { "XXXS", ? ? ? ? ?{ Gdq, VexGdq, Edq } },
>>>
>>>
>>
>> Your BMI implementation has some issues:
>>
>> [hjl@gnu-6 i386]$ head -7 x86-64-bmi.s
>>
>> ? ? ? ?.allow_index_reg
>> ? ? ? ?.text
>>
>> _start:
>>
>> ? ?ANDN ? ? %eax,%r15d,%eax
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> [hjl@gnu-6 i386]$ head -9 x86-64-bmi.d
>> #objdump: -dw
>> #name: x86-64 BMI
>>
>> .*: +file format .*
>>
>> Disassembly of section .text:
>>
>> 0+ <_start>:
>> [ ? ? ? ]*[a-f0-9]+: ? ?c4 c2 00 f2 c0 ? ? ? ? ?andn ? %r8d,%r15d,%eax
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> Your assembler doesn't match your disassembler. I checked in
>> my BMI implementation:
>>
>> http://sourceware.org/ml/binutils/2011-01/msg00038.html
>>
>> Please double check.
>>
>> Thanks.
>>
>> --
>> H.J.
>>
>
> [Resending to the the list]
>
> I'll work on a TBM patch based on this BMI code.
>
> I looked at VEXNDS and VEXXDS encoding but didn't think modifying
> those code paths was the way to go.
>
> Looks okay to me, a couple of questions:
> In tc-i386.c why use ~0 as a sentinel instead of MAX_OPERANDS (for mem
> on line 5582, or vex_reg on line 5636)?

I used unsigned ~0 to check incorrectly initialized operand.
Setting it to MAX_OPERANDS may still lead to legal, but bad
operand number.

> In i386-dis.c how did you name REG_VEX_0F38F3, I don't see a 0x38
> anywhere in the encodings.

VEX opcode prefixes can be 0F, 0F38 and 0F3A.  0F38F3
is 0F38 F3.

> In i386-dis.c don't you need to order your REG_VEX enum and entry in
> the reg_table (move REG_VEX_0F38F3 above REG_VEX_0FAE):
> +++ b/opcodes/i386-dis.c
> @@ -598,8 +598,8 @@ enum
> ?REG_VEX_0F71,
> ?REG_VEX_0F72,
> ?REG_VEX_0F73,
> - ?REG_VEX_0FAE,
> ?REG_VEX_0F38F3,
> + ?REG_VEX_0FAE,
> ?REG_XOP_LWPCB,
> ?REG_XOP_LWP
> ?};

I have

  REG_VEX_0F73,
  REG_VEX_0FAE,
  REG_VEX_0F38F3,
  REG_XOP_LWPCB,
  REG_XOP_LWP

Please make sure your source is correct.


-- 
H.J.