>> +/* Implement TARGET_MEMTAG_ADD_TAG. */
>> +rtx
>> +aarch64_memtag_add_tag (rtx base, poly_int64 offset, uint8_t tag_offset)
>> +{
>> + rtx target = NULL;
>> + poly_int64 addr_offset = offset;
>> + rtx offset_rtx = gen_int_mode (addr_offset, DImode);
>
> This should only be done for the non-default path. Same below.
ACK.
>
>> +
>> + if (!memtag_sanitize_p () || tag_offset == 0)
>> + return default_memtag_add_tag (base, offset, tag_offset);
>
> It would be good to use a consistent style about whether to exit
> early for !memtag_sanitize_p or use:
>
> if (memtag_sanitizie_p ())
> ...
> else
> ...default...;
>
> The former seems to be the general style used in GCC. Same below.
>
ACK.
> I suppose if we don't use UNSPEC_GEN_TAG here, we won't match and use
> GMI when operand 2 is zero. Is that why you changed this pattern too?
Yes :)
> If so, I think we should make it:
>
> (unspec:DI [(match_operand:DI 1 "register_operand")
> (const_int 0)]
> UNSPEC_GEN_TAG)
>
> instead.
>
Will do.
>> + (match_operand:DI 2 "aarch64_reg_or_zero")))]
>> "TARGET_MEMTAG"
>> - "gmi\\t%0, %1, %2"
>> - [(set_attr "type" "memtag")]
>> + {@ [ cons: =0, 1, 2 ; attrs: type ]
>> + [ r , rk, r ; memtag ] gmi\\t%0, %1, %2
>> + [ r , rk, Z ; memtag ] gmi\\t%0, %1, xzr
>> + }
>
> There's no need to separate these alternatives out. We can use rZ
> as the constraint and %x2 as the output operand.
ACK
>> + [(set (match_operand:TI 0 "aarch64_granule16_memory_operand" "=Umg")
>> + (unspec:TI
>> + [(match_operand:TI 1 "aarch64_granule16_memory_operand" "Umg")
>> + (match_operand:DI 2 "register_operand" "rk")]
>> + UNSPEC_TAG_SPACE))]
>> + "TARGET_MEMTAG && aarch64_check_memtag_ops (operands[0], operands[1])"
>> + "stg\\t%2, %0"
>> + [(set_attr "type" "memtag")]
>> +)
>
> In the previous reviews, I'd suggested we use things like:
>
> (set (match_operand:TI 0 "aarch64_granule_memory_operand" "+Umg")
> (unspec_volatile:TI
> [(match_dup 0)
> (match_operand:DI 1 "register_operand" "rk")]
> UNSPECV...))
>
> for the STG patterns. Does that not work? I remember there was some
> follow-up discussion about the post-increment case, but I thought the
> plan had been to use a separate pattern for those.
First, I wanted to avoid using unpsec_volatile constructions. Then,
using match_dup 0 is not good when we do have POST/PRE INC/DEC/MODIFY,
as the compiler sees two operands doing the PRE/POST operation. Hence, I
introduced the aarch64_check_memtag_ops() function which checks if the
operands are the same or not (ignoring the POST/PRE constructions in
operand 0). Moreover, I wanted to use the already in machinery of
classifying an address (see aarch64_granule16_memory_address_p).
>
> (Sorry if I'm misremembering, been a while since the last series.)
>
> The problem with the pattern in the patch is that operands 0 and 1's
> predicates and constraints treat the operands as entirely separate.
> aarch64_check_memtag_ops would be applied during insn recognition but
> it would have no effect on (for example) register allocation.
Alternatively, I can "explode" the above pattern to handle separately
PRE/POST cases.
>
> That said... I wonder if it would work to drop the aarch64_check_memtag_ops
> condition and instead use something like:
>
> return (GET_RTX_CLASS (GET_CODE (XEXP (operands[1], 0))) == RTX_AUTOINC
> ? "stg\\t%2, %1"
> : "stg\\t%2, %0");
>
> We know that operands 0 and 1 started out as the same address value and
> it would be invalid for any later pass to change that value (rather than
> the way that the value is computed).
>
> I don't remember seeing that RTX_AUTOINC pattern used before, and it
> feels a bit underhand, but I can't immediately think of a reason why
> it's wrong in principle.
>
> The unspec must be unspec_volatile though, for the reason in the
> previous review:
>
> The unspec_volatile should ensure that nothing tries to remove the
> store as dead (which would especially be a problem when clearing tags).
Indeed. For avoiding DCE, I have added a memory barrier. Of course, I
can revert it back to unspec_volatile.
>> +(define_constraint "Utg"
>> + "@internal
>> + A constant that can be used as tag offset for an ADDG/SUBG operation."
>> + (match_operand 0 "aarch64_memtag_tag_offset"))
>
> There's no need for this constraint, since the ADDG operand uses
> aarch64_memtag_tag_offset as the predicate. The constraints for
> operand 3 can just be blank.
ACK. (didn't like the generator complaining about missing operand:) )
Thank you,
Claudiu
