Indu Bhagat <indu.bha...@oracle.com> writes:
> subg (Subtract with Tag) is an Armv8.5-A memory tagging (MTE)
> instruction.  It can be used to subtract an immediate value scaled by
> the tag granule from the address in the source register.
>
> gcc/ChangeLog:
>
>       * config/aarch64/aarch64.md (subg): New definition.

Thanks for the patches and sorry for the slow review.  I'm still
thinking about the bigger-picture questions, but FWIW, here are some
minor comments on the patterns themselves:

> ---
>  gcc/config/aarch64/aarch64.md | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 8d10197c9e8d..1ec872afef71 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -8193,6 +8193,23 @@
>    [(set_attr "type" "memtag")]
>  )
>  
> +(define_insn "subg"
> +  [(set (match_operand:DI 0 "register_operand" "=rk")
> +     (ior:DI
> +      (and:DI (minus:DI (match_operand:DI 1 "register_operand" "rk")
> +                       (match_operand:DI 2 "aarch64_granule16_uimm6" "i"))
> +              (const_int -1080863910568919041)) ;; 0xf0ff...
> +      (ashift:DI
> +       (unspec:QI

The modes needs to agree here: the first operand of the ashift needs to
be the same mode as the ashift itself.  So I think this should be unspec:DI.

> +        [(and:QI (lshiftrt:DI (match_dup 1) (const_int 56)) (const_int 15))
> +         (match_operand:QI 3 "aarch64_memtag_tag_offset" "i")]
> +        UNSPEC_GEN_TAG)

Similarly here for the and vs. lshiftrt.  But since it's an unspec
anyway, we could just use:

        (unspec:DI [(match_dup 1)
                    (match_operand:QI 3 "aarch64_memtag_tag_offset")]
                   UNSPEC_GEN_TAG)

I realise this construct is used a lot, but IMO it's better to leave out
the constraints for constant operands, since e.g. the operand predicates
above don't accept all the things that the "i" constraint does.  In general,
constraints should only match things that the predicates match.

Thanks,
Richard

> +       (const_int 56))))]
> +  "TARGET_MEMTAG"
> +  "subg\\t%0, %1, #%2, #%3"
> +  [(set_attr "type" "memtag")]
> +)
> +
>  (define_insn "subp"
>    [(set (match_operand:DI 0 "register_operand" "=r")
>       (minus:DI

Reply via email to