Re: RFA: contribute Synopsis DesignWare ARC port

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

On Thu, Nov 22, 2012 at 9:22 PM, Joern Rennecke wrote:
(nothing but a ChangeLog :-)

Looking at the ARC port a bit, and IMHO it doesn't look very messy...

First some general comments:

This target apparently wants to play tricks with reload (e.g.
arc_preserve_reload_p). IMHO at this point ports that don't work with
LRA should not be accepted.

TARGET_EXPAND_ADDDI is not covered in any test. Actually most target
options are not covered.

Can't the following option be just removed:
mold-di-patterns
Target Var(TARGET_OLD_DI_PATTERNS)
enable use of old DI patterns that have presumably been obsoleted by
subreg lowering.

There quite some old cruft that should be cleaned up. A few examples:

1. Things that probably worked before GCC3 but shouldn't be in a new port:
/*    /\* Compute LOG_LINKS.  *\/ */
/*    for (bb = 0; bb < current_nr_blocks; bb++) */
/*      compute_block_backward_dependences (bb); */

2. Old scheduler description:
;; Function units of the ARC

;; (define_function_unit {name} {num-units} {n-users} {test}
;;                       {ready-delay} {issue-delay} [{conflict-list}])
(etc...)

3. Presumably no longer applicable comments if this port is supposed
to be part of the FSF repo:
;; *** N.B.: the use of '* return...' in "*movsi_insn", et al, rely
;; on ARC-local changes to genoutput.c's process_template() function



Some random observations in config/arc/arc.c:
arc_dead_or_set_postreload_p() and the related functions is not used AFAICT.
Many functions (most?) have no leading comments.



About config/arc/arc.md:

What's this comment about?
;;  ashwin : include options.h from build dir
;; (include "arc.c")

The is_NON_SIBCALL attribute appears to be unused. If it can be
removed, then so can the is_SIBCALL attribute.
How is the "*millicode_sibthunk_ld" pattern used?

What are the commented-out patterns (e.g. "*adc_0") for?

Before the sibcall and return_i patterns:
; Since the demise of REG_N_SETS, it is no longer possible to find out
; in the prologue / epilogue expanders how many times blink is set.
I don't understand this, REG_N_SETS still exists.

C-style comment in a .md file (does that even work??):
/* Store a value to directly to memory.  The location might also be cached.
   Since the cached copy can cause a write-back at unpredictable times,
   we first write cached, then we write uncached.  */

Why are there patterns that have a "switch (which_alternative)" to
emit the insn template, instead of an "@..." string? See e.g.
(define_insn "*zero_extendhisi2_i" and "*zero_extendqisi2_ac".


It seems to me that a bit of cleaning up is in order before this port
is accepted.

Ciao!
Steven