[PATCH v2] Support APX PUSHP/POPP

[PATCH v2] Support APX PUSHP/POPP

[Cui, Lili]

gas/ChangeLog:

	* config/tc-i386.c (process_operands): Handle "PUSHP/POPP requires
	rex2.w == 1."
	* testsuite/gas/i386/x86-64.exp: Add new test for PUSHP/POPP.
	* testsuite/gas/i386/x86-64-apx-pushp-popp-intel.d: New test.
	* testsuite/gas/i386/x86-64-apx-pushp-popp-inval.l: Ditto.
	* testsuite/gas/i386/x86-64-apx-pushp-popp-inval.s: Ditto.
	* testsuite/gas/i386/x86-64-apx-pushp-popp.d: Ditto.
	* testsuite/gas/i386/x86-64-apx-pushp-popp.s: Ditto.

opcodes/ChangeLog:

	* i386-dis.c (putop): print pushp and popp.
	* i386-opc.tbl: Added new insns.
	* i386-init.h : Regenerated.
	* i386-mnem.h : Regenerated.
	* i386-tbl.h: Regenerated.
---
 gas/config/tc-i386.c                          |  3 +-
 .../gas/i386/x86-64-apx-pushp-popp-intel.d    | 14 ++++++
 .../gas/i386/x86-64-apx-pushp-popp-inval.l    |  5 ++
 .../gas/i386/x86-64-apx-pushp-popp-inval.s    |  7 +++
 .../gas/i386/x86-64-apx-pushp-popp.d          | 14 ++++++
 .../gas/i386/x86-64-apx-pushp-popp.s          |  8 ++++
 gas/testsuite/gas/i386/x86-64.exp             |  3 ++
 opcodes/i386-dis.c                            | 48 ++++++++++++-------
 opcodes/i386-opc.h                            |  2 +
 opcodes/i386-opc.tbl                          |  3 ++
 10 files changed, 90 insertions(+), 17 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/x86-64-apx-pushp-popp-intel.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-apx-pushp-popp-inval.l
 create mode 100644 gas/testsuite/gas/i386/x86-64-apx-pushp-popp-inval.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-apx-pushp-popp.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-apx-pushp-popp.s

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index ca4432085be..0fe4162d7e9 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -3894,7 +3894,8 @@ is_apx_evex_encoding (void)
 static INLINE bool
 is_apx_rex2_encoding (void)
 {
-  return i.rex2 || i.rex2_encoding;
+  return i.rex2 || i.rex2_encoding
+    || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
 }
 
 static unsigned int
diff --git a/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-intel.d b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-intel.d
new file mode 100644
index 00000000000..44e3e96a5df
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-intel.d
@@ -0,0 +1,14 @@
+#as:
+#objdump: -dw -Mintel
+#name: x86_64 APX_F pushp popp insns (Intel disassembly)
+#source: x86-64-apx-pushp-popp.s
+
+.*: +file format .*
+
+Disassembly of section \.text:
+
+0+ <_start>:
+\s*[a-f0-9]+:\s*d5 08 50[	 ]+pushp  rax
+\s*[a-f0-9]+:\s*d5 19 57[	 ]+pushp  r31
+\s*[a-f0-9]+:\s*d5 08 58[	 ]+popp   rax
+\s*[a-f0-9]+:\s*d5 19 5f[	 ]+popp   r31
diff --git a/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-inval.l b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-inval.l
new file mode 100644
index 00000000000..c4d774b9673
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-inval.l
@@ -0,0 +1,5 @@
+.* Assembler messages:
+.*:4: Error: operand size mismatch for `pushp'
+.*:5: Error: operand size mismatch for `popp'
+.*:6: Error: operand size mismatch for `pushp'
+.*:7: Error: operand size mismatch for `popp'
diff --git a/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-inval.s b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-inval.s
new file mode 100644
index 00000000000..28ed5d8145a
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp-inval.s
@@ -0,0 +1,7 @@
+# Check bytecode of APX_F pushp popp instructions with illegal instructions.
+
+	.text
+	pushp %eax
+	popp  %eax
+	pushp (%rax)
+	popp  (%rax)
diff --git a/gas/testsuite/gas/i386/x86-64-apx-pushp-popp.d b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp.d
new file mode 100644
index 00000000000..b20e5ba9a35
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp.d
@@ -0,0 +1,14 @@
+#as:
+#objdump: -dw
+#name: x86_64 APX_F pushp popp insns
+#source: x86-64-apx-pushp-popp.s
+
+.*: +file format .*
+
+Disassembly of section \.text:
+
+0+ <_start>:
+\s*[a-f0-9]+:\s*d5 08 50[ 	]+pushp  %rax
+\s*[a-f0-9]+:\s*d5 19 57[ 	]+pushp  %r31
+\s*[a-f0-9]+:\s*d5 08 58[ 	]+popp   %rax
+\s*[a-f0-9]+:\s*d5 19 5f[ 	]+popp   %r31
diff --git a/gas/testsuite/gas/i386/x86-64-apx-pushp-popp.s b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp.s
new file mode 100644
index 00000000000..0ea66d0e70c
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-apx-pushp-popp.s
@@ -0,0 +1,8 @@
+# Check 64bit APX_F pushp popp instructions
+
+       .text
+ _start:
+	pushp %rax
+	pushp %r31
+	popp  %rax
+	popp  %r31
diff --git a/gas/testsuite/gas/i386/x86-64.exp b/gas/testsuite/gas/i386/x86-64.exp
index 8b41f9891a5..74433a20e24 100644
--- a/gas/testsuite/gas/i386/x86-64.exp
+++ b/gas/testsuite/gas/i386/x86-64.exp
@@ -348,6 +348,9 @@ run_dump_test "x86-64-avx512dq-rcigrne"
 run_dump_test "x86-64-apx-push2pop2"
 run_dump_test "x86-64-apx-push2pop2-intel"
 run_list_test "x86-64-apx-push2pop2-inval"
+run_dump_test "x86-64-apx-pushp-popp"
+run_dump_test "x86-64-apx-pushp-popp-intel"
+run_list_test "x86-64-apx-pushp-popp-inval"
 run_dump_test "x86-64-avx512dq-rcigru-intel"
 run_dump_test "x86-64-avx512dq-rcigru"
 run_dump_test "x86-64-avx512dq-rcigrz-intel"
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index b33b44d7c27..fb582226f5c 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -638,6 +638,9 @@ fetch_error (const instr_info *ins)
 #define AFLAG 2
 #define DFLAG 1
 
+/* {rex2} is not printed when the REX2_SPECIAL is set.  */
+#define REX2_SPECIAL 16
+
 enum
 {
   /* byte operand */
@@ -1931,23 +1934,23 @@ static const struct dis386 dis386[] = {
   { "dec{S|}",		{ RMeSI }, 0 },
   { "dec{S|}",		{ RMeDI }, 0 },
   /* 50 */
-  { "push{!P|}",		{ RMrAX }, 0 },
-  { "push{!P|}",		{ RMrCX }, 0 },
-  { "push{!P|}",		{ RMrDX }, 0 },
-  { "push{!P|}",		{ RMrBX }, 0 },
-  { "push{!P|}",		{ RMrSP }, 0 },
-  { "push{!P|}",		{ RMrBP }, 0 },
-  { "push{!P|}",		{ RMrSI }, 0 },
-  { "push{!P|}",		{ RMrDI }, 0 },
+  { "push!P",		{ RMrAX }, 0 },
+  { "push!P",		{ RMrCX }, 0 },
+  { "push!P",		{ RMrDX }, 0 },
+  { "push!P",		{ RMrBX }, 0 },
+  { "push!P",		{ RMrSP }, 0 },
+  { "push!P",		{ RMrBP }, 0 },
+  { "push!P",		{ RMrSI }, 0 },
+  { "push!P",		{ RMrDI }, 0 },
   /* 58 */
-  { "pop{!P|}",		{ RMrAX }, 0 },
-  { "pop{!P|}",		{ RMrCX }, 0 },
-  { "pop{!P|}",		{ RMrDX }, 0 },
-  { "pop{!P|}",		{ RMrBX }, 0 },
-  { "pop{!P|}",		{ RMrSP }, 0 },
-  { "pop{!P|}",		{ RMrBP }, 0 },
-  { "pop{!P|}",		{ RMrSI }, 0 },
-  { "pop{!P|}",		{ RMrDI }, 0 },
+  { "pop!P",		{ RMrAX }, 0 },
+  { "pop!P",		{ RMrCX }, 0 },
+  { "pop!P",		{ RMrDX }, 0 },
+  { "pop!P",		{ RMrBX }, 0 },
+  { "pop!P",		{ RMrSP }, 0 },
+  { "pop!P",		{ RMrBP }, 0 },
+  { "pop!P",		{ RMrSI }, 0 },
+  { "pop!P",		{ RMrDI }, 0 },
   /* 60 */
   { X86_64_TABLE (X86_64_60) },
   { X86_64_TABLE (X86_64_61) },
@@ -10621,6 +10624,19 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
 	case 'P':
 	  if (l == 0)
 	    {
+	      if (!cond && ins->last_rex2_prefix >= 0 && (ins->rex & REX_W))
+		{
+		  /* For pushp and popp, p is printed and do not print {rex2}
+		     for them.  */
+		  *ins->obufp++ = 'p';
+		  ins->rex2 |= REX2_SPECIAL;
+		  break;
+		}
+
+	      /* If "!p" prints nothing in intel_syntax.  */
+	      if (!cond && ins->intel_syntax)
+		break;
+
 	      if ((ins->modrm.mod == 3 || !cond)
 		  && !(sizeflag & SUFFIX_ALWAYS))
 		break;
diff --git a/opcodes/i386-opc.h b/opcodes/i386-opc.h
index 256f5a3865e..5a704740583 100644
--- a/opcodes/i386-opc.h
+++ b/opcodes/i386-opc.h
@@ -579,6 +579,8 @@ enum
   /* Instrucion requires that destination must be distinct from source
      registers.  */
 #define DISTINCT_DEST 9
+  /* Instrucion requires REX2 prefix.  */
+#define REX2_REQUIRED 10
   OperandConstraint,
   /* instruction ignores operand size prefix and in Intel mode ignores
      mnemonic size suffix check.  */
diff --git a/opcodes/i386-opc.tbl b/opcodes/i386-opc.tbl
index d1e4b8bf800..26d7ac329f5 100644
--- a/opcodes/i386-opc.tbl
+++ b/opcodes/i386-opc.tbl
@@ -85,6 +85,7 @@
 #define RegKludge         OperandConstraint=REG_KLUDGE
 #define SwapSources       OperandConstraint=SWAP_SOURCES
 #define Ugh               OperandConstraint=UGH
+#define Rex2              OperandConstraint=REX2_REQUIRED
 
 #define IgnoreSize	MnemonicSize=IGNORESIZE
 #define DefaultSize	MnemonicSize=DEFAULTSIZE
@@ -225,6 +226,7 @@ push, 0x68, i186&No64, DefaultSize|No_bSuf|No_sSuf|No_qSuf, { Imm16|Imm32 }
 push, 0x6, No64, DefaultSize|No_bSuf|No_sSuf|No_qSuf, { SReg }
 // In 64bit mode, the operand size is implicitly 64bit.
 push, 0x50, x64, No_bSuf|No_lSuf|No_sSuf|NoRex64, { Reg16|Reg64 }
+pushp, 0x50, APX_F, No_bSuf|No_wSuf|No_lSuf|No_sSuf|Rex2, { Reg64 }
 push, 0xff/6, x64, Modrm|DefaultSize|No_bSuf|No_lSuf|No_sSuf|NoRex64, { Reg16|Reg64|Word|Qword|Unspecified|BaseIndex }
 push, 0x6a, x64, DefaultSize|No_bSuf|No_lSuf|No_sSuf|NoRex64, { Imm8S }
 push, 0x68, x64, DefaultSize|No_bSuf|No_lSuf|No_sSuf|NoRex64, { Imm16|Imm32S }
@@ -238,6 +240,7 @@ pop, 0x8f/0, No64, Modrm|DefaultSize|No_bSuf|No_sSuf|No_qSuf, { Reg16|Reg32|Word
 pop, 0x7, No64, DefaultSize|No_bSuf|No_sSuf|No_qSuf, { SReg }
 // In 64bit mode, the operand size is implicitly 64bit.
 pop, 0x58, x64, No_bSuf|No_lSuf|No_sSuf|NoRex64, { Reg16|Reg64 }
+popp, 0x58, APX_F, No_bSuf|No_wSuf|No_lSuf|No_sSuf|Rex2, { Reg64 }
 pop, 0x8f/0, x64, Modrm|DefaultSize|No_bSuf|No_lSuf|No_sSuf|NoRex64, { Reg16|Reg64|Word|Qword|Unspecified|BaseIndex }
 pop, 0xfa1, x64, DefaultSize|No_bSuf|No_lSuf|No_sSuf|NoRex64, { SReg }
 
-- 
2.25.1

Re: [PATCH v2] Support APX PUSHP/POPP

[Jan Beulich]

On 07.12.2023 09:56, Cui, Lili wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -3894,7 +3894,8 @@ is_apx_evex_encoding (void)
>  static INLINE bool
>  is_apx_rex2_encoding (void)
>  {
> -  return i.rex2 || i.rex2_encoding;
> +  return i.rex2 || i.rex2_encoding
> +    || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;

Nit: Odd indentation.

> @@ -10621,6 +10624,19 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
>  	case 'P':
>  	  if (l == 0)
>  	    {
> +	      if (!cond && ins->last_rex2_prefix >= 0 && (ins->rex & REX_W))
> +		{
> +		  /* For pushp and popp, p is printed and do not print {rex2}
> +		     for them.  */
> +		  *ins->obufp++ = 'p';
> +		  ins->rex2 |= REX2_SPECIAL;

Apart from the two minor cosmetic remarks (one above, one below) the
dealing with unused REX2 bits is the main thing needing resolution. H.J.,
awaiting your input on that matter.

The other remark I have to make: Sending individual patches out of context
is certainly irritating, at least to me. In this submission you don't even
mention any of the dependencies. Yet this also extends to you sending new
versions of individual patches of the earlier 9-patch series. I understand
you're eager to make progress, but please also consider the recipients'
perspective.

> +		  break;
> +		}
> +
> +	      /* If "!p" prints nothing in intel_syntax.  */
> +	      if (!cond && ins->intel_syntax)
> +		break;

I'm afraid I can't make sense of the comment here. What is it that you're
trying to express?

Jan

RE: [PATCH v2] Support APX PUSHP/POPP

[Cui, Lili]

> On 07.12.2023 09:56, Cui, Lili wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -3894,7 +3894,8 @@ is_apx_evex_encoding (void)  static INLINE bool
> > is_apx_rex2_encoding (void)  {
> > -  return i.rex2 || i.rex2_encoding;
> > +  return i.rex2 || i.rex2_encoding
> > +    || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
> 
> Nit: Odd indentation.
> 

Sorry, I didn't find the indentation problem you mentioned.


> > @@ -10621,6 +10624,19 @@ putop (instr_info *ins, const char
> *in_template, int sizeflag)
> >  	case 'P':
> >  	  if (l == 0)
> >  	    {
> > +	      if (!cond && ins->last_rex2_prefix >= 0 && (ins->rex & REX_W))
> > +		{
> > +		  /* For pushp and popp, p is printed and do not print {rex2}
> > +		     for them.  */
> > +		  *ins->obufp++ = 'p';
> > +		  ins->rex2 |= REX2_SPECIAL;
> 
> Apart from the two minor cosmetic remarks (one above, one below) the
> dealing with unused REX2 bits is the main thing needing resolution. H.J.,
> awaiting your input on that matter.
> 
> The other remark I have to make: Sending individual patches out of context is
> certainly irritating, at least to me. In this submission you don't even mention
> any of the dependencies. Yet this also extends to you sending new versions of
> individual patches of the earlier 9-patch series. I understand you're eager to
> make progress, but please also consider the recipients'
> perspective.
> 

H.J is back and I just wanted to update our patch so he can see our latest version. Use this as a point to follow up on the APX patches, I mentioned in another email. Otherwise it would be inconvenient to have so many comments superimposed on the old patch.
No disrespect intended. This morning I sorted out the issues that were previously discussed but unresolved to facilitate follow-up.

> > +		  break;
> > +		}
> > +
> > +	      /* If "!p" prints nothing in intel_syntax.  */
> > +	      if (!cond && ins->intel_syntax)
> > +		break;
> 
> I'm afraid I can't make sense of the comment here. What is it that you're trying
> to express?

For example, if the instruction uses "!p", in the case of intel_syntax we will break.

  { "push!P",           { RMrAX }, 0 },
  { "push!P",           { RMrCX }, 0 },
  { "push!P",           { RMrDX }, 0 },

Thanks,
Lili.

Re: [PATCH v2] Support APX PUSHP/POPP

[Jan Beulich]

On 07.12.2023 15:26, Cui, Lili wrote:
>> On 07.12.2023 09:56, Cui, Lili wrote:
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -3894,7 +3894,8 @@ is_apx_evex_encoding (void)  static INLINE bool
>>> is_apx_rex2_encoding (void)  {
>>> -  return i.rex2 || i.rex2_encoding;
>>> +  return i.rex2 || i.rex2_encoding
>>> +    || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
>>
>> Nit: Odd indentation.
>>
> 
> Sorry, I didn't find the indentation problem you mentioned.

  return i.rex2 || i.rex2_encoding
         || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;

>>> @@ -10621,6 +10624,19 @@ putop (instr_info *ins, const char
>> *in_template, int sizeflag)
>>>  	case 'P':
>>>  	  if (l == 0)
>>>  	    {
>>> +	      if (!cond && ins->last_rex2_prefix >= 0 && (ins->rex & REX_W))
>>> +		{
>>> +		  /* For pushp and popp, p is printed and do not print {rex2}
>>> +		     for them.  */
>>> +		  *ins->obufp++ = 'p';
>>> +		  ins->rex2 |= REX2_SPECIAL;
>>
>> Apart from the two minor cosmetic remarks (one above, one below) the
>> dealing with unused REX2 bits is the main thing needing resolution. H.J.,
>> awaiting your input on that matter.
>>
>> The other remark I have to make: Sending individual patches out of context is
>> certainly irritating, at least to me. In this submission you don't even mention
>> any of the dependencies. Yet this also extends to you sending new versions of
>> individual patches of the earlier 9-patch series. I understand you're eager to
>> make progress, but please also consider the recipients'
>> perspective.
>>
> 
> H.J is back and I just wanted to update our patch so he can see our latest version. Use this as a point to follow up on the APX patches, I mentioned in another email. Otherwise it would be inconvenient to have so many comments superimposed on the old patch.
> No disrespect intended. This morning I sorted out the issues that were previously discussed but unresolved to facilitate follow-up.
> 
>>> +		  break;
>>> +		}
>>> +
>>> +	      /* If "!p" prints nothing in intel_syntax.  */
>>> +	      if (!cond && ins->intel_syntax)
>>> +		break;
>>
>> I'm afraid I can't make sense of the comment here. What is it that you're trying
>> to express?
> 
> For example, if the instruction uses "!p", in the case of intel_syntax we will break.
> 
>   { "push!P",           { RMrAX }, 0 },
>   { "push!P",           { RMrCX }, 0 },
>   { "push!P",           { RMrDX }, 0 },

So perhaps 'For "!P" print nothing else in Intel syntax.' ?

Jan

RE: [PATCH v2] Support APX PUSHP/POPP

[Cui, Lili]

> On 07.12.2023 15:26, Cui, Lili wrote:
> >> On 07.12.2023 09:56, Cui, Lili wrote:
> >>> --- a/gas/config/tc-i386.c
> >>> +++ b/gas/config/tc-i386.c
> >>> @@ -3894,7 +3894,8 @@ is_apx_evex_encoding (void)  static INLINE
> >>> bool is_apx_rex2_encoding (void)  {
> >>> -  return i.rex2 || i.rex2_encoding;
> >>> +  return i.rex2 || i.rex2_encoding
> >>> +    || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
> >>
> >> Nit: Odd indentation.
> >>
> >
> > Sorry, I didn't find the indentation problem you mentioned.
> 
>   return i.rex2 || i.rex2_encoding
>          || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
> 

I want to look for answers in existing cases and find two different uses. This is a small change, and I'll keep it consistent with your style.

Case 1:

static INLINE bool need_evex_encoding (void)
{
  return i.vec_encoding == vex_encoding_evex
        || i.vec_encoding == vex_encoding_evex512
        || i.mask.reg;
}


Case 2:
irf_operand (int op, const char *field)
{
  if (!field)
    {
      return op == IA64_OPND_RR_R3 || op == IA64_OPND_DBR_R3
        || op == IA64_OPND_IBR_R3  || op == IA64_OPND_PKR_R3
        || op == IA64_OPND_PMC_R3  || op == IA64_OPND_PMD_R3
        || op == IA64_OPND_MSR_R3 || op == IA64_OPND_CPUID_R3;
    }
...
}

Thanks,
Lili.

Re: [PATCH v2] Support APX PUSHP/POPP

[Jan Beulich]

On 08.12.2023 03:41, Cui, Lili wrote:
>> On 07.12.2023 15:26, Cui, Lili wrote:
>>>> On 07.12.2023 09:56, Cui, Lili wrote:
>>>>> --- a/gas/config/tc-i386.c
>>>>> +++ b/gas/config/tc-i386.c
>>>>> @@ -3894,7 +3894,8 @@ is_apx_evex_encoding (void)  static INLINE
>>>>> bool is_apx_rex2_encoding (void)  {
>>>>> -  return i.rex2 || i.rex2_encoding;
>>>>> +  return i.rex2 || i.rex2_encoding
>>>>> +    || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
>>>>
>>>> Nit: Odd indentation.
>>>>
>>>
>>> Sorry, I didn't find the indentation problem you mentioned.
>>
>>   return i.rex2 || i.rex2_encoding
>>          || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
>>
> 
> I want to look for answers in existing cases and find two different uses. This is a small change, and I'll keep it consistent with your style.
> 
> Case 1:
> 
> static INLINE bool need_evex_encoding (void)
> {
>   return i.vec_encoding == vex_encoding_evex
>         || i.vec_encoding == vex_encoding_evex512
>         || i.mask.reg;
> }
> 
> 
> Case 2:
> irf_operand (int op, const char *field)
> {
>   if (!field)
>     {
>       return op == IA64_OPND_RR_R3 || op == IA64_OPND_DBR_R3
>         || op == IA64_OPND_IBR_R3  || op == IA64_OPND_PKR_R3
>         || op == IA64_OPND_PMC_R3  || op == IA64_OPND_PMD_R3
>         || op == IA64_OPND_MSR_R3 || op == IA64_OPND_CPUID_R3;
>     }
> ...
> }

Funny you should pick out such an (imo obviously bad) example. If
this was the style to use, then things like

  if ((code == BFD_RELOC_64 || code == BFD_RELOC_64_PCREL)
      && GOT_symbol
      && fixp->fx_addsy == GOT_symbol)

would also need to be

  if ((code == BFD_RELOC_64 || code == BFD_RELOC_64_PCREL)
    && GOT_symbol
    && fixp->fx_addsy == GOT_symbol)

According to my understanding (and somewhat simplified), indentation
of the start of a statement is determined relative to what is before
it, whereas indentation within a (wrapped) statement is such that
respective expression parts match up (taking into account both real
and virtual parentheses), with operators placed at the start of the
(wrapped) lines. I'm not aware of any place where this is properly
written down, though. IOW my understanding in this regard may also
be entirely wrong.

Jan

RE: [PATCH v2] Support APX PUSHP/POPP

[Cui, Lili]

> On 08.12.2023 03:41, Cui, Lili wrote:
> >> On 07.12.2023 15:26, Cui, Lili wrote:
> >>>> On 07.12.2023 09:56, Cui, Lili wrote:
> >>>>> --- a/gas/config/tc-i386.c
> >>>>> +++ b/gas/config/tc-i386.c
> >>>>> @@ -3894,7 +3894,8 @@ is_apx_evex_encoding (void)  static INLINE
> >>>>> bool is_apx_rex2_encoding (void)  {
> >>>>> -  return i.rex2 || i.rex2_encoding;
> >>>>> +  return i.rex2 || i.rex2_encoding
> >>>>> +    || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
> >>>>
> >>>> Nit: Odd indentation.
> >>>>
> >>>
> >>> Sorry, I didn't find the indentation problem you mentioned.
> >>
> >>   return i.rex2 || i.rex2_encoding
> >>          || i.tm.opcode_modifier.operandconstraint == REX2_REQUIRED;
> >>
> >
> > I want to look for answers in existing cases and find two different uses. This is
> a small change, and I'll keep it consistent with your style.
> >
> > Case 1:
> >
> > static INLINE bool need_evex_encoding (void) {
> >   return i.vec_encoding == vex_encoding_evex
> >         || i.vec_encoding == vex_encoding_evex512
> >         || i.mask.reg;
> > }
> >
> >
> > Case 2:
> > irf_operand (int op, const char *field) {
> >   if (!field)
> >     {
> >       return op == IA64_OPND_RR_R3 || op == IA64_OPND_DBR_R3
> >         || op == IA64_OPND_IBR_R3  || op == IA64_OPND_PKR_R3
> >         || op == IA64_OPND_PMC_R3  || op == IA64_OPND_PMD_R3
> >         || op == IA64_OPND_MSR_R3 || op == IA64_OPND_CPUID_R3;
> >     }
> > ...
> > }
> 
> Funny you should pick out such an (imo obviously bad) example. If this was
> the style to use, then things like
> 
>   if ((code == BFD_RELOC_64 || code == BFD_RELOC_64_PCREL)
>       && GOT_symbol
>       && fixp->fx_addsy == GOT_symbol)
> 
> would also need to be
> 
>   if ((code == BFD_RELOC_64 || code == BFD_RELOC_64_PCREL)
>     && GOT_symbol
>     && fixp->fx_addsy == GOT_symbol)
> 
> According to my understanding (and somewhat simplified), indentation of the
> start of a statement is determined relative to what is before it, whereas
> indentation within a (wrapped) statement is such that respective expression
> parts match up (taking into account both real and virtual parentheses), with
> operators placed at the start of the
> (wrapped) lines. I'm not aware of any place where this is properly written
> down, though. IOW my understanding in this regard may also be entirely
> wrong.
> 

I don't think this is funny, I also found the same indentation as mine in gcc. Maybe they adjusted it using the same script as me. Anyway I don't want to spend any time on this.

Lili.