public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Richard Sandiford <richard.sandiford@arm.com>
To: "dongbo \(E\)" <dongbo4@huawei.com>
Cc: Shaokun Zhang via Binutils <binutils@sourceware.org>,
	 Shaokun Zhang <zhangshaokun@hisilicon.com>,
	 Jingtao Cai <caijingtao@huawei.com>,
	 "Richard Earnshaw" <rearnsha@arm.com>,
	 Marcus Shawcroft <marcus.shawcroft@arm.com>,
	 Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH RESEND v2] Aarch64: Allow explicit size specifier for predicate operand of {sq, uq, }{incp, decp}
Date: Fri, 30 Sep 2022 15:54:12 +0100	[thread overview]
Message-ID: <mptbkqwoquj.fsf@arm.com> (raw)
In-Reply-To: <f05ac6b0-fd13-fd37-e228-c49e55526d44@huawei.com> (dongbo's message of "Wed, 28 Sep 2022 10:19:50 +0800")

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

"dongbo (E)" <dongbo4@huawei.com> writes:
> Hi, thank you all for the replies.
>
> On 2022/9/12 16:17, Richard Sandiford wrote:
>> Hi,
>>
>> Thanks for the patch and sorry for the slow reply.  For the record,
>> I'm also not an aarch64 maintainer, but Richard E and I were talking
>> about this last week.
>>
>> Shaokun Zhang via Binutils <binutils@sourceware.org> writes:
>>> From: Jingtao Cai <caijingtao@huawei.com>
>>>
>>> Omitting predicate size specifier in vector form of {sq, uq, }{decp, incp} is deprecated and will be prohibited in a future release of the aarch64,
>>> see https://developer.arm.com/documentation/ddi0602/2021-09/SVE-Instructions/DECP--vector---Decrement-vector-by-count-of-true-predicate-elements-.
>>>
>>> This allows explicit size specifier, e.g. `decp z0.h, p0.h`, for predicate operand of these SVE instructions.
>>> The exsiting behaviour of not requiring the specifier is preserved, but will be warned.
>>> And the disasembly is with the specifier with this patch.
>> The patch is definitely doing the right thing by the architecture
>> definition.  However, given that the old syntax has been in use for
>> a while, it might be quite disruptive to deprecate the old syntax
>> now in the GNU tools.  When Richard and I discussed the various ways
>> of coping with the deprecation in GCC, none of the options seemed
>> particularly appealing.
>>
>> It's extremely unlikely that the old syntax would be repurposed to mean
>> something else.  Therefore, Richard and I thought that it might be better
>> to continue to accept the old syntax alongside the new syntax in the GNU
>> tools, as a GNU extension to the official assembler spec, without the
>> deprecation message.  Sorry to raise this after so long, and after you'd
>> already done the work.
>
> OK, I think we can delete the warning to make this done.
>
>>> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
>>> index cb039d63eba3..794e2d24f528 100644
>>> --- a/opcodes/aarch64-tbl.h
>>> +++ b/opcodes/aarch64-tbl.h
>>> @@ -4200,6 +4200,7 @@ const struct aarch64_opcode aarch64_opcode_table[] =
>>>     _SVE_INSN ("decd", 0x04f0e400, 0xfff0fc00, sve_misc, 0, OP2 (Rd, SVE_PATTERN_SCALED), OP_SVE_XU, F_OPD1_OPT | F_DEFAULT(31), 0),
>>>     _SVE_INSNC ("dech", 0x0470c400, 0xfff0fc00, sve_misc, 0, OP2 (SVE_Zd, SVE_PATTERN_SCALED), OP_SVE_HU, F_OPD1_OPT | F_DEFAULT(31), C_SCAN_MOVPRFX, 0),
>>>     _SVE_INSN ("dech", 0x0470e400, 0xfff0fc00, sve_misc, 0, OP2 (Rd, SVE_PATTERN_SCALED), OP_SVE_XU, F_OPD1_OPT | F_DEFAULT(31), 0),
>>> +  _SVE_INSNC ("decp", 0x252d8000, 0xff3ffe00, sve_size_hsd, 0, OP2 (SVE_Zd, SVE_Pg4_5), OP_SVE_VV_HSD, 0, C_SCAN_MOVPRFX, 0),
>>>     _SVE_INSNC ("decp", 0x252d8000, 0xff3ffe00, sve_size_hsd, 0, OP2 (SVE_Zd, SVE_Pg4_5), OP_SVE_VU_HSD, 0, C_SCAN_MOVPRFX, 0),
>> I haven't tested whether this works, but rather than have two entries per
>> instruction, I think it'd be better to add a new OP_SVE_ macro that lists
>> both the qualified and unqualified forms.  The current naming scheme is:
>>
>> /* The naming convention for SVE macros is:
>>
>>     OP_SVE_<operands>[_<sizes>]*
>>
>>     <operands> contains one character per operand, using the following scheme:
>>
>>     - U: the operand is unqualified (NIL).
>>
>>     - [BHSD]: the operand has a S_[BHSD] qualifier and the choice of
>>       qualifier is the same for all variants.  This is used for both
>>       .[BHSD] suffixes on an SVE predicate or vector register and
>>       scalar FPRs of the form [BHSD]<number>.
>>
>>     - [WX]: the operand has a [WX] qualifier and the choice of qualifier
>>       is the same for all variants.
>>
>>     - [ZM]: the operand has a /[ZM] suffix and the choice of suffix
>>       is the same for all variants.
>>
>>     - V: the operand has a S_[BHSD] qualifier and the choice of qualifier
>>       is not the same for all variants.
>>
>>     - R: the operand has a [WX] qualifier and the choice of qualifier is
>>       not the same for all variants.
>>
>>     - P: the operand has a /[ZM] suffix and the choice of suffix is not
>>       the same for all variants.
>>
>>     The _<sizes>, if present, give the subset of [BHSD] that are accepted
>>     by the V entries in <operands>.  */
>>
>> so maybe we could use a lower case "v" to represent "like V, but the
>> qualifier is optional".  That is:
>>
>> #define OP_SVE_Vv_HSD                                   \
>> {                                                       \
>>    QLF2(S_H,NIL),                                        \
>>    QLF2(S_S,NIL),                                        \
>>    QLF2(S_D,NIL),                                        \
>>    QLF2(S_H,S_H),                                        \
>>    QLF2(S_S,S_S),                                        \
>>    QLF2(S_D,S_D),                                        \
>> }
>>
>> (Not sure how we ended up with two definitions of OP_SVE_VU_HSD
>> in aarch64-tbl.h -- oops.)
>
> We tested the `OP_SVE_Vv_HSD` above.
>
> For `decp z0.h, p0.h`, we also need to modify `sve_size_hsd` encoding:
>
> ```
>
> opcodes/aarch64-asm.c:
>      aarch64_encode_variant_using_iclass (struct aarch64_inst *inst) {
>          ...
>          case sve_size_hsd:
>   -            insert_field (FLD_size, &inst->value, aarch64_get_variant 
> (inst) + 1, 0);
> +            insert_field (FLD_size, &inst->value, aarch64_get_variant 
> (inst) % 3 + 1, 0);
>              break;
>          ...
>      }
>
> ```
>
> Both `decp z0.h, p0` and `decp z0.h, p0.h` can be encoded correctly.

Ah, yeah, that looks right.

> However, what bothers us is `decp` instructions are disassembled to 
> unqualified versions, i.e. `decp z0.h, p0`,
>
> because `aarch64_decode_variant_using_iclass` chooses unqualified variant:
>
> ```
>
>      aarch64_decode_variant_using_iclass (aarch64_inst *inst) {
>          ....
>          case sve_size_hsd:
>              i = extract_field (FLD_size, inst->value, 0);
>              if (i < 1)
>                  return false;
>              variant = i - 1;
>              break;
>          ...
>      }
>
> ```
>
> It's better to get `decp z0.h, p0.h`, right?
>
>
> 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?

Thanks,
Richard



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-data-ref-Fix-ranges_maybe_overlap_p-test.patch --]
[-- Type: text/x-diff, Size: 1400 bytes --]

From: Richard Sandiford <richard.sandiford@arm.com>
Subject: [PATCH] data-ref: Fix ranges_maybe_overlap_p test
To: gcc-patches@gcc.gnu.org
Gcc: private.sent
--text follows this line--
dr_may_alias_p rightly used poly_int_tree_p to guard a use of
ranges_maybe_overlap_p, but used the non-poly extractors.
This caused a few failures in the SVE ACLE asm tests.

gcc/
	* tree-data-ref.cc (dr_may_alias_p): Use to_poly_widest instead
	of to_widest.
---
 gcc/tree-data-ref.cc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gcc/tree-data-ref.cc b/gcc/tree-data-ref.cc
index 91bfb619d66..978c3f002f7 100644
--- a/gcc/tree-data-ref.cc
+++ b/gcc/tree-data-ref.cc
@@ -2979,10 +2979,10 @@ dr_may_alias_p (const struct data_reference *a, const struct data_reference *b,
 	  && operand_equal_p (DR_OFFSET (a), DR_OFFSET (b))
 	  && poly_int_tree_p (tree_size_a)
 	  && poly_int_tree_p (tree_size_b)
-	  && !ranges_maybe_overlap_p (wi::to_widest (DR_INIT (a)),
-				      wi::to_widest (tree_size_a),
-				      wi::to_widest (DR_INIT (b)),
-				      wi::to_widest (tree_size_b)))
+	  && !ranges_maybe_overlap_p (wi::to_poly_widest (DR_INIT (a)),
+				      wi::to_poly_widest (tree_size_a),
+				      wi::to_poly_widest (DR_INIT (b)),
+				      wi::to_poly_widest (tree_size_b)))
 	{
 	  gcc_assert (integer_zerop (DR_STEP (a))
 		      && integer_zerop (DR_STEP (b)));
-- 
2.25.1


  reply	other threads:[~2022-09-30 14:54 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-16  0:53 Shaokun Zhang
2022-02-21 13:13 ` Jan Beulich
2022-03-02  7:36   ` Shaokun Zhang
2022-03-10  9:38     ` Shaokun Zhang
2022-03-10 10:38       ` Jan Beulich
2022-03-10 11:33         ` Shaokun Zhang
2022-03-10 11:50           ` Jan Beulich
2022-03-10 12:24             ` Shaokun Zhang
2022-03-10 12:27 ` Shaokun Zhang
2022-09-12  8:17 ` Richard Sandiford
2022-09-12  8:27   ` Jan Beulich
2022-09-12  9:38     ` Richard Sandiford
2022-09-28  2:19   ` dongbo (E)
2022-09-30 14:54     ` Richard Sandiford [this message]
2022-10-09  1:31       ` dongbo (E)
2022-10-10 10:25         ` Richard Sandiford
2022-10-11  2:19           ` dongbo (E)
2022-10-17  9:33             ` Richard Sandiford
2022-10-18  5:57               ` dongbo (E)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=mptbkqwoquj.fsf@arm.com \
    --to=richard.sandiford@arm.com \
    --cc=binutils@sourceware.org \
    --cc=caijingtao@huawei.com \
    --cc=dongbo4@huawei.com \
    --cc=jbeulich@suse.com \
    --cc=marcus.shawcroft@arm.com \
    --cc=rearnsha@arm.com \
    --cc=zhangshaokun@hisilicon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).