Re: [PATCH 3/3] Enable Intel MOVDIRI, MOVDIR64B instructions.

["Jan Beulich" <JBeulich at suse dot com>]

>>> On 26.04.18 at 22:27, <hjl.tools@gmail.com> wrote:
> On Thu, Apr 26, 2018 at 12:49 PM, Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> wrote:
>> Intel has disclosed a set of new instructions for Tremont processor.
>> The spec is
>> 
> https://software.intel.com/en-us/intel-architecture-instruction-set-extension 
> s-programming-reference
>>
>> This patch enables Intel MOVDIRI, MOVDIR64B instructions.
>>
>> gas/
>>         * config/tc-i386.c (cpu_arch): Add .movdir, .movdir64b.
>>         (cpu_noarch): Likewise.
>>         (process_suffix): Add check for register size.
>>         * doc/c-i386.texi: Document movdiri, movdir64b.
>>         * testsuite/gas/i386/i386.exp: Run MOVDIR{I,64B} tests.
>>         * testsuite/gas/i386/movdir-intel.d: New test.
>>         * testsuite/gas/i386/movdir.d: Likewise.
>>         * testsuite/gas/i386/movdir.s: Likewise.
>>         * testsuite/gas/i386/movdir64b-reg.s: Likewise.
>>         * testsuite/gas/i386/movdir64b-reg.l: Likewise.
>>         * testsuite/gas/i386/x86-64-movdir-intel.d: Likewise.
>>         * testsuite/gas/i386/x86-64-movdir.d: Likewise.
>>         * testsuite/gas/i386/x86-64-movdir.s: Likewise.
>>         * testsuite/gas/i386/x86-64-movdir64b-reg.s: Likewise.
>>         * testsuite/gas/i386/x86-64-movdir64b-reg.l: Likewise.
>>
>> opcodes/
>>         * i386-dis.c (enum): Add PREFIX_0F38F8, PREFIX_0F38F9,
>>         MOD_0F38F8_PREFIX_2, MOD_0F38F9_PREFIX_0.
>>         (prefix_table): New instructions (see prefix above).
>>         (mod_table): New instructions (see prefix above).
>>         Add Gva macro and handling in OP_G.
>>         * i386-gen.c (cpu_flag_init): Add CPU_MOVDIRI_FLAGS,
>>         CPU_MOVDIR64B_FLAGS.
>>         (cpu_flags): Likewise.
>>         (opcode_modifiers): Add AddrPrefixOpReg.
>>         (i386_opcode_modifier): Likewise.
>>         * i386-opc.h (enum): Add CpuMOVDIRI, CpuMOVDIR64B.
>>         (i386_cpu_flags): Likewise.
>>         * i386-opc.tbl: Add movidir{i,64b}.
>>         * i386-init.h: Regenerate.
>>         * i386-tbl.h: Likewise.
>>
>> OK for trunk.
>>
>> Igor
> 
> OK.

Okay, I see this has been committed already, and the re-basing on top
of my changes earlier this week has gone completely wrong: You're
re-introducing attributes removed by 6e041cf4b0 (and you don't
mention any of this in the ChangeLog entry). This is a no-go imo
(the patch should first of all have been submitted in a form such that it
actually applies to the tip of master, and non-trivial re-basing should
have resulted in a v2 to be sent). Please revert, fix (including my
review comments), and then please allow at least a little bit of time for
people other than H.J. to look at it. It is intentional that I, except for
very obvious changes, never commit changes the same day I submit
them, even if I get approval right away. (To be honest, I'm even not
entirely comfortable committing the next day, but the chance of
getting comments from someone other than H.J. are [sadly] so slim
that this looks like an acceptable thing to do.)

Looking at that ChangeLog entry I also notice that

       * i386-gen.c (cpu_flag_init): Add CPU_MOVDIRI_FLAGS,
       CPU_MOVDIR64B_FLAGS.
       (cpu_flags): Likewise.

is wrong: cpu_flags has CpuMOVDIRI and CpuMOVDIR64B added,
which is different from cpu_flag_init. It's not unusual for me (and I
suppose also for others) to grep ChangeLog-s for specific identifiers
to figure out at roughly what time they've been introduced, and
perhaps derive just from the respective ChangeLog entry what
their purpose is.

Jan