On петък, 22 юни 2018 г. 12:20:46 EEST Jeff Law wrote:
> 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.
I'll fix these in patch v2.
>
[...]
>
> 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.
Yes, FSF has my assignment since November 2016.
>
> 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
Yes, I'm willing to maintain it. To be fair, it is entirely in my own spare
time. I've been writing, rewriting and maintaning this out-of-tree port for
the past 4 years. I believe I'll have time and will to continue doing so for
the foreseeable future.
Emailing the results would actually be easier for me than maintaining my own
testresults web page. Thanks.
>
> 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.
Yes, port is using LRA. No cc0.
>
> > 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.
I have not detected issues with top-of-tree. But keep in mind that PRU is 8-
bit only for the GCC internal representation. The port declares efficient 32-
bit ops for SImode. From newlib's point of view, PRU is native 32-bit.
Some history here: http://gcc.gnu.org/ml/gcc/2017-01/msg00217.html
>
> > 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".
I would like to avoid increasing memory footprint. I saw that AVR folks use
it, too. My understanding is that eh_frame data will have to be included for
each function when using dwarf2 unwind.
For reference, typical data memory size for PRU is 4kb.
>
> > +
> > +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.
This was copy-pasta, sorry. Will remove for v2.
>
> > 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.
Will send it with the v2 patch set.
>
> > 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..
TI ABI defines data pointers as 32-bit and function pointers as 16-bit. I
could not implement this reliably for the PRU GCC port. And it doesn't seem to
be a good idea anyway:
http://gcc.gnu.org/ml/gcc/2012-04/msg00870.html
My GCC implementation for TI ABI does not support function pointers and large
return values. Hence I defined GNU ABI where those are supported, users can
enjoy full C language capabilities, and a decent testsuite coverage is
achieved.
>
> > 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?
A while ago I copied this snippet from AVR port in order to fix a testsuite
case failure. I'll recheck and either remove it, or update the comment.
>
> > +
> > +/* 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.
Yes it is needed. I saw that nios2 port uses it, and I had no idea it is a
hack.
I'll need some time to research how to fix it. Pointers or suggestions for a
properly designed GCC backend would be appreciated.
>
> > 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 :-)
Sure, I'll retest.
>
>
> 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.
I answered in the thread with change itself.
Thanks,
Dimitar
