Re: [PATCH 10/10] Support Intel PREFETCHI

[Jan Beulich <jbeulich@suse.com>]

On 14.10.2022 11:12, Haochen Jiang wrote:
> From: "Cui, Lili" <lili.cui@intel.com>
> 
> gas/
> 	* NEWS: Add support for Intel PREFETCHI instruction.
> 	* config/tc-i386.c: Add prefetchi.
> 	* doc/c-i386.texi: Document .prefetchi, noprefetchi.
> 	* testsuite/gas/i386/i386.exp: Run PREFETCHI tests.
> 	* testsuite/gas/i386/x86-64-prefetchi-intel.d: New test.
> 	* testsuite/gas/i386/x86-64-prefetchi-inval-register.d: Likewise.
> 	* testsuite/gas/i386/x86-64-prefetchi-inval-register.s: Likewise.
> 	* testsuite/gas/i386/x86-64-prefetchi.d: Likewise.
> 	* testsuite/gas/i386/x86-64-prefetchi.s: Likewise.
> 
> opcodes/
> 	* i386-dis.c (MOD_0F18_REG_6): New.
> 	(MOD_0F18_REG_7): Ditto.
> 	(X86_64_MOD_0F18_REG_6): Ditto.
> 	(X86_64_MOD_0F18_REG_7): Ditto.
> 	(x86_64_table): Add X86_64_MOD_0F18_REG_6 and X86_64_MOD_0F18_REG_7.
> 	(mod_table): Add MOD_0F18_REG_6 and MOD_0F18_REG_7.
> 	(PREFETCHI_Fixup): New.
> 	* i386-gen.c (cpu_flag_init): Add CPU_PREFETCHI_FLAGS and
> 	CPU_ANY_PREFETCHI_FLAGS.
> 	(cpu_flags): Add CpuPREFETCHI.
> 	* i386-opc.h (CpuPREFETCHI): New.
> 	(i386_cpu_flags): Add cpuprefetchi.
> 	* i386-opc.tbl: Add Intel PREFETCHI instructions.
> 	* i386-init.h: Regenerated.
> 	* i386-tbl.h: Likewise.
> ---
>  gas/NEWS                                      |    2 +
>  gas/config/tc-i386.c                          |    4 +-
>  gas/doc/c-i386.texi                           |    4 +-
>  gas/testsuite/gas/i386/i386.exp               |    3 +
>  gas/testsuite/gas/i386/x86-64-lfence-load.d   |    2 +
>  gas/testsuite/gas/i386/x86-64-lfence-load.s   |    2 +
>  gas/testsuite/gas/i386/x86-64-lockbad-1.l     |  104 +-
>  gas/testsuite/gas/i386/x86-64-lockbad-1.s     |    4 +

As for earlier patches I question the additions here. The purpose of this
test (and its 32-bit counterpart) isn't to cover all insns not valid with
LOCK, but just forms of insns which _may_ allow for LOCK. (But yes, these
tests aren't really complete.)

Note also how your ChangeLog entry doesn't mention tests you're altering.
You're not required anymore to provide ChangeLog entries, but if you do I
think they will want to match the actual patch.

> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -1102,6 +1102,7 @@ static const arch_entry cpu_arch[] =
>    SUBARCH (wrmsrns, WRMSRNS, ANY_WRMSRNS, false),
>    SUBARCH (msrlist, MSRLIST, ANY_MSRLIST, false),
>    SUBARCH (amx_fp16, AMX_FP16, ANY_AMX_FP16, false),
> +  SUBARCH (prefetchi, PREFETCHI, ANY_PREFETCHI, false),
>  };

Once again - likely no need for ANY_PREFETCHI (but see question further
down).

> @@ -4522,7 +4523,8 @@ load_insn_p (void)
>      {
>        /* Anysize insns: lea, invlpg, clflush, prefetchnta, prefetcht0,
>  	 prefetcht1, prefetcht2, prefetchtw, bndmk, bndcl, bndcu, bndcn,
> -	 bndstx, bndldx, prefetchwt1, clflushopt, clwb, cldemote.  */
> +	 bndstx, bndldx, prefetchwt1, clflushopt, clwb, cldemote, prefetchit0
> +	 prefetchit1.  */
>        if (i.tm.opcode_modifier.anysize)
>  	return 0;

Rather than further increasing the comment volume (and hence making it
harder to recognize quickly what is or is not covered here), may I suggest
to fold all mentioning of prefetches here into a single "prefetch*"?

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-prefetchi-inval-register.d
> @@ -0,0 +1,13 @@
> +#as:
> +#objdump: -dw
> +#name: x86-64 PREFETCHI INVAL REGISTER insns
> +
> +.*: +file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+ <\.text>:
> +[ 	]*[a-f0-9]+:[ 	]0f 18 39[ 	]*nopl   \(%rcx\)
> +[ 	]*[a-f0-9]+:[ 	]0f 18 31[ 	]*nopl   \(%rcx\)
> +#pass
> diff --git a/gas/testsuite/gas/i386/x86-64-prefetchi-inval-register.s b/gas/testsuite/gas/i386/x86-64-prefetchi-inval-register.s
> new file mode 100644
> index 0000000000..550449a0c9
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-prefetchi-inval-register.s
> @@ -0,0 +1,9 @@
> +.text
> +        #prefetchit0 (%rcx) PREFETCHIT0/1 apply without RIP-relative addressing, should stay NOPs.
> +        .byte 0x0f
> +        .byte 0x18
> +        .byte 0x39
> +        #prefetchit1 (%rcx) PREFETCHIT1/1 apply without RIP-relative addressing, should stay NOPs.
> +        .byte 0x0f
> +        .byte 0x18
> +        .byte 0x31

This is the disassembler side test. An assembler side counterpart is
needed as well, which I assume will point out that you also need to
make another change to the assembler (to actually reject non-RIP-
relative addressing).

> @@ -1297,6 +1300,8 @@ enum
>    X86_64_0F01_REG_7_MOD_3_RM_6_PREFIX_1,
>    X86_64_0F01_REG_7_MOD_3_RM_6_PREFIX_3,
>    X86_64_0F01_REG_7_MOD_3_RM_7_PREFIX_1,
> +  X86_64_MOD_0F18_REG_6,
> +  X86_64_MOD_0F18_REG_7,

X86_64_0F18_REG_6_MOD_0 and X86_64_0F18_REG_7_MOD_0 respectively.

> @@ -4414,6 +4419,18 @@ static const struct dis386 x86_64_table[][2] = {
>      { "psmash",	{ Skip_MODRM }, 0 },
>    },
>  
> +  /* X86_64_MOD_0F18_REG_6 */
> +  {
> +    { "nopQ",		{ Ev }, 0 },
> +    { "prefetchit1",    { { PREFETCHI_Fixup, b_mode } }, 0 },
> +  },
> +
> +  /* X86_64_MOD_0F18_REG_7 */
> +  {
> +    { "nopQ",		{ Ev }, 0 },
> +    { "prefetchit0",    { { PREFETCHI_Fixup, b_mode } }, 0 },
> +  },

Nit: Please use consistent padding (tabs) after the first comma each.

> @@ -14021,3 +14048,18 @@ OP_Rounding (instr_info *ins, int bytemode, int sizeflag ATTRIBUTE_UNUSED)
>      }
>    oappend (ins, "sae}");
>  }
> +
> +static void
> +PREFETCHI_Fixup (instr_info *ins, int bytemode, int sizeflag)
> +{
> +  if (ins->modrm.mod != 0 || ins->modrm.rm != 5)
> +    {
> +      if (ins->intel_syntax)
> +	ins->mnemonicendp = stpcpy (ins->obuf, "nop   ");
> +      else
> +	ins->mnemonicendp = stpcpy (ins->obuf, "nopl  ");

Why "nopl"? There's no NP ahead of the opcode (and you also don't go
through prefix_table[]), so I expect operand size should be expressed
here correctly.

> --- a/opcodes/i386-gen.c
> +++ b/opcodes/i386-gen.c
> @@ -261,6 +261,8 @@ static initializer cpu_flag_init[] =
>      "CpuMSRLIST" },
>    { "CPU_AMX_FP16_FLAGS",
>      "CpuAMX_FP16" },
> +  { "CPU_PREFETCHI_FLAGS",
> +    "CpuPREFETCHI"},
>    { "CPU_IAMCU_FLAGS",
>      "Cpu186|Cpu286|Cpu386|Cpu486|Cpu586|CpuIAMCU" },
>    { "CPU_ADX_FLAGS",
> @@ -471,6 +473,8 @@ static initializer cpu_flag_init[] =
>      "CpuMSRLIST" },
>    { "CPU_ANY_AMX_FP16_FLAGS",
>      "CpuAMX_FP16" },
> +  { "CPU_ANY_PREFETCHI_FLAGS",
> +    "CpuPREFETCHI" },
>  };

Are there intended to be dependencies between these and the earlier
prefetch features?

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3345,3 +3345,10 @@ wrmsrlist, 0xf30f01c6, None, CpuMSRLIST|Cpu64, No_bSuf|No_wSuf|No_lSuf|No_sSuf|N
>  tdpfp16ps, 0xf25c, None, CpuAMX_FP16|Cpu64, Modrm|Vex128|Space0F38|VexVVVV=1|VexW0|SwapSources|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { RegTMM, RegTMM, RegTMM }
>  
>  // AMX-FP16 instructions end.
> +
> +// PREFETCHI instructions.
> +
> +prefetchit0, 0xf18, 0x7, CpuPREFETCHI|Cpu64, Modrm|Anysize|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { BaseIndex }
> +prefetchit1, 0xf18, 0x6, CpuPREFETCHI|Cpu64, Modrm|Anysize|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { BaseIndex }
> +
> +// PREFETCHI instructions end.

With the restriction to RIP-relative addressing I think a better form of
expressing such operands would be along the lines of CALL/JMP:

	prefetchit0 code_label
	...
code_label:

I think it should be suggested to those having defined the ISA extension
to at least permit assemblers to support this form (and then do so here,
along with the present forms, unless the doc was changed to _only_ allow
for the alternative form).

Jan