Re: [Patch ARM] Refactor neon-schedgen.ml and regenerate A8 neon descriptions.

[Julian Brown <julian at codesourcery dot com>]

On Wed, 25 Aug 2010 09:07:49 +0100
Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> wrote:

> Hi,
> 
> This patch refactors neon-schedgen a bit to allow support for multiple
> cores and generate multiple scheduler descriptions out of a single
> source. I am only submitting the regenerated A8 descriptions and the
> changes to the ML. 
> 
> The major change to neon-schedgen.ml is the addition of a core type,
> thus adding support for a new core is essentially adding the new core
> type, adding the core to the list of allCores and then ending up
> regenerating all the insn_reservations and the bypasses from this. The
> only bits that are still hand-written are the bits that bolt these
> reservations and bypasses with the interactions with the other parts
> of the scheduler description.

I might have the wrong end of the stick here but... neon.md is filled
with "neon_type" specifications which are already hairy and verbose,
e.g.:

  [(set (attr "neon_type")
      (if_then_else (ne (symbol_ref "<Is_float_mode>") (const_int 0))
                    (if_then_else (ne (symbol_ref "<Is_d_reg>") (const_int 0))
                                  (const_string "neon_fp_vadd_ddd_vabs_dd")
                                  (const_string "neon_fp_vadd_qqq_vabs_qq"))
                    (if_then_else (ne (symbol_ref "<Is_d_reg>") (const_int 0))
                                  (if_then_else
                                    (ne (symbol_ref "<Scalar_mul_8_16>") (const_int 0))
                                    (const_string "neon_mul_ddd_8_16_qdd_16_8_long_32_16_long")
                                    (const_string "neon_mul_qqq_8_16_32_ddd_32"))
                                  (if_then_else (ne (symbol_ref "<Scalar_mul_8_16>") (const_int 0))
                                    (const_string "neon_mul_qqq_8_16_32_ddd_32")
                                    (const_string "neon_mul_qqq_8_16_32_ddd_32")))))]

It seems like adding cortex_a8 to the start of each of these won't
exactly help make these clearer -- and what are we going to do about
new cores? The real problem here is that these "neon_types" are far too
strongly tied to one physical implementation of the NEON ISA. My
(admittedly vague) notion is that we should be moving towards more
"logical" units for the neon instructions, decoupling them from any
particular implementation, and hopefully cut down on these rather opaque
conditionals whilst doing it. Or is that in fact what you are
suggesting?

If you're going to hack neon-schedgen.ml (which I didn't write, and
admit I don't have any great understanding of), would it make sense to
create a new table for Cortex-A9 (etc.), rather than mixing
definitions for all cores together in the existing availability_table?

Also a minor point on the ML: you can write,

let coreStr core = 
                 match core with
                   CortexA8 -> "cortex_a8" 
                 | CortexA9 -> "cortex_a9"

as:

let coreStr = function
    CortexA8 -> "cortex_a8"
  | CortexA9 -> "cortex_a9"

I know there's no GNU style guide for ML, but please try to follow the
existing formatting (re: indentation and so forth) if possible.

Cheers,

Julian