[Patch 1/4][Aarch64] v2: Implement Aarch64 SIMD ABI

Tue Dec 11 23:01:00 GMT 2018

On Fri, 2018-12-07 at 17:34 +0000, Richard Sandiford wrote:
> 
> I'm not an expert on this stuff, but it looks like:
> 
>   struct cgraph_node *node = cgraph_node::get (fndecl);
>   return node && node->simdclone;
> 
> might work.  But in some ways it would be cleaner to add the
> aarch64_vector_pcs attribute for SIMD clones, e.g. via a new hook,
> so that the function type is "correct".

I have changed this to add the aarch64_vector_pcs attribute to clones.
To do this I had to tweak the targetm.simd_clone.adjust target
function, but I did not have to add a new target function.  That change
is part of Patch 2/4 and I will resubmit that after this email.  The
change in this patch is to just check for the aarch64_vector_pcs
attribute and not look at the simd attribute to determine the ABI.

> 
> > @@ -4863,6 +4949,7 @@ aarch64_process_components (sbitmap
> > components, bool prologue_p)
> >        mergeable with the current one into a pair.  */
> >        if (!satisfies_constraint_Ump (mem)
> >         || GP_REGNUM_P (regno) != GP_REGNUM_P (regno2)
> > +       || (aarch64_simd_decl_p (cfun->decl) && (FP_REGNUM_P
> > (regno)))
> 
> Formatting nit: redundant brackets around FP_REGNUM_P (regno).

Fixed.

> > 
> > -(define_insn "simple_return"
> > +(define_expand "simple_return"
> > +  [(simple_return)]
> > +  "aarch64_use_simple_return_insn_p ()"
> > +  ""
> > +)
> > +
> > +(define_insn "*simple_return"
> >    [(simple_return)]
> >    ""
> >    "ret"
> 
> Can't you just change the condition on the existing define_insn,
> without turning it in a define_expand?  Worth a comment explaining
> why if not.

Yes, I am not sure why I did it this way (ciopying some other target
probably) but I got rid of the define_expand and changed the
define_insn and things seem to work fine.

> 
> > @@ -1487,6 +1538,23 @@
> >    [(set_attr "type" "neon_store1_2reg<q>")]
> >  )
> > 
> > +(define_insn "storewb_pair<TX:mode>_<P:mode>"
> > +  [(parallel
> > +    [(set (match_operand:P 0 "register_operand" "=&k")
> > +          (plus:P (match_operand:P 1 "register_operand" "0")
> > +                  (match_operand:P 4 "aarch64_mem_pair_offset"
> > "n")))
> > +     (set (mem:TX (plus:P (match_dup 0)
> > +                  (match_dup 4)))
> 
> Should be indented under the (match_dup 0).

Fixed.

> 
> > +          (match_operand:TX 2 "register_operand" "w"))
> > +     (set (mem:TX (plus:P (match_dup 0)
> > +                  (match_operand:P 5 "const_int_operand" "n")))
> > +          (match_operand:TX 3 "register_operand" "w"))])]
> 
> Think this last part should be:
> 
>      (set (mem:TX (plus:P (plus:P (match_dup 0)
>                                   (match_dup 4))
>                           (match_operand:P 5 "const_int_operand"
> "n")))
>           (match_operand:TX 3 "register_operand" "w"))])]

I think you are right about this.  What I have for
loadwb_pair<TX:mode>_<P:mode> matches what is there for
loadwb_pair<GPF:mode>_<P:mode>.  If this one is wrong, then I assume
the others are wrong too?  This won't make a practical difference since
we call these with gen_loadwb_pair*_* calls and not via pattern
recognition, but still they should be right.  Should I change them
all?  I did not change this as part of this patch.

> 
> > +  "TARGET_SIMD &&
> > +   INTVAL (operands[5]) == INTVAL (operands[4]) + GET_MODE_SIZE
> > (<TX:MODE>mode)"
> 
> && should be on the second line (which makes the line long enough to
> need breaking).

Fixed.

> 
> > diff --git a/gcc/testsuite/gcc.target/aarch64/torture/simd-abi-2.c
> > 
> 
> Comment doesn't match code: this is testing a normal PCS function.

Fixed.
> 
> > +++ b/gcc/testsuite/gcc.target/aarch64/torture/simd-abi-5.c
> 
> This is a nice test, but I think it would also be good to have versions
> that don't clobber full register pairs.  E.g. one without q9 and another
> without q10 would test individual STR Qs.

I added two new tests for this, simd-abi-6.c and simd-abi-7.c.

Steve Ellcey
sellcey@marvell.com

ChangeLog:

2018-12-11  Steve Ellcey  <sellcey@cavium.com>

	* config/aarch64/aarch64-protos.h (aarch64_use_simple_return_insn_p):
	New prototype.
	(aarch64_epilogue_uses): Ditto.
	* config/aarch64/aarch64.c (aarch64_attribute_table): New array.
	(aarch64_simd_decl_p): New function.
	(aarch64_reg_save_mode): New function.
	(aarch64_function_ok_for_sibcall): Check for simd calls.
	(aarch64_layout_frame): Check for simd function.
	(aarch64_gen_storewb_pair): Handle E_TFmode.
	(aarch64_push_regs): Use aarch64_reg_save_mode to get mode.
	(aarch64_gen_loadwb_pair): Handle E_TFmode.
	(aarch64_pop_regs): Use aarch64_reg_save_mode to get mode.
	(aarch64_gen_store_pair): Handle E_TFmode.
	(aarch64_gen_load_pair): Ditto.
	(aarch64_save_callee_saves): Handle different mode sizes.
	(aarch64_restore_callee_saves): Ditto.
	(aarch64_components_for_bb): Check for simd function.
	(aarch64_epilogue_uses): New function.
	(aarch64_process_components): Check for simd function.
	(aarch64_expand_prologue): Ditto.
	(aarch64_expand_epilogue): Ditto.
	(aarch64_expand_call): Ditto.
	(aarch64_use_simple_return_insn_p): New function.
	(TARGET_ATTRIBUTE_TABLE): New define.
	* config/aarch64/aarch64.h (EPILOGUE_USES): Redefine.
	(FP_SIMD_SAVED_REGNUM_P): New macro.
	* config/aarch64/aarch64.md (simple_return): New define_expand.
	(load_pair_dw_tftf): New instruction.
	(store_pair_dw_tftf): Ditto.
	(loadwb_pair<TX:mode>_<P:mode>): Ditto.
	(storewb_pair<TX:mode>_<P:mode>): Ditto.

Testsuite ChangeLog:

2018-12-11  Steve Ellcey  <sellcey@cavium.com>

	* gcc.target/aarch64/torture/aarch64-torture.exp: New file.
	* gcc.target/aarch64/torture/simd-abi-1.c: New test.
	* gcc.target/aarch64/torture/simd-abi-2.c: Ditto.
	* gcc.target/aarch64/torture/simd-abi-3.c: Ditto.
	* gcc.target/aarch64/torture/simd-abi-4.c: Ditto.
	* gcc.target/aarch64/torture/simd-abi-5.c: Ditto.
	* gcc.target/aarch64/torture/simd-abi-6.c: Ditto.
	* gcc.target/aarch64/torture/simd-abi-7.c: Ditto.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-vect-abi.patch
Type: text/x-patch
Size: 19356 bytes
Desc: gcc-vect-abi.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20181211/2cf9e519/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-vect-abi-test.patch
Type: text/x-patch
Size: 12975 bytes
Desc: gcc-vect-abi-test.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20181211/2cf9e519/attachment-0001.bin>