[PATCH] Add support for RISC-V Z-extension arch attributes.

Jim Wilson jimw@sifive.com
Thu Aug 1 01:34:00 GMT 2019


On Mon, Jul 29, 2019 at 6:10 AM Maxim Blinov <maxim.blinov@embecosm.com> wrote:
> This patch adds support for operating with the proposed
> Z-extension architecture attributes. Some examples:

Kito tells me that the LLVM folks are discussing this issue, and that
we need to coordinate with them to ensure that the GNU and LLVM tools
handle this the same way.  Hopefully Kito can do that.

> I have changed the order of extension parsing to the following:
> 1: 's' (standard supervisor extensions)
> 2: 'sx' (non-standard standard supervisor extensions)
> 3: 'z' (standard extensions)
> 4: 'x' (non-standard extensions)
>
> The order before this patch was as follows:
> 1: 'x'
> 2: 's'
> 3: 'sx'

This is another potential problem, as this introduces an
incompatibility for anyone using the old scheme.  Kito has proposed
adding a configure option to specify which version of the ISA spec is
being implemented, and then presumably we can use the order specified
by that version of the ISA spec.

> +/* Predicator for Z standard extensions. */

Coding conventions call for two spaces after a period.  Both before
the end of the comment, and between sentences in the middle of a
comment.

> +static void
> +riscv_init_std_z_ext_mergetab (riscv_std_z_ext_mergetab_t *map)
> +{
> +  memset ((void *)map, 0, sizeof (riscv_std_z_ext_mergetab_t));
> +}

There should be an explanatory comment before each function.  This is
probably missing from some functions, but we shouldn't make this worse
by adding new functions without comments.  This can be a brief
description of what the function does, and mention what the parameters
do.

> +      /* Neither object files use this extension. Skip. */

/* Neither object file uses this extension.  Skip.  */

> +static bfd_boolean
> +riscv_merge_std_z_ext (bfd *ibfd,
> +                      riscv_subset_t **pin,
> +                      riscv_subset_t **pout,
> +                      bfd_boolean (*predicate_func) (const char *))
> +{
> +  riscv_subset_t *in = *pin;
> +  riscv_subset_t *out = *pout;

Missing an explanatory comment before this function saying what it
does.  And a number of functions below.

This is setting in and out, but they aren't used except at the end to
update the args.  This isn't useful.

> +  /* Populate table. */
> +  status_in = riscv_populate_std_z_ext_mergetab
> +    (ibfd, pin, predicate_func, mt.input);
> +  status_out = riscv_populate_std_z_ext_mergetab
> +    (ibfd, pout, predicate_func, mt.output);

These function calls should be updating pin/pout as they parse
extensions from the list.  I would expect something that looks more
like what riscv_merge_non_std_and_sv_ext does.  So
riscv_populate_std_z_ext_mergetab has "*pin = in;" at the end, and
this function does not need to set *pin.

>    /* Merge non-standard supervisor extension.  */
>    if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_sv_ext_p))
>      return NULL;
> +  /* Merge standard Z extensions. */
> +  if (!riscv_merge_std_z_ext (ibfd, &in, &out, riscv_std_z_ext_p))
> +    return NULL;

You changed the order of parsing s, sx, and x below, but did not
change it here.  This is inconsistent, and looks like it may not work.
We should be using the same order here as below.  Changing the order
here means getting the code to set &in and &out right is important
because the z parsing isn't at the end anymore.

> +const char * const z_ext_strtab[RISCV_STD_Z_EXT_COUNT] =
> +  {
> +   "zam",
> +
> +   /* Standard Bitmanip ISA subsets. */
> +   "zbb", "zbc", "zbe", "zbf", "zbm", "zbp", "zbr", "zbs", "zbt",
> +
> +   "zifencei", "ztso"
> +  };

There are no standard Bitmanip ISA subsets.  I think this needs to be
pulled out and maintained as a separate patch.  We can put this on a
branch, in the FSF tree, or in the github.com/riscv trees.  I don't
want to add support for isa subsets that haven't been formally
approved yet, as they may change in the draft, and they don't have ISA
versions yet.

zam is also in draft form, but is at least in the ISA manual.  The B
extensions are not.  ztso is not formally approved yet but is frozen
so should be OK.  You are missing zicsr.

The ISA spec says that z extensions should be in ISA subset order and
then alphabetically.  So zifencei should come before zam, because I
comes before A in the canonical extensions order.  If this is
inconvenient maybe we can propose a change to the ISA spec to do it
alphabetically ignoring the isa subset ordering.

> +  /* Convert string to all-lowercase. */
> +  for (i = 0; i < len; ++i)
> +    {
> +      if (ISUPPER (ext[i]))
> +       lower[i] += 'a' - 'A';
> +    }

Converting the string to lowercase and throwing it away is wasteful.
And this code is probably not locale safe, for instance it probably
doesn't work with EBCDIC.

> +      if (!strcmp (lower, z_ext_strtab[i]))

Simpler to just use strcasecmp here, as is done in other places.

> + start:
> +  /* Assert this is prefixed with 'z' */
> +  if (*p != 'z')
> +    return p;

I would much prefer a while statement here instead of a label and a
goto label at the end of the function.

> +  if (!strcmp (last_name, name_buffer))
> +    {
> +      rps->error_handler ("-march=%s: Duplicate Z ISA extension: \'%s\'",
> +                         arch, name_buffer);
> +      free (name_buffer);
> +      return NULL;
> +    }

Other parts of the code aren't checking for duplicates.  Maybe we need
to extend them to do this also?

> +  /* Assert that we are in alphabetical order. */
> +  if (riscv_valid_std_z_ext_p (last_name)
> +      && strcmp (last_name, name_buffer) > 0)

I wonder if this is locale safe.  That might be an issue for the ISA
spec to clearly specify what locale is being used for their
alphabetical sort.

> +  /* Skip trailing whitespace */
> +  for (; *iter == '_'; iter++);

Other places in the code only allow one underscore.  This should be
same as the other places, or they should all be changed.

Jim



More information about the Binutils mailing list