Re: [gen/AArch64] Generate helpers for substituting iterator values into pattern names
"Richard Earnshaw (lists)" writes:
> On 13/07/18 10:15, Richard Sandiford wrote:
>> Given a pattern like:
>>
>> (define_insn "aarch64_frecpe" ...)
>>
>> the SVE ACLE implementation wants to generate the pattern for a
>> particular (non-constant) mode. This patch automatically generates
>> helpers to do that, specifically:
>>
>> // Return CODE_FOR_nothing on failure.
>> insn_code maybe_code_for_aarch64_frecpe (machine_mode);
>>
>> // Assert that the code exists.
>> insn_code code_for_aarch64_frecpe (machine_mode);
>>
>> // Return NULL_RTX on failure.
>> rtx maybe_gen_aarch64_frecpe (machine_mode, rtx, rtx);
>>
>> // Assert that generation succeeds.
>> rtx gen_aarch64_frecpe (machine_mode, rtx, rtx);
>>
>> Many patterns don't have sensible names when all <...>s are removed.
>> E.g. "2" would give a base name "2". The new functions
>> therefore require explicit opt-in, which should also help to reduce
>> code bloat.
>>
>> The (arbitrary) opt-in syntax I went for was to prefix the pattern
>> name with '@', similarly to the existing '*' marker.
>>
>> The patch also makes config/aarch64 use the new routines in cases where
>> they obviously apply. This was mostly straight-forward, but it seemed
>> odd that we defined:
>>
>>aarch64_reload_movcp<...>
>>
>> but then only used it with DImode, never SImode. If we should be
>> using Pmode instead of DImode, then that's a simple change,
>> but should probably be a separate patch.
>>
>> Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
>> and x86_64-linux-gnu. I think I can self-approve the gen* bits,
>> but OK for the AArch64 parts?
>>
>> Any objections to this approach or syntax?
>
> So if I have two or more iterator rules in the MD, with a similar
> 'shape', eg
>
> (define_insn "@fred"...
>
> (define_insn "@fred"...
>
> this will generate a single factory function? If so, that sounds sensible.
Yeah, that's right. I think this is going to be quite common with the
SVE ACLE implementation, where some patterns need to cope with immediates
and others don't, and so are more naturally done as separate patterns.
Thanks to C++ overloading, it's also possible to have different shapes
(and thus different factories) with the same name, although YMMV on
whether that's a good idea in practice.
The original patch put the code_for_* declarations in insn-codes.h
(where codes are defined) and the gen_* declarations in insn-flags.h
(where normal generators are declared). Kugan pointed out that that
wouldn't work with code iterators (not covered by the aarch64 changes),
so I've moved then to insn-opinit.h instead. In some ways this seems
cleaner, since this is providing a very similar service to optabs.
I also realised that just removing <...> could lead to awkward
trailing or double underscores, so this patch avoids that.
I tested both updates with local changes to aarch64-sve.md
(not part of the commit).
Retested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
and x86_64-linux-gnu. Applied.
Thanks,
Richard
2018-08-02 Richard Sandiford
gcc/
* doc/md.texi: Expand the documentation of instruction names
to mention port-local uses. Document '@' in pattern names.
* read-md.h (overloaded_instance, overloaded_name): New structs.
(mapping): Declare.
(md_reader::handle_overloaded_name): New member function.
(md_reader::get_overloads): Likewise.
(md_reader::m_first_overload): New member variable.
(md_reader::m_next_overload_ptr): Likewise.
(md_reader::m_overloads_htab): Likewise.
* read-md.c (md_reader::md_reader): Initialize m_first_overload,
m_next_overload_ptr and m_overloads_htab.
* read-rtl.c (iterator_group): Add "type" and "get_c_token" fields.
(get_mode_token, get_code_token, get_int_token): New functions.
(map_attr_string): Add an optional argument that passes back
the associated iterator.
(overloaded_name_hash, overloaded_name_eq_p, named_rtx_p):
(md_reader::handle_overloaded_name, add_overload_instance): New
functions.
(apply_iterators): Handle '@' names. Report an error if '@'
is used without iterators.
(initialize_iterators): Initialize the new iterator_group fields.
* genopinit.c (handle_overloaded_code_for)
(handle_overloaded_gen): New functions.
(main): Use them to print declarations of maybe_code_for_* and
maybe_gen_* functions, and inline definitions of code_for_* and gen_*.
* genemit.c (print_overload_arguments, print_overload_test)
(handle_overloaded_code_for, handle_overloaded_gen): New functions.
(main): Use it to print definitions of maybe_code_for_* and
maybe_gen_* functions.
* config/aarch64/aarch64.c (aarch64_split_128bit_move): Use
gen_aarch64_mov{low,high}_di and gen_aarch64_movdi_{low,high}
instead of explicit mode checks.
Re: [gen/AArch64] Generate helpers for substituting iterator values into pattern names
On 13/07/18 10:15, Richard Sandiford wrote:
> Given a pattern like:
>
> (define_insn "aarch64_frecpe" ...)
>
> the SVE ACLE implementation wants to generate the pattern for a
> particular (non-constant) mode. This patch automatically generates
> helpers to do that, specifically:
>
> // Return CODE_FOR_nothing on failure.
> insn_code maybe_code_for_aarch64_frecpe (machine_mode);
>
> // Assert that the code exists.
> insn_code code_for_aarch64_frecpe (machine_mode);
>
> // Return NULL_RTX on failure.
> rtx maybe_gen_aarch64_frecpe (machine_mode, rtx, rtx);
>
> // Assert that generation succeeds.
> rtx gen_aarch64_frecpe (machine_mode, rtx, rtx);
>
> Many patterns don't have sensible names when all <...>s are removed.
> E.g. "2" would give a base name "2". The new functions
> therefore require explicit opt-in, which should also help to reduce
> code bloat.
>
> The (arbitrary) opt-in syntax I went for was to prefix the pattern
> name with '@', similarly to the existing '*' marker.
>
> The patch also makes config/aarch64 use the new routines in cases where
> they obviously apply. This was mostly straight-forward, but it seemed
> odd that we defined:
>
>aarch64_reload_movcp<...>
>
> but then only used it with DImode, never SImode. If we should be
> using Pmode instead of DImode, then that's a simple change,
> but should probably be a separate patch.
>
> Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
> and x86_64-linux-gnu. I think I can self-approve the gen* bits,
> but OK for the AArch64 parts?
>
> Any objections to this approach or syntax?
So if I have two or more iterator rules in the MD, with a similar
'shape', eg
(define_insn "@fred"...
(define_insn "@fred"...
this will generate a single factory function? If so, that sounds sensible.
(havent looked at the details of the patch, but if James has already
done that, then that should be enough).
R.
>
> Richard
>
>
> 2018-07-13 Richard Sandiford
>
> gcc/
> * doc/md.texi: Expand the documentation of instruction names
> to mention port-local uses. Document '@' in pattern names.
> * read-md.h (overloaded_instance, overloaded_name): New structs.
> (mapping): Declare.
> (md_reader::handle_overloaded_name): New member function.
> (md_reader::get_overloads): Likewise.
> (md_reader::m_first_overload): New member variable.
> (md_reader::m_next_overload_ptr): Likewise.
> (md_reader::m_overloads_htab): Likewise.
> * read-md.c (md_reader::md_reader): Initialize m_first_overload,
> m_next_overload_ptr and m_overloads_htab.
> * read-rtl.c (iterator_group): Add "type" and "get_c_token" fields.
> (get_mode_token, get_code_token, get_int_token): New functions.
> (map_attr_string): Add an optional argument that passes back
> the associated iterator.
> (overloaded_name_hash, overloaded_name_eq_p, named_rtx_p):
> (md_reader::handle_overloaded_name, add_overload_instance): New
> functions.
> (apply_iterators): Handle '@' names. Report an error if '@'
> is used without iterators.
> (initialize_iterators): Initialize the new iterator_group fields.
> * gencodes.c (handle_overloaded_code_for): New function.
> (main): Use it to print declarations of maybe_code_for_* functions
> and inline definitions of code_for_*.
> * genflags.c (emit_overloaded_gen_proto): New function.
> (main): Use it to print declarations of maybe_gen_* functions
> and inline definitions of gen_*.
> * genemit.c (print_overload_arguments, print_overload_test)
> (handle_overloaded_code_for, handle_overloaded_gen): New functions.
> (main): Use it to print definitions of maybe_code_for_* and
> maybe_gen_* functions.
> * config/aarch64/aarch64.c (aarch64_split_128bit_move): Use
> gen_aarch64_mov{low,high}_di and gen_aarch64_movdi_{low,high}
> instead of explicit mode checks.
> (aarch64_split_simd_combine): Likewise gen_aarch64_simd_combine.
> (aarch64_split_simd_move): Likewise gen_aarch64_split_simd_mov.
> (aarch64_emit_load_exclusive): Likewise gen_aarch64_load_exclusive.
> (aarch64_emit_store_exclusive): Likewise gen_aarch64_store_exclusive.
> (aarch64_expand_compare_and_swap): Likewise
> gen_aarch64_compare_and_swap and gen_aarch64_compare_and_swap_lse
> (aarch64_gen_atomic_cas): Likewise gen_aarch64_atomic_cas.
> (aarch64_emit_atomic_swap): Likewise gen_aarch64_atomic_swp.
> (aarch64_constant_pool_reload_icode): Delete.
> (aarch64_secondary_reload): Use code_for_aarch64_reload_movcp
> instead of aarch64_constant_pool_reload_icode. Use
> code_for_aarch64_reload_mov instead of explicit mode checks.
> (rsqrte_type, get_rsqrte_type, rsqrts_type, get_rsqrts_type): Delete.
> (aarch64_emit_approx_sqrt): Use gen_aarch64_rsqrte instead of
> get_rsqrte_type and
Re: [gen/AArch64] Generate helpers for substituting iterator values into pattern names
On Fri, Jul 13, 2018 at 04:15:41AM -0500, Richard Sandiford wrote:
> Given a pattern like:
>
> (define_insn "aarch64_frecpe" ...)
>
> the SVE ACLE implementation wants to generate the pattern for a
> particular (non-constant) mode. This patch automatically generates
> helpers to do that, specifically:
>
> // Return CODE_FOR_nothing on failure.
> insn_code maybe_code_for_aarch64_frecpe (machine_mode);
>
> // Assert that the code exists.
> insn_code code_for_aarch64_frecpe (machine_mode);
>
> // Return NULL_RTX on failure.
> rtx maybe_gen_aarch64_frecpe (machine_mode, rtx, rtx);
>
> // Assert that generation succeeds.
> rtx gen_aarch64_frecpe (machine_mode, rtx, rtx);
>
> Many patterns don't have sensible names when all <...>s are removed.
> E.g. "2" would give a base name "2". The new functions
> therefore require explicit opt-in, which should also help to reduce
> code bloat.
>
> The (arbitrary) opt-in syntax I went for was to prefix the pattern
> name with '@', similarly to the existing '*' marker.
>
> The patch also makes config/aarch64 use the new routines in cases where
> they obviously apply. This was mostly straight-forward, but it seemed
> odd that we defined:
>
>aarch64_reload_movcp<...>
>
> but then only used it with DImode, never SImode. If we should be
> using Pmode instead of DImode, then that's a simple change,
> but should probably be a separate patch.
>
> Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
> and x86_64-linux-gnu. I think I can self-approve the gen* bits,
> but OK for the AArch64 parts?
For what it is worth, I like the change to AArch64, and would support it
when you get consensus around the new syntax from other targets.
You only have to look at something like:
> - rtx (*gen) (rtx, rtx, rtx);
> -
> - switch (src_mode)
> -{
> -case E_V8QImode:
> - gen = gen_aarch64_simd_combinev8qi;
> - break;
> -case E_V4HImode:
> - gen = gen_aarch64_simd_combinev4hi;
> - break;
> -case E_V2SImode:
> - gen = gen_aarch64_simd_combinev2si;
> - break;
> -case E_V4HFmode:
> - gen = gen_aarch64_simd_combinev4hf;
> - break;
> -case E_V2SFmode:
> - gen = gen_aarch64_simd_combinev2sf;
> - break;
> -case E_DImode:
> - gen = gen_aarch64_simd_combinedi;
> - break;
> -case E_DFmode:
> - gen = gen_aarch64_simd_combinedf;
> - break;
> -default:
> - gcc_unreachable ();
> -}
> -
> - emit_insn (gen (dst, src1, src2));
> + emit_insn (gen_aarch64_simd_combine (src_mode, dst, src1, src2));
To understand this is a Good Thing for code maintainability.
Thanks,
James
>
> Any objections to this approach or syntax?
>
> Richard
[gen/AArch64] Generate helpers for substituting iterator values into pattern names
Given a pattern like:
(define_insn "aarch64_frecpe" ...)
the SVE ACLE implementation wants to generate the pattern for a
particular (non-constant) mode. This patch automatically generates
helpers to do that, specifically:
// Return CODE_FOR_nothing on failure.
insn_code maybe_code_for_aarch64_frecpe (machine_mode);
// Assert that the code exists.
insn_code code_for_aarch64_frecpe (machine_mode);
// Return NULL_RTX on failure.
rtx maybe_gen_aarch64_frecpe (machine_mode, rtx, rtx);
// Assert that generation succeeds.
rtx gen_aarch64_frecpe (machine_mode, rtx, rtx);
Many patterns don't have sensible names when all <...>s are removed.
E.g. "2" would give a base name "2". The new functions
therefore require explicit opt-in, which should also help to reduce
code bloat.
The (arbitrary) opt-in syntax I went for was to prefix the pattern
name with '@', similarly to the existing '*' marker.
The patch also makes config/aarch64 use the new routines in cases where
they obviously apply. This was mostly straight-forward, but it seemed
odd that we defined:
aarch64_reload_movcp<...>
but then only used it with DImode, never SImode. If we should be
using Pmode instead of DImode, then that's a simple change,
but should probably be a separate patch.
Tested on aarch64-linux-gnu (with and without SVE), aarch64_be-elf
and x86_64-linux-gnu. I think I can self-approve the gen* bits,
but OK for the AArch64 parts?
Any objections to this approach or syntax?
Richard
2018-07-13 Richard Sandiford
gcc/
* doc/md.texi: Expand the documentation of instruction names
to mention port-local uses. Document '@' in pattern names.
* read-md.h (overloaded_instance, overloaded_name): New structs.
(mapping): Declare.
(md_reader::handle_overloaded_name): New member function.
(md_reader::get_overloads): Likewise.
(md_reader::m_first_overload): New member variable.
(md_reader::m_next_overload_ptr): Likewise.
(md_reader::m_overloads_htab): Likewise.
* read-md.c (md_reader::md_reader): Initialize m_first_overload,
m_next_overload_ptr and m_overloads_htab.
* read-rtl.c (iterator_group): Add "type" and "get_c_token" fields.
(get_mode_token, get_code_token, get_int_token): New functions.
(map_attr_string): Add an optional argument that passes back
the associated iterator.
(overloaded_name_hash, overloaded_name_eq_p, named_rtx_p):
(md_reader::handle_overloaded_name, add_overload_instance): New
functions.
(apply_iterators): Handle '@' names. Report an error if '@'
is used without iterators.
(initialize_iterators): Initialize the new iterator_group fields.
* gencodes.c (handle_overloaded_code_for): New function.
(main): Use it to print declarations of maybe_code_for_* functions
and inline definitions of code_for_*.
* genflags.c (emit_overloaded_gen_proto): New function.
(main): Use it to print declarations of maybe_gen_* functions
and inline definitions of gen_*.
* genemit.c (print_overload_arguments, print_overload_test)
(handle_overloaded_code_for, handle_overloaded_gen): New functions.
(main): Use it to print definitions of maybe_code_for_* and
maybe_gen_* functions.
* config/aarch64/aarch64.c (aarch64_split_128bit_move): Use
gen_aarch64_mov{low,high}_di and gen_aarch64_movdi_{low,high}
instead of explicit mode checks.
(aarch64_split_simd_combine): Likewise gen_aarch64_simd_combine.
(aarch64_split_simd_move): Likewise gen_aarch64_split_simd_mov.
(aarch64_emit_load_exclusive): Likewise gen_aarch64_load_exclusive.
(aarch64_emit_store_exclusive): Likewise gen_aarch64_store_exclusive.
(aarch64_expand_compare_and_swap): Likewise
gen_aarch64_compare_and_swap and gen_aarch64_compare_and_swap_lse
(aarch64_gen_atomic_cas): Likewise gen_aarch64_atomic_cas.
(aarch64_emit_atomic_swap): Likewise gen_aarch64_atomic_swp.
(aarch64_constant_pool_reload_icode): Delete.
(aarch64_secondary_reload): Use code_for_aarch64_reload_movcp
instead of aarch64_constant_pool_reload_icode. Use
code_for_aarch64_reload_mov instead of explicit mode checks.
(rsqrte_type, get_rsqrte_type, rsqrts_type, get_rsqrts_type): Delete.
(aarch64_emit_approx_sqrt): Use gen_aarch64_rsqrte instead of
get_rsqrte_type and gen_aarch64_rsqrts instead of gen_rqrts_type.
(recpe_type, get_recpe_type, recps_type, get_recps_type): Delete.
(aarch64_emit_approx_div): Use gen_aarch64_frecpe instead of
get_recpe_type and gen_aarch64_frecps instead of get_recps_type.
(aarch64_atomic_load_op_code): Delete.
(aarch64_emit_atomic_load_op): Likewise.
(aarch64_gen_atomic_ldop): Use UNSPECV_ATOMIC_* instead of
aarch64_atomic_load_op_code. Us
