[PATCH RESEND v2] Aarch64: Allow explicit size specifier for predicate operand of {sq, uq, }{incp, decp}
Richard Sandiford
richard.sandiford@arm.com
Fri Sep 30 14:54:12 GMT 2022
"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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-data-ref-Fix-ranges_maybe_overlap_p-test.patch
Type: text/x-diff
Size: 1400 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20220930/666837a9/attachment.bin>
More information about the Binutils
mailing list