Quoting Diego Novillo <dnovi...@google.com>:
On Sat, Sep 28, 2013 at 9:54 AM, Joern Rennecke
<joern.renne...@embecosm.com> wrote:
The main part of the port (everything but the testsuite) is still waiting
for review:
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00323.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00324.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00325.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00328.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01870.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02070.html
I have finished reading through these patches. They are OK to commit.
The changes indicated below are minor. Ideally, you'd address them
before committing the patch, but if it's easier to do it post-commit,
that's OK too.
Oops, I've already started my commit-spree after discussion on IRC.
- The Copyright years should be 2013 in every new file. Or has this
port been released before?
The port has been available via git for quite a while:
https://github.com/foss-for-synopsys-dwc-arc-processors/gcc
I've also added earlier versions as a branches in the FSF gcc svn repo
in 2008 and 2009.
- In config/arc/arc-protos.h:
+/* insn-attrtab.c doesn't include reload.h, which declares
regno_clobbered_p. */
+extern int regno_clobbered_p (unsigned int, rtx, enum machine_mode, int);
Why not include reload.h here? Interface changes (however rare) make
this a hassle.
There was also a general rule against including headers in header files,
although that has been weakened in the interim. Not sure what the exact
position now is.
At any rate, things that didn't use to depend on reload.h would suddenly do.
Not sure if the automatic dependencies take care of adding all the
new dependencies, but at any rate, this constrains the build.
And all the requirements for reload.h would also have to be included.
Worst of all, a number of generator programs includes tm_p.h, so if any
of the include files that have to be included for the sake of reload.h
is generated, there'll be a circular dependency. AFAICT, insn-modes.h
is a current example. And it is intrinsically needed for the prototype
I want.
Finally, tricks like the #ifdef RTX_CODE in function.h backfire when
the order of includes gets modified by includes from tm_p.h .
I'd rather have to copy a prototype in response to a clear error message
once in a blue moon than constantly fight with weird breakages of the
build system.
I suppose a better way would be for the *.md file that causes a header
file dependency to somehow request the inclusion of the header file by
the generated file(s).
- In config/arc/simdext.md
+;; Va, [Ib,u8] instructions
+;; (define_insn "vld32wh_insn"
+;; [(set (match_operand:V8HI 0 "vector_register_operand" "=v")
+;; (vec_concat:V8HI (unspec:V4HI [(match_operand:SI 1
"immediate_operand" "P")
+;; (vec_select:HI (match_operand:V8HI 2 "vector_register_operand" "v")
+;; (parallel [(match_operand:SI 3 "immediate_operand" "L")]))]
UNSPEC_ARC_SIMD_VLD32WH)
+;; (vec_select:V4HI (match_dup 0)
Necessary? If so, please add a comment stating why it's commented out.
As you can see in svn, this was already commented out in arc-20081210-branch;
ISTR that this was just a pair of UNSPEC patterns that could be replaced with
then-new vector operations; but the exact history is lost with the ARC svn
repo. At any rate, the replacement patterns are clearly below, so I deleted
the old commented out patterns along with their unspec constants.
- In doc/extend.texi:
+Permissible values for this parameter are: @w{@code{ilink1}} and
+@w{@code{ilink2}}.
+
ARC developers already know what ilink1 and ilink2 mean?
The ones that have to program interrupts should, or they can read about
them in the architecture manual.
These are two link registers (for interrupt return addresses) associated
with specific interrupt levels.
+@cindex indirect calls on Epiphany
+These attribute specifies how a particular function is called on
+ARC, ARM and Epiphany
s/specifies/specify/
+because __alignof__ sees only the type of the dereference, wheras
+__builtin_arc_align uses alignment information from the pointer
s/wheras/whereas/
Fixed.
- I have not fully cross-referenced the list of documented builtins vs
the list of implemented builtins. Please double check them.
- Ditto the list of -m options. It looks like they're all documented,
but I haven't diff'd the doc vs the options file.
I think we already did this, but these things have a way of having forgotten
items and/or grow new inconsistencies... I'll try to remember ot check
again...
- In libgcc/config/arc/gmon/mcount.c
The file has a different copyright/license notice at the top. Is this
from a third party source?
Yes, it is from one of the newer BSD releases. Can't recall exactly which,
but as you can see, I took care to use a base that has the three-clause
license, so there should be no issue with license compatibility.
Can it be changed to lgpl?
Nonetheless, the license says a condition is the reproduction of the license,
so the lgpl could only be added, not be used to replace the
Copyright notice/license.
+#if 0
+ if (catomic_compare_and_exchange_bool_acq (&p->state, GMON_PROF_BUSY,
+ GMON_PROF_ON))
+ return;
Get rid of this?
There are some newer ARC variants which support more atomic operations.
Once I get a suitable test platform and have some time for it, I'd
like to use a suitable atomic operation there, for configurations
where this is possible and relevant.
+#elif defined (__ARC700__)
+/* ??? This could temporrarily loose the ERROR / OFF condition in a race,
s/temporrarily/temporarily/
s/loose/lose/
- Many files in libgcc/config/arc/... have #if0 blocks. Are they
really necessary?
ARCompact is a rather CISCy RISC. There are often lots of ways to express
the same thing. To find the best one, you have to optimize each of
the promising candidates and then compare how they perform on the
current
microarchitecture / with the current memory subsystem. Some of which is
unknown, and all of which is subject to change. So yesterday's runner-up
algorithm could become tomorrows winner.
- In libgcc/config/arc/ieee-754/arc600-dsp/muldf3.S
+/* We have checked for infinitey / NaN input before, and transformed
+ denormalized inputs into normalized inputs. Thus, the worst case
s/infinitey/infinity/
It also happens in another muldf3.S file in a sibling directory.
And in the parent directory. Fixed.
- libgcc/config/arc/t-arc-newlib does not contain the exception clause.
We got 16 files libgcc/config/*/t-* mentioning libgcc (3 arc ones), only
t-arc700-uClibc has the exception. Shouldn't I rather delete it there?
The t- makefile fragments are not code that users would link into their
programs, but statements that are interpreted by make when the library
is built.
- In config/arc/arc.md there are several define patterns commented
out. Toss them out?
IIRC, adc_0 / adc_0+1 were attempts to recognize C idioms for carry bit
usage, but didn't help due to some flaw in combine. I've added a comment
to that effect.
The commented out doloop_begin_i illustrates an intuitive pattern that
falls foul of over-zealous optimizers, and likewise the commented out
negdf2 shows how it'd look like in order to explain why it isn't used.
AFAICS, that just leaves call_value_via_label_mixed, which was just an
ill-conceived way to use bl_s, so I replaced that with a comment about
bl_s.
- In config/arc/arc.c:
No need to include stdio.h
No need to mark struct arc_frame_info with GTY. It contains no pointers.
In arc_expand_epilogue():
Get rid of fp_restored_p
if (1)
{
unsigned int pretend_size = cfun->machine->frame_info.pretend_size;
Just move everything out of the if()?
Oops, that was a relic of supporting older variants. Never quite knew when
to take the history hit of re-indenting everything - plus the variables
would have had to be moved. Well, at least the latter is no longer needed
since we switched the implementation language.
In output_shift(): Get rid of the #if 1s?
I'm not sure if it should be #if 1, #if 0, or if (TARGET_FOO).
As the comment says, I'd need some scheduling comparisons.
I'll keep tying to get more data on this.
In arc_encode_section_info():
/* Symbols in the text segment can be accessed without indirecting via the
constant pool; it may take an extra binary operation, but this is still
faster than indirecting via memory. Don't do this when not optimizing,
since we won't be calculating al of the offsets necessary to do this
simplification. */
But that seems out of sync with the code. It never checks whether
optimizations are enabled.
Oops. This was never a comment for ARC. It seems to have been copied over
from ARM; it was kept at a distance by an ARC specific comment, which I
deleted along with the commented-on code when that was obsoleted by changes
in the supported hardware and in GCC.
Sort of resembles the joke that every program can be reduced to a
single instruction that doesn't work.
