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.