[11/67] target/arm: Add stubs for aa32 decodetree

Message ID 20190726175032.6769-12-richard.henderson@linaro.org
State Superseded
Headers show
Series
  • target/arm: Convert aa32 base isa to decodetree
Related show

Commit Message

Richard Henderson July 26, 2019, 5:49 p.m.
Add the infrastructure that will become the new decoder.
No instructions adjusted so far.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/translate.c       | 45 +++++++++++++++++++++++++++++++++++-
 target/arm/Makefile.objs     | 18 +++++++++++++++
 target/arm/a32-uncond.decode | 23 ++++++++++++++++++
 target/arm/a32.decode        | 23 ++++++++++++++++++
 target/arm/t32.decode        | 20 ++++++++++++++++
 5 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 target/arm/a32-uncond.decode
 create mode 100644 target/arm/a32.decode
 create mode 100644 target/arm/t32.decode

-- 
2.17.1

Comments

Peter Maydell July 29, 2019, 2:42 p.m. | #1
On Fri, 26 Jul 2019 at 18:50, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Add the infrastructure that will become the new decoder.

> No instructions adjusted so far.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/arm/translate.c       | 45 +++++++++++++++++++++++++++++++++++-

>  target/arm/Makefile.objs     | 18 +++++++++++++++

>  target/arm/a32-uncond.decode | 23 ++++++++++++++++++

>  target/arm/a32.decode        | 23 ++++++++++++++++++

>  target/arm/t32.decode        | 20 ++++++++++++++++

>  5 files changed, 128 insertions(+), 1 deletion(-)

>  create mode 100644 target/arm/a32-uncond.decode

>  create mode 100644 target/arm/a32.decode

>  create mode 100644 target/arm/t32.decode

>

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index 36419025db..4738b91957 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -7715,6 +7715,33 @@ static void arm_skip_unless(DisasContext *s, uint32_t cond)

>      }

>  }

>

> +/*

> + * Include the generated decoders.

> + * Note that the T32 decoder reuses some of the trans_* functions

> + * initially declared by the A32 decoder, which results in duplicate

> + * declaration warnings.  Suppress them.

> + */

> +

> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

> +# pragma GCC diagnostic push

> +# pragma GCC diagnostic ignored "-Wredundant-decls"

> +# ifdef __clang__

> +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"

> +# endif

> +#endif

> +

> +#include "decode-a32.inc.c"

> +#include "decode-a32-uncond.inc.c"

> +#include "decode-t32.inc.c"

> +

> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

> +# pragma GCC diagnostic pop

> +#endif


I'm not a great fan of having to use the diagnostic pragmas --
they seem a bit ugly and not very robust. Is this the only way?

> +

> +/*

> + * Legacy decoder.

> + */

> +

>  static void disas_arm_insn(DisasContext *s, unsigned int insn)

>  {

>      unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh;

> @@ -7733,7 +7760,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>          return;

>      }

>      cond = insn >> 28;

> -    if (cond == 0xf){

> +

> +    if (cond == 0xf) {

>          /* In ARMv3 and v4 the NV condition is UNPREDICTABLE; we

>           * choose to UNDEF. In ARMv5 and above the space is used

>           * for miscellaneous unconditional instructions.


This whitespace fixup should probably be in its own patch.

> @@ -7741,6 +7769,11 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>          ARCH(5);

>

>          /* Unconditional instructions.  */

> +        if (disas_a32_uncond(s, insn)) {

> +            return;

> +        }

> +        /* fall back to legacy decoder */

> +

>          if (((insn >> 25) & 7) == 1) {

>              /* NEON Data processing.  */

>              if (!arm_dc_feature(s, ARM_FEATURE_NEON)) {

> @@ -7953,6 +7986,11 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>

>      arm_skip_unless(s, cond);

>

> +    if (disas_a32(s, insn)) {

> +        return;

> +    }

> +    /* fall back to legacy decoder */

> +

>      if ((insn & 0x0f900000) == 0x03000000) {

>          if ((insn & (1 << 21)) == 0) {

>              ARCH(6T2);

> @@ -9440,6 +9478,11 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)

>          ARCH(6T2);

>      }

>

> +    if (disas_t32(s, insn)) {

> +        return;

> +    }

> +    /* fall back to legacy decoder */

> +

>      rn = (insn >> 16) & 0xf;

>      rs = (insn >> 12) & 0xf;

>      rd = (insn >> 8) & 0xf;


> diff --git a/target/arm/a32.decode b/target/arm/a32.decode

> new file mode 100644

> index 0000000000..2d84a02861

> --- /dev/null

> +++ b/target/arm/a32.decode

> @@ -0,0 +1,23 @@

> +# A32 conditional instructions

> +#

> +#  Copyright (c) 2019 Linaro, Ltd

> +#

> +# This library is free software; you can redistribute it and/or

> +# modify it under the terms of the GNU Lesser General Public

> +# License as published by the Free Software Foundation; either

> +# version 2 of the License, or (at your option) any later version.

> +#

> +# This library is distributed in the hope that it will be useful,

> +# but WITHOUT ANY WARRANTY; without even the implied warranty of

> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +# Lesser General Public License for more details.

> +#

> +# You should have received a copy of the GNU Lesser General Public

> +# License along with this library; if not, see <http://www.gnu.org/licenses/>.

> +

> +#

> +# This file is processed by scripts/decodetree.py

> +#

> +# All of the insn that have a COND field in insn[31:28] are here.

> +# All insns that have 0xf in insn[31:28] are in a32u.decode.


"a32-uncond.decode"

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM
Philippe Mathieu-Daudé Aug. 6, 2019, 9:41 p.m. | #2
On 7/26/19 7:49 PM, Richard Henderson wrote:
> Add the infrastructure that will become the new decoder.

> No instructions adjusted so far.

> 

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/arm/translate.c       | 45 +++++++++++++++++++++++++++++++++++-

>  target/arm/Makefile.objs     | 18 +++++++++++++++

>  target/arm/a32-uncond.decode | 23 ++++++++++++++++++

>  target/arm/a32.decode        | 23 ++++++++++++++++++

>  target/arm/t32.decode        | 20 ++++++++++++++++

>  5 files changed, 128 insertions(+), 1 deletion(-)

>  create mode 100644 target/arm/a32-uncond.decode

>  create mode 100644 target/arm/a32.decode

>  create mode 100644 target/arm/t32.decode

> 

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index 36419025db..4738b91957 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -7715,6 +7715,33 @@ static void arm_skip_unless(DisasContext *s, uint32_t cond)

>      }

>  }

>  

> +/*

> + * Include the generated decoders.

> + * Note that the T32 decoder reuses some of the trans_* functions

> + * initially declared by the A32 decoder, which results in duplicate

> + * declaration warnings.  Suppress them.


Ah! This has been bugging me with the various MIPS ISA for some time
too... Which makes me wonder if this shouldn't be "generated" using some
option when calling the decodetree script. Or have it notice the target
has dup declarations and automatically emit the guard.

> + */

> +

> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

> +# pragma GCC diagnostic push

> +# pragma GCC diagnostic ignored "-Wredundant-decls"

> +# ifdef __clang__

> +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"

> +# endif

> +#endif

> +

> +#include "decode-a32.inc.c"

> +#include "decode-a32-uncond.inc.c"

> +#include "decode-t32.inc.c"

[...]
Aleksandar Markovic Aug. 8, 2019, 11:41 a.m. | #3
On Fri, Jul 26, 2019 at 8:05 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> Add the infrastructure that will become the new decoder.

> No instructions adjusted so far.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/arm/translate.c       | 45 +++++++++++++++++++++++++++++++++++-

>  target/arm/Makefile.objs     | 18 +++++++++++++++

>  target/arm/a32-uncond.decode | 23 ++++++++++++++++++

>  target/arm/a32.decode        | 23 ++++++++++++++++++

>  target/arm/t32.decode        | 20 ++++++++++++++++

>  5 files changed, 128 insertions(+), 1 deletion(-)

>  create mode 100644 target/arm/a32-uncond.decode

>  create mode 100644 target/arm/a32.decode

>  create mode 100644 target/arm/t32.decode

>

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index 36419025db..4738b91957 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -7715,6 +7715,33 @@ static void arm_skip_unless(DisasContext *s,

> uint32_t cond)

>      }

>  }

>

> +/*

> + * Include the generated decoders.

> + * Note that the T32 decoder reuses some of the trans_* functions

> + * initially declared by the A32 decoder, which results in duplicate

> + * declaration warnings.  Suppress them.

> + */

> +

> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

> +# pragma GCC diagnostic push

> +# pragma GCC diagnostic ignored "-Wredundant-decls"

> +# ifdef __clang__

> +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"

> +# endif

> +#endif

> +

>


This looks more like a "band aid" solution rather than the right one.

I find it surprising that in spite of ever-growing complexity and numerous
refinements of the decodetree module, it still generates code that causes
these complaints of the compiler.

Regards,
Aleksandar



> +#include "decode-a32.inc.c"

> +#include "decode-a32-uncond.inc.c"

> +#include "decode-t32.inc.c"

> +

> +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

> +# pragma GCC diagnostic pop

> +#endif

> +

> +/*

> + * Legacy decoder.

> + */

> +

>  static void disas_arm_insn(DisasContext *s, unsigned int insn)

>  {

>      unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh;

> @@ -7733,7 +7760,8 @@ static void disas_arm_insn(DisasContext *s, unsigned

> int insn)

>          return;

>      }

>      cond = insn >> 28;

> -    if (cond == 0xf){

> +

> +    if (cond == 0xf) {

>          /* In ARMv3 and v4 the NV condition is UNPREDICTABLE; we

>           * choose to UNDEF. In ARMv5 and above the space is used

>           * for miscellaneous unconditional instructions.

> @@ -7741,6 +7769,11 @@ static void disas_arm_insn(DisasContext *s,

> unsigned int insn)

>          ARCH(5);

>

>          /* Unconditional instructions.  */

> +        if (disas_a32_uncond(s, insn)) {

> +            return;

> +        }

> +        /* fall back to legacy decoder */

> +

>          if (((insn >> 25) & 7) == 1) {

>              /* NEON Data processing.  */

>              if (!arm_dc_feature(s, ARM_FEATURE_NEON)) {

> @@ -7953,6 +7986,11 @@ static void disas_arm_insn(DisasContext *s,

> unsigned int insn)

>

>      arm_skip_unless(s, cond);

>

> +    if (disas_a32(s, insn)) {

> +        return;

> +    }

> +    /* fall back to legacy decoder */

> +

>      if ((insn & 0x0f900000) == 0x03000000) {

>          if ((insn & (1 << 21)) == 0) {

>              ARCH(6T2);

> @@ -9440,6 +9478,11 @@ static void disas_thumb2_insn(DisasContext *s,

> uint32_t insn)

>          ARCH(6T2);

>      }

>

> +    if (disas_t32(s, insn)) {

> +        return;

> +    }

> +    /* fall back to legacy decoder */

> +

>      rn = (insn >> 16) & 0xf;

>      rs = (insn >> 12) & 0xf;

>      rd = (insn >> 8) & 0xf;

> diff --git a/target/arm/Makefile.objs b/target/arm/Makefile.objs

> index 5cafc1eb6c..7806b4dac0 100644

> --- a/target/arm/Makefile.objs

> +++ b/target/arm/Makefile.objs

> @@ -28,9 +28,27 @@ target/arm/decode-vfp-uncond.inc.c:

> $(SRC_PATH)/target/arm/vfp-uncond.decode $(D

>           $(PYTHON) $(DECODETREE) --static-decode disas_vfp_uncond -o $@

> $<,\

>           "GEN", $(TARGET_DIR)$@)

>

> +target/arm/decode-a32.inc.c: $(SRC_PATH)/target/arm/a32.decode

> $(DECODETREE)

> +       $(call quiet-command,\

> +         $(PYTHON) $(DECODETREE) --static-decode disas_a32 -o $@ $<,\

> +         "GEN", $(TARGET_DIR)$@)

> +

> +target/arm/decode-a32-uncond.inc.c:

> $(SRC_PATH)/target/arm/a32-uncond.decode $(DECODETREE)

> +       $(call quiet-command,\

> +         $(PYTHON) $(DECODETREE) --static-decode disas_a32_uncond -o $@

> $<,\

> +         "GEN", $(TARGET_DIR)$@)

> +

> +target/arm/decode-t32.inc.c: $(SRC_PATH)/target/arm/t32.decode

> $(DECODETREE)

> +       $(call quiet-command,\

> +         $(PYTHON) $(DECODETREE) --static-decode disas_t32 -o $@ $<,\

> +         "GEN", $(TARGET_DIR)$@)

> +

>  target/arm/translate-sve.o: target/arm/decode-sve.inc.c

>  target/arm/translate.o: target/arm/decode-vfp.inc.c

>  target/arm/translate.o: target/arm/decode-vfp-uncond.inc.c

> +target/arm/translate.o: target/arm/decode-a32.inc.c

> +target/arm/translate.o: target/arm/decode-a32-uncond.inc.c

> +target/arm/translate.o: target/arm/decode-t32.inc.c

>

>  obj-y += tlb_helper.o debug_helper.o

>  obj-y += translate.o op_helper.o

> diff --git a/target/arm/a32-uncond.decode b/target/arm/a32-uncond.decode

> new file mode 100644

> index 0000000000..8dee26d3b6

> --- /dev/null

> +++ b/target/arm/a32-uncond.decode

> @@ -0,0 +1,23 @@

> +# A32 unconditional instructions

> +#

> +#  Copyright (c) 2019 Linaro, Ltd

> +#

> +# This library is free software; you can redistribute it and/or

> +# modify it under the terms of the GNU Lesser General Public

> +# License as published by the Free Software Foundation; either

> +# version 2 of the License, or (at your option) any later version.

> +#

> +# This library is distributed in the hope that it will be useful,

> +# but WITHOUT ANY WARRANTY; without even the implied warranty of

> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +# Lesser General Public License for more details.

> +#

> +# You should have received a copy of the GNU Lesser General Public

> +# License along with this library; if not, see <

> http://www.gnu.org/licenses/>.

> +

> +#

> +# This file is processed by scripts/decodetree.py

> +#

> +# All insns that have 0xf in insn[31:28] are decoded here.

> +# All of those that have a COND field in insn[31:28] are in a32.decode

> +#

> diff --git a/target/arm/a32.decode b/target/arm/a32.decode

> new file mode 100644

> index 0000000000..2d84a02861

> --- /dev/null

> +++ b/target/arm/a32.decode

> @@ -0,0 +1,23 @@

> +# A32 conditional instructions

> +#

> +#  Copyright (c) 2019 Linaro, Ltd

> +#

> +# This library is free software; you can redistribute it and/or

> +# modify it under the terms of the GNU Lesser General Public

> +# License as published by the Free Software Foundation; either

> +# version 2 of the License, or (at your option) any later version.

> +#

> +# This library is distributed in the hope that it will be useful,

> +# but WITHOUT ANY WARRANTY; without even the implied warranty of

> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +# Lesser General Public License for more details.

> +#

> +# You should have received a copy of the GNU Lesser General Public

> +# License along with this library; if not, see <

> http://www.gnu.org/licenses/>.

> +

> +#

> +# This file is processed by scripts/decodetree.py

> +#

> +# All of the insn that have a COND field in insn[31:28] are here.

> +# All insns that have 0xf in insn[31:28] are in a32u.decode.

> +#

> diff --git a/target/arm/t32.decode b/target/arm/t32.decode

> new file mode 100644

> index 0000000000..ac01fb6958

> --- /dev/null

> +++ b/target/arm/t32.decode

> @@ -0,0 +1,20 @@

> +# Thumb2 instructions

> +#

> +#  Copyright (c) 2019 Linaro, Ltd

> +#

> +# This library is free software; you can redistribute it and/or

> +# modify it under the terms of the GNU Lesser General Public

> +# License as published by the Free Software Foundation; either

> +# version 2 of the License, or (at your option) any later version.

> +#

> +# This library is distributed in the hope that it will be useful,

> +# but WITHOUT ANY WARRANTY; without even the implied warranty of

> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +# Lesser General Public License for more details.

> +#

> +# You should have received a copy of the GNU Lesser General Public

> +# License along with this library; if not, see <

> http://www.gnu.org/licenses/>.

> +

> +#

> +# This file is processed by scripts/decodetree.py

> +#

> --

> 2.17.1

>

>

>
Richard Henderson Aug. 8, 2019, 3:43 p.m. | #4
On 8/8/19 4:41 AM, Aleksandar Markovic wrote:
>     +/*

>     + * Include the generated decoders.

>     + * Note that the T32 decoder reuses some of the trans_* functions

>     + * initially declared by the A32 decoder, which results in duplicate

>     + * declaration warnings.  Suppress them.

>     + */

>     +

>     +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

>     +# pragma GCC diagnostic push

>     +# pragma GCC diagnostic ignored "-Wredundant-decls"

>     +# ifdef __clang__

>     +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"

>     +# endif

>     +#endif

>     +

> 

> 

> This looks more like a "band aid" solution rather than the right one.


What would the "right" solution be, would you say?

A couple of days ago Phil suggested moving these pragmas into the generated
code, so that this need not be done by hand in the several targets that use
multiple decoders.  That sounds reasonable to me.


r~
Aleksandar Markovic Aug. 9, 2019, 10:31 a.m. | #5
On Thu, Aug 8, 2019 at 5:43 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 8/8/19 4:41 AM, Aleksandar Markovic wrote:

> >     +/*

> >     + * Include the generated decoders.

> >     + * Note that the T32 decoder reuses some of the trans_* functions

> >     + * initially declared by the A32 decoder, which results in duplicate

> >     + * declaration warnings.  Suppress them.

> >     + */

> >     +

> >     +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

> >     +# pragma GCC diagnostic push

> >     +# pragma GCC diagnostic ignored "-Wredundant-decls"

> >     +# ifdef __clang__

> >     +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"

> >     +# endif

> >     +#endif

> >     +

> >

> >

> > This looks more like a "band aid" solution rather than the right one.

>

> What would the "right" solution be, would you say?

>

>

The right (without quotation marks) solution is not to generate the code
that generates compiler complaints.

However, I do not say that this is a stopping issue for this series.
Perhaps at some time in future you can think of ways that would achieve not
resorting to pragmas. Obviously, in this case, decodetree-generated code
shows that it is inferior to human-generated code. Still, for now, go ahead
with this patch, as far as I am concerned.

Yours,
Aleksandar



> A couple of days ago Phil suggested moving these pragmas into the generated

> code, so that this need not be done by hand in the several targets that use

> multiple decoders.  That sounds reasonable to me.

>

>

> r~

>
Richard Henderson Aug. 9, 2019, 2:55 p.m. | #6
On 8/9/19 3:31 AM, Aleksandar Markovic wrote:
> 

> 

> On Thu, Aug 8, 2019 at 5:43 PM Richard Henderson <richard.henderson@linaro.org

> <mailto:richard.henderson@linaro.org>> wrote:

> 

>     On 8/8/19 4:41 AM, Aleksandar Markovic wrote:

>     >     +/*

>     >     + * Include the generated decoders.

>     >     + * Note that the T32 decoder reuses some of the trans_* functions

>     >     + * initially declared by the A32 decoder, which results in duplicate

>     >     + * declaration warnings.  Suppress them.

>     >     + */

>     >     +

>     >     +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

>     >     +# pragma GCC diagnostic push

>     >     +# pragma GCC diagnostic ignored "-Wredundant-decls"

>     >     +# ifdef __clang__

>     >     +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"

>     >     +# endif

>     >     +#endif

>     >     +

>     >

>     >

>     > This looks more like a "band aid" solution rather than the right one.

> 

>     What would the "right" solution be, would you say?

> 

> 

> The right (without quotation marks) solution is not to generate the code that

> generates compiler complaints.


That would be impossible with the information supplied.

Emitting zero declarations will result in invalid C.  We ensure that each
individual decodetree file does not emit duplicates.  However, there is no
knowledge across separate decodetree files about which declarations are duplicate.

In this particular case, I do not even agree that the warnings themselves are
useful.  I suppose it's not impossible that they could diagnose some weirdness
in a code base, where a function is declared in multiple headers or a patch
application has gone awry leaving multiple sequential declarations, but
honestly how often does that happen?

I suppose with some work, we could not invoke decodetree multiple times, but
give it all of the input files at once, producing a single output file.


> Obviously, in this case, decodetree-generated code shows that it is

> inferior to human-generated code.


That is not obvious at all.  Moreover, I disagree strongly.


r~
Aleksandar Markovic Aug. 9, 2019, 3:30 p.m. | #7
On Fri, Aug 9, 2019 at 4:55 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 8/9/19 3:31 AM, Aleksandar Markovic wrote:

> >

> >

> > On Thu, Aug 8, 2019 at 5:43 PM Richard Henderson <

> richard.henderson@linaro.org

> > <mailto:richard.henderson@linaro.org>> wrote:

> >

> >     On 8/8/19 4:41 AM, Aleksandar Markovic wrote:

> >     >     +/*

> >     >     + * Include the generated decoders.

> >     >     + * Note that the T32 decoder reuses some of the trans_*

> functions

> >     >     + * initially declared by the A32 decoder, which results in

> duplicate

> >     >     + * declaration warnings.  Suppress them.

> >     >     + */

> >     >     +

> >     >     +#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE

> >     >     +# pragma GCC diagnostic push

> >     >     +# pragma GCC diagnostic ignored "-Wredundant-decls"

> >     >     +# ifdef __clang__

> >     >     +#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"

> >     >     +# endif

> >     >     +#endif

> >     >     +

> >     >

> >     >

> >     > This looks more like a "band aid" solution rather than the right

> one.

> >

> >     What would the "right" solution be, would you say?

> >

> >

> > The right (without quotation marks) solution is not to generate the code

> that

> > generates compiler complaints.

>

> That would be impossible with the information supplied.

>


That limitation is a fault of decodetree design, not an obstacle per se.


> Emitting zero declarations will result in invalid C.  We ensure that each

> individual decodetree file does not emit duplicates.  However, there is no

> knowledge across separate decodetree files about which declarations are

> duplicate.

>

> In this particular case, I do not even agree that the warnings themselves

> are

> useful.  I suppose it's not impossible that they could diagnose some

> weirdness

> in a code base, where a function is declared in multiple headers or a patch

> application has gone awry leaving multiple sequential declarations, but

> honestly how often does that happen?

>

> I suppose with some work, we could not invoke decodetree multiple times,

> but

> give it all of the input files at once, producing a single output file.

>

>

> > Obviously, in this case, decodetree-generated code shows that it is

> > inferior to human-generated code.

>

> That is not obvious at all.  Moreover, I disagree strongly.

>

>

OK. I'm fine with your disagreeing. That why open source exists - to have
opportunities to hear different views.

Yours,
Aleksandar


> r~

>

Patch

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 36419025db..4738b91957 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7715,6 +7715,33 @@  static void arm_skip_unless(DisasContext *s, uint32_t cond)
     }
 }
 
+/*
+ * Include the generated decoders.
+ * Note that the T32 decoder reuses some of the trans_* functions
+ * initially declared by the A32 decoder, which results in duplicate
+ * declaration warnings.  Suppress them.
+ */
+
+#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wredundant-decls"
+# ifdef __clang__
+#  pragma GCC diagnostic ignored "-Wtypedef-redefinition"
+# endif
+#endif
+
+#include "decode-a32.inc.c"
+#include "decode-a32-uncond.inc.c"
+#include "decode-t32.inc.c"
+
+#ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
+# pragma GCC diagnostic pop
+#endif
+
+/*
+ * Legacy decoder.
+ */
+
 static void disas_arm_insn(DisasContext *s, unsigned int insn)
 {
     unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh;
@@ -7733,7 +7760,8 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
         return;
     }
     cond = insn >> 28;
-    if (cond == 0xf){
+
+    if (cond == 0xf) {
         /* In ARMv3 and v4 the NV condition is UNPREDICTABLE; we
          * choose to UNDEF. In ARMv5 and above the space is used
          * for miscellaneous unconditional instructions.
@@ -7741,6 +7769,11 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
         ARCH(5);
 
         /* Unconditional instructions.  */
+        if (disas_a32_uncond(s, insn)) {
+            return;
+        }
+        /* fall back to legacy decoder */
+
         if (((insn >> 25) & 7) == 1) {
             /* NEON Data processing.  */
             if (!arm_dc_feature(s, ARM_FEATURE_NEON)) {
@@ -7953,6 +7986,11 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
 
     arm_skip_unless(s, cond);
 
+    if (disas_a32(s, insn)) {
+        return;
+    }
+    /* fall back to legacy decoder */
+
     if ((insn & 0x0f900000) == 0x03000000) {
         if ((insn & (1 << 21)) == 0) {
             ARCH(6T2);
@@ -9440,6 +9478,11 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
         ARCH(6T2);
     }
 
+    if (disas_t32(s, insn)) {
+        return;
+    }
+    /* fall back to legacy decoder */
+
     rn = (insn >> 16) & 0xf;
     rs = (insn >> 12) & 0xf;
     rd = (insn >> 8) & 0xf;
diff --git a/target/arm/Makefile.objs b/target/arm/Makefile.objs
index 5cafc1eb6c..7806b4dac0 100644
--- a/target/arm/Makefile.objs
+++ b/target/arm/Makefile.objs
@@ -28,9 +28,27 @@  target/arm/decode-vfp-uncond.inc.c: $(SRC_PATH)/target/arm/vfp-uncond.decode $(D
 	  $(PYTHON) $(DECODETREE) --static-decode disas_vfp_uncond -o $@ $<,\
 	  "GEN", $(TARGET_DIR)$@)
 
+target/arm/decode-a32.inc.c: $(SRC_PATH)/target/arm/a32.decode $(DECODETREE)
+	$(call quiet-command,\
+	  $(PYTHON) $(DECODETREE) --static-decode disas_a32 -o $@ $<,\
+	  "GEN", $(TARGET_DIR)$@)
+
+target/arm/decode-a32-uncond.inc.c: $(SRC_PATH)/target/arm/a32-uncond.decode $(DECODETREE)
+	$(call quiet-command,\
+	  $(PYTHON) $(DECODETREE) --static-decode disas_a32_uncond -o $@ $<,\
+	  "GEN", $(TARGET_DIR)$@)
+
+target/arm/decode-t32.inc.c: $(SRC_PATH)/target/arm/t32.decode $(DECODETREE)
+	$(call quiet-command,\
+	  $(PYTHON) $(DECODETREE) --static-decode disas_t32 -o $@ $<,\
+	  "GEN", $(TARGET_DIR)$@)
+
 target/arm/translate-sve.o: target/arm/decode-sve.inc.c
 target/arm/translate.o: target/arm/decode-vfp.inc.c
 target/arm/translate.o: target/arm/decode-vfp-uncond.inc.c
+target/arm/translate.o: target/arm/decode-a32.inc.c
+target/arm/translate.o: target/arm/decode-a32-uncond.inc.c
+target/arm/translate.o: target/arm/decode-t32.inc.c
 
 obj-y += tlb_helper.o debug_helper.o
 obj-y += translate.o op_helper.o
diff --git a/target/arm/a32-uncond.decode b/target/arm/a32-uncond.decode
new file mode 100644
index 0000000000..8dee26d3b6
--- /dev/null
+++ b/target/arm/a32-uncond.decode
@@ -0,0 +1,23 @@ 
+# A32 unconditional instructions
+#
+#  Copyright (c) 2019 Linaro, Ltd
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, see <http://www.gnu.org/licenses/>.
+
+#
+# This file is processed by scripts/decodetree.py
+#
+# All insns that have 0xf in insn[31:28] are decoded here.
+# All of those that have a COND field in insn[31:28] are in a32.decode
+#
diff --git a/target/arm/a32.decode b/target/arm/a32.decode
new file mode 100644
index 0000000000..2d84a02861
--- /dev/null
+++ b/target/arm/a32.decode
@@ -0,0 +1,23 @@ 
+# A32 conditional instructions
+#
+#  Copyright (c) 2019 Linaro, Ltd
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, see <http://www.gnu.org/licenses/>.
+
+#
+# This file is processed by scripts/decodetree.py
+#
+# All of the insn that have a COND field in insn[31:28] are here.
+# All insns that have 0xf in insn[31:28] are in a32u.decode.
+#
diff --git a/target/arm/t32.decode b/target/arm/t32.decode
new file mode 100644
index 0000000000..ac01fb6958
--- /dev/null
+++ b/target/arm/t32.decode
@@ -0,0 +1,20 @@ 
+# Thumb2 instructions
+#
+#  Copyright (c) 2019 Linaro, Ltd
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, see <http://www.gnu.org/licenses/>.
+
+#
+# This file is processed by scripts/decodetree.py
+#