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.
- The Copyright years should be 2013 in every new file. Or has this
port been released before?
- 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.
- 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.
- 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?
+@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/
- 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.
- 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? Can it be changed to lgpl?
+#if 0
+ if (catomic_compare_and_exchange_bool_acq (&p->state, GMON_PROF_BUSY,
+ GMON_PROF_ON))
+ return;
Get rid of this?
+#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?
- 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.
- libgcc/config/arc/t-arc-newlib does not contain the exception clause.
- In config/arc/arc.md there are several define patterns commented
out. Toss them out?
- 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()?
In output_shift(): Get rid of the #if 1s?
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.
Thanks. Diego.
