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

[Quentin Neill <quentin dot neill dot gnu at gmail dot com>]

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)?
In i386-dis.c how did you name REG_VEX_0F38F3, I don't see a 0x38
anywhere in the encodings.

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
 };
@@ -2755,13 +2755,6 @@ static const struct dis386 reg_table[][8] = {
    { MOD_TABLE (MOD_VEX_0F73_REG_6) },
    { MOD_TABLE (MOD_VEX_0F73_REG_7) },
  },
-  /* REG_VEX_0FAE */
-  {
-    { Bad_Opcode },
-    { Bad_Opcode },
-    { MOD_TABLE (MOD_VEX_0FAE_REG_2) },
-    { MOD_TABLE (MOD_VEX_0FAE_REG_3) },
-  },
  /* REG_VEX_0F38F3 */
  {
    { Bad_Opcode },
@@ -2769,6 +2762,13 @@ static const struct dis386 reg_table[][8] = {
    { PREFIX_TABLE (PREFIX_VEX_0F38F3_REG_2) },
    { PREFIX_TABLE (PREFIX_VEX_0F38F3_REG_3) },
  },
+  /* REG_VEX_0FAE */
+  {
+    { Bad_Opcode },
+    { Bad_Opcode },
+    { MOD_TABLE (MOD_VEX_0FAE_REG_2) },
+    { MOD_TABLE (MOD_VEX_0FAE_REG_3) },
+  },
  /* REG_XOP_LWPCB */
  {
    { "llwpcb", { { OP_LWPCB_E, 0 } } },
-- 
Quentin