Re: [PATCH RESEND v2] Aarch64: Allow explicit size specifier for predicate operand of {sq, uq, }{incp, decp}

[Richard Sandiford <richard.sandiford@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]

"dongbo (E)" <dongbo4@huawei.com> writes:
> Hi, Richard.
>
> On 2022/9/30 22:54, Richard Sandiford wrote:
>> "dongbo (E)" <dongbo4@huawei.com> writes:
>>>
>>> We tried to put `S_H` in front of `NIL`:
>>>
>>> ```
>>>
>>>       #define OP_SVE_Vv_HSD                        \
>>> {                                                               \
>>>         QLF2(S_H,S_H),                                      \
>>>         QLF2(S_S,S_S),                                       \
>>>         QLF2(S_D,S_D),                                      \
>>>         QLF2(S_H,NIL),                                       \
>>>         QLF2(S_S,NIL),                                       \
>>>         QLF2(S_D,NIL),                                       \
>>>       }
>>>
>>> ```
>> Yeah, good point.  It should be in this order, like you say.
>>
>> The fixes you mention look correct to me.
>>
>>> But assembler will fail in `match_operands_qualifier` :(.
>>>
>>> ```
>>>
>>>       match_operands_qualifier (aarch64_inst *inst, bool update_p)
>>>       {
>>>           ...
>>>           if (!aarch64_find_best_match (...))
>>>           ...
>>>           if (inst->opcode->flags & F_STRICT)
>>>           {
>>>               /* Require an exact qualifier match, even for NIL
>>> qualifiers.  */
>>>               nops = aarch64_num_of_operands (inst->opcode);
>>>               for (i = 0; i < nops; ++i)
>>>                   if (inst->operands[i].qualifier != qualifiers[i])
>>>                       return false;
>>>           }
>>>       }
>>>
>>> ```
>> I think that's a misfeature of the F_STRICT handling.  Does it work
>> with the patch below?
>
> We cannot find the `tree-data-ref.cc` in binutils, it is a file in GCC?
>
> Is `0001-data-ref-Fix-ranges_maybe_overlap_p-test.patch` the patch you 
> meant to send?

Gah, no, sorry.

> We also found a way to fix the F_STRICT matching failure. See patch 
> below. :)
>
> The main point is to give another `find_best_match` try when we get a 
> qualifier mismatch for HSD operands.

I think it's simpler than that.  Here's the patch I meant to send
(which is more -s than +s).

Richard


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-aarch64-Tweak-handling-of-F_STRICT.patch --]
[-- Type: text/x-diff, Size: 2705 bytes --]

From f33351e591b3e1caed2611e4c2e5f7b470150fa7 Mon Sep 17 00:00:00 2001
From: Richard Sandiford <richard.sandiford@arm.com>
Date: Fri, 30 Sep 2022 12:34:59 +0100
Subject: [PATCH] aarch64: Tweak handling of F_STRICT
To: binutils@sourceware.org

Current F_STRICT qualifier checking is enforced after the fact
rather than as part of the match.  This makes it impossible to
have, e.g.:

   QLF2(S_D, S_D)
   QLF2(S_D, NIL)

in the same list.

opcodes/
	* aarch64-opc.c (aarch64_find_best_match): Handle F_STRICT here
	rather than...
	(match_operands_qualifier): ...here.
---
 opcodes/aarch64-opc.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/opcodes/aarch64-opc.c b/opcodes/aarch64-opc.c
index 1d4668a3fbd..a2882bdfaba 100644
--- a/opcodes/aarch64-opc.c
+++ b/opcodes/aarch64-opc.c
@@ -958,19 +958,19 @@ aarch64_find_best_match (const aarch64_inst *inst,
 	dump_match_qualifiers (inst->operands, qualifiers);
 #endif
 
-      /* Most opcodes has much fewer patterns in the list.
-	 First NIL qualifier indicates the end in the list.   */
-      if (empty_qualifier_sequence_p (qualifiers))
+      /* The first entry should be taken literally, even if it's an empty
+	 qualifier sequence.  (This matters for strict testing.)  In other
+	 positions an empty sequence acts as a terminator.  */
+      if (i > 0 && empty_qualifier_sequence_p (qualifiers))
 	{
-	  DEBUG_TRACE_IF (i == 0, "SUCCEED: empty qualifier list");
-	  if (i)
-	    found = 0;
+	  found = 0;
 	  break;
 	}
 
       for (j = 0; j < num_opnds && j <= stop_at; ++j, ++qualifiers)
 	{
-	  if (inst->operands[j].qualifier == AARCH64_OPND_QLF_NIL)
+	  if (inst->operands[j].qualifier == AARCH64_OPND_QLF_NIL
+	      && !(inst->opcode->flags & F_STRICT))
 	    {
 	      /* Either the operand does not have qualifier, or the qualifier
 		 for the operand needs to be deduced from the qualifier
@@ -1038,7 +1038,7 @@ aarch64_find_best_match (const aarch64_inst *inst,
 static int
 match_operands_qualifier (aarch64_inst *inst, bool update_p)
 {
-  int i, nops;
+  int i;
   aarch64_opnd_qualifier_seq_t qualifiers;
 
   if (!aarch64_find_best_match (inst, inst->opcode->qualifiers_list, -1,
@@ -1048,15 +1048,6 @@ match_operands_qualifier (aarch64_inst *inst, bool update_p)
       return 0;
     }
 
-  if (inst->opcode->flags & F_STRICT)
-    {
-      /* Require an exact qualifier match, even for NIL qualifiers.  */
-      nops = aarch64_num_of_operands (inst->opcode);
-      for (i = 0; i < nops; ++i)
-	if (inst->operands[i].qualifier != qualifiers[i])
-	  return false;
-    }
-
   /* Update the qualifiers.  */
   if (update_p)
     for (i = 0; i < AARCH64_MAX_OPND_NUM; ++i)
-- 
2.25.1