On 06/13/2018 12:57 PM, Dimitar Dimitrov wrote:
> ChangeLog:
>
> 2018-06-13 Dimitar Dimitrov <dimi...@dinux.eu>
>
> * configure: Regenerate.
> * configure.ac: Add PRU target.
>
> gcc/ChangeLog:
>
> 2018-06-13 Dimitar Dimitrov <dimi...@dinux.eu>
>
> * config/pru/pru-ldst-multiple.md: Generate using pru-ldst-multiple.ml.
> * common/config/pru/pru-common.c: New file.
> * config.gcc: Add PRU target.
> * config/pru/alu-zext.md: New file.
> * config/pru/constraints.md: New file.
> * config/pru/predicates.md: New file.
> * config/pru/pru-ldst-multiple.ml: New file.
> * config/pru/pru-opts.h: New file.
> * config/pru/pru-passes.c: New file.
> * config/pru/pru-pragma.c: New file.
> * config/pru/pru-protos.h: New file.
> * config/pru/pru.c: New file.
> * config/pru/pru.h: New file.
> * config/pru/pru.md: New file.
> * config/pru/pru.opt: New file.
> * config/pru/t-pru: New file.
> * doc/extend.texi: Document PRU pragmas.
> * doc/invoke.texi: Document PRU-specific options.
> * doc/md.texi: Document PRU asm constraints.
Joseph has already made some notes about diagnostics. Those will need
to be addressed.
A couple global comments on style issues.
First, each function should have a comment describing what it does,
ideally describing the input parameters and output value.
Second the function definition should always look like
[static] <type>
name (type1 arg1, type2 arg2)
{
body
}
In some cases you've joined the linkage/type line with the function's
name. Can you please review pru.c in particular to fix these issues.
There's been another round of rtx -> rtx_insn * conversions done in the
generic parts of the compiler. There's a reasonable chance they will
require trivial updates to the pru port. When the time comes for
integration the trivial changes to cope with those conversions are
pre-approved and will not require a separate review cycle.
I've already asked about your copyright assignment status. So you know,
we can't go forward with any nontrivial contributions until the
assignment is in place.
I'm going to assume that you plan to maintain this port. Ideally you'll
have it using an auto-builder and posting tests gcc-testresults :-0
I'm assuming that since you're patching LRA in a different patch that
you're using LRA rather than reload :-) That also implies that you're
not using the deprecated cc0 bits, which is good.
>
> diff --git a/configure.ac b/configure.ac
> index 28155a0e593..684a7f58515 100644
> --- a/configure.ac
> +++ b/configure.ac
[ ... ]
So it looks like you're supporting libgloss/newlib. Does it work with
the current libgloss/newlib trunk? I've had troubles over the last few
months with 16 bit targets.
> diff --git a/gcc/common/config/pru/pru-common.c
> b/gcc/common/config/pru/pru-common.c
> new file mode 100644
> index 00000000000..e87d70ce9ca
> --- /dev/null
> +++ b/gcc/common/config/pru/pru-common.c
[ ... ]
> @@ -0,0 +1,36 @@
> +
> +#undef TARGET_EXCEPT_UNWIND_INFO
> +#define TARGET_EXCEPT_UNWIND_INFO sjlj_except_unwind_info
SJLJ exceptions? Is there some reason you're not using dwarf2 unwind
info? It may mean adding some notes in the prologue generation code,
but otherwise I'd expect it to "just work".
> +
> +struct gcc_targetm_common targetm_common = TARGETM_COMMON_INITIALIZER;
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index 8b2fd908c38..d1cddb86c36 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
[ ... ]
> +pru*-*-*)
> + tm_file="dbxelf.h elfos.h newlib-stdint.h ${tm_file}"
> + tmake_file="${tmake_file} pru/t-pru"
> + extra_objs="pru-pragma.o pru-passes.o"
> + use_gcc_stdint=wrap
Can you try to avoid dbxelf.h? We're trying to get away from embedded
stabs. I'd like to avoid creating new ports that reference this stuff.
> diff --git a/gcc/config/pru/pru-ldst-multiple.ml
> b/gcc/config/pru/pru-ldst-multiple.ml
> new file mode 100644
> index 00000000000..961a9fb33e6
> --- /dev/null
> +++ b/gcc/config/pru/pru-ldst-multiple.ml
> @@ -0,0 +1,144 @@
> +(* Auto-generate PRU load/store-multiple patterns
> + Copyright (C) 2014-2018 Free Software Foundation, Inc.
> + Based on arm-ldmstm.ml
[ ... ]
So please make sure to include the generated file in the repository. We
don't want to force developers to have to have ocaml installed to build
the port. I believe this is consistent with how the ARM port works.
> diff --git a/gcc/config/pru/pru-opts.h b/gcc/config/pru/pru-opts.h
> new file mode 100644
> index 00000000000..1c1514cb2a3
> --- /dev/null
> +++ b/gcc/config/pru/pru-opts.h
[ ... ]
> +
> +/* Definitions for option handling for PRU. */
> +
> +#ifndef GCC_PRU_OPTS_H
> +#define GCC_PRU_OPTS_H
> +
> +/* ABI variant for code generation. */
> +enum pru_abi {
> + PRU_ABI_GNU,
> + PRU_ABI_TI
Is there really any value in having two ABIs? If there's another
toolchain out there it'd be best if we were just compatible with that
rather than defining a GNU ABI. I guess it might be helpful if the TI
ABI is crippled enough that it's hard to run the testsuite..
> diff --git a/gcc/config/pru/pru.c b/gcc/config/pru/pru.c
> new file mode 100644
> index 00000000000..41b0a283d44
> --- /dev/null
> +++ b/gcc/config/pru/pru.c
f
> +
> + /* Unwind tables currently require a frame pointer for correctness,
> + see toplev.c:process_options(). */
> +
> + if ((flag_unwind_tables
> + || flag_non_call_exceptions
> + || flag_asynchronous_unwind_tables)
> + && !ACCUMULATE_OUTGOING_ARGS)
> + {
> + flag_omit_frame_pointer = 0;
> + }
!?! The comment doesn't make any sense. Dwarf2 unwinders can certainly
handle this case. What specific comment in process_options are you
referring to?
> +
> +/* Say that the epilogue uses the return address register. Note that
> + in the case of sibcalls, the values "used by the epilogue" are
> + considered live at the start of the called function. */
> +#define EPILOGUE_USES(REGNO) (epilogue_completed && \
> + (((REGNO) == RA_REGNO) \
> + || (REGNO) == (RA_REGNO + 1)))
Is this really needed? If you properly describe the dataflow for your
epilogues I don't think you need this hack.
> diff --git a/gcc/config/pru/pru.md b/gcc/config/pru/pru.md
> new file mode 100644
> index 00000000000..328fb484847
> --- /dev/null
> +++ b/gcc/config/pru/pru.md
> @@ -0,0 +1,905 @@
> +
> +; Combine phase tends to produce this expression when given the following
> +; expression: "uint dst = (ushort) op0 & (uint) op1;
> +;
> +; TODO - fix properly! Understand whether we need to fix combine core,
> augment
> +; our hooks, or PRU is giving incorrect costs for subexpressions.
> +(define_insn "*and_combine_zero_extend_workaround<mode>"
> + [(set (match_operand:SI 0 "register_operand" "=r")
> + (zero_extend:SI
> + (subreg:EQD
> + (and:SI
> + (match_operand:SI 1 "register_operand" "%r")
> + (match_operand:SI 2 "register_operand" "rI"))
> + 0)))]
> + ""
> + "and\\t%0, %1, %F2<EQD:regwidth>"
> + [(set_attr "type" "alu")])
You might want to revisit the need for this -- we went through a round
of fixes during the last couple release cycles to clean this stuff up.
Note I would have said something about this regardless of your TODO note
because of the subreg expression (they're certainly *valid* in machine
descriptions, but often a symptom of a problem elsewhere). This was the
only subreg expression I saw in your md file, which is good :-)
So the biggest issue is the desire to have more than 30 operands on some
insns. Can you describe in greater detail why that's beneficial.
jeff
