[1/3] Re-submission of Altera Nios II port, gcc parts

Hi Bernd,
My response to the various issues you raised are below. The new patch
has been re-tested. Please see if you can approve for committing now.

Thanks,
Chung-Lin

On 13/10/17 10:13 PM, Bernd Schmidt wrote:
> On 07/14/2013 09:54 AM, Chung-Lin Tang wrote:
>> Hi, the last ping of the Nios II patches was:
>> http://gcc.gnu.org/ml/gcc-patches/2013-06/msg01416.html
>>
>> After assessing the state, we feel it would be better to post a
>> re-submission of the newest patches.
> 
> Since this hasn't attracted attention for months I'll have a go at a
> review. I was not involved in this project and haven't seen this code
> before.
> 
>> @@ -4196,6 +4203,7 @@ esac
>>  # version to the per-target configury.
>>  case "$cpu_type" in
>>    alpha | arm | avr | bfin | cris | i386 | m32c | m68k | microblaze | mips \
>> +  | nios2 \
>>    | pa | rs6000 | score | sparc | spu | tilegx | tilepro | xstormy16 | xtensa)
>>      insn="nop"
> 
> Could maybe format this nicer to fill up all but the last line.

Fixed after merge with latest trunk.

>> +;; These are documented for use in inline asm.
>> +(define_register_constraint "D00" "D00_REG" "Hard register 0.")
>> +(define_register_constraint "D01" "D01_REG" "Hard register 1.")
>> +(define_register_constraint "D02" "D02_REG" "Hard register 2.")
> [...]
> 
> This doesn't strike me as a good idea. To really make this work for
> cases where people write things like "D28D13" you'd have to provide all
> the union classes as well which is obviously infeasible. There are
> alternative mechanisms to get assembly to use the registers you want,
> and I'll note that libgcc seems to be using those instead of these
> constraints.
> 
> This probably also slows down the compiler, and it might be worth an
> experiment to see whether deleting these has any effect on register
> allocation.

These constraints/regclasses have been removed.

>> +;; We use the following constraint letters for constants
>> +;;
>> +;;  I: -32768 to -32767
>> +;;  J: 0 to 65535
> 
>> +(define_insn "movhi_internal"
>> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=m, r,r, r,r")
>> +        (match_operand:HI 1 "general_operand"       "rM,m,rM,I,J"))]
> 
> The J alternative is unnecessary. The range 32768...65535 doesn't give a
> valid HImode constant, the RTL representation of integer constants
> always sign extends. Probably you can just use "i" here and in
> movqi_internal.

I'm not sure that 'i' here serves the needed constraint, but I removed
'J' here.

>> +(define_insn "movsi_internal"
>> +  [(set (match_operand:SI 0 "nonimmediate_operand" "=m, r,r, r,r,r,r,r")
>> +        (match_operand:SI 1 "general_operand"       "rM,m,rM,I,J,K,S,i"))]
> 
> Not required, but as an experiment you might want to reorder the
> alternatives and sprinkle extra "r"s to get optional reloads and see
> whether that improves code generation. You'd have
> 
> (match_operand:SI 0 "nonimmediate_operand" "=r,rm,r ,r ,r ,r ,r ,r")
> (match_operand:SI 1 "general_operand"      "rM,rM,rm,rI,rJ,rK,rS,ri"))]
> 
> with the register-register move first. It might not make a difference.

Tried experimenting a bit, so far haven't seen any improvements. I'll
keep this as is for now.

>> +;; Split patterns for register alternative cases.
>> +(define_split
>> +  [(set (match_operand:SI 0 "register_operand" "")
>> +        (sign_extend:SI (match_operand:HI 1 "register_operand" "")))]
> 
> Could merge into a define_insn_and_split?

This is a post-reload split that only applies to the "r = r"
alternative. ARM also has similar patterns (although they have multiple
such splitters for the various arch-levels)

>> +  "reload_completed"
>> +  [(set (match_dup 0)
>> +        (and:SI (match_dup 1) (const_int 65535)))
>> +   (set (match_dup 0)
>> +        (xor:SI (match_dup 0) (const_int 32768)))
>> +   (set (match_dup 0)
>> +        (plus:SI (match_dup 0) (const_int -32768)))]
>> +  "operands[1] = gen_lowpart (SImode, operands[1]);")
> 
> Is this really faster than two shifts (either as expander or splitter)?

As the scheduler descriptions indicate, shifts have a long latency.
Anyways, this was inherited from the old Altera code.

>> +;; Arithmetic Operations
>> +
>> +(define_insn "addsi3"
>> +  [(set (match_operand:SI 0 "register_operand"          "=r")
>> +        (plus:SI (match_operand:SI 1 "register_operand" "%r")
>> +                 (match_operand:SI 2 "arith_operand"   "rIT")))]
>> +  ""
>> +  "add%i2\\t%0, %1, %z2"
>> +  [(set_attr "type" "alu")])
> 
> [...]
> 
>> +(define_insn "mulsi3"
>> +  [(set (match_operand:SI 0 "register_operand"          "=r")
>> +        (mult:SI (match_operand:SI 1 "register_operand" "%r")
>> +                 (match_operand:SI 2 "arith_operand"    "rI")))]
>> +  "TARGET_HAS_MUL"
>> +  "mul%i2\\t%0, %1, %z2"
>> +  [(set_attr "type" "mul")])
> 
> There seems to be a slight mismatch here between arith_operand and the
> constraints. Unlike in addsi3, "T" (which calls nios2_unspec_reloc_p)
> isn't allowed here and in the other uses of arith_operand. That suggests
> that arith_operand should be split into two different predicates, one
> for addsi3 and one for all the other uses.

I have added a new 'add_regimm_operand' and used it for this case. Note
that this change has somewhat effects on generated code under -fpic,
although the overall code quality appears to be the same.

>> +(define_expand "divsi3"
>> +  [(set (match_operand:SI 0 "register_operand"          "=r")
>> +        (div:SI (match_operand:SI 1 "register_operand"   "r")
>> +                (match_operand:SI 2 "register_operand"   "r")))]
>> +  ""
>> +{
>> +  if (!TARGET_HAS_DIV)
>> +    {
>> +      if (!TARGET_FAST_SW_DIV)
>> +        FAIL;
>> +      else
>> +        {
>> +          if (nios2_emit_expensive_div (operands, SImode))
>> +            DONE;
>> +        }
>> +    }
>> +})
> 
> Shouldn't this FAIL if !nios2_emit_expensive_div (ok so that function
> never returns 0 but still...)?

Fixed, nios2_emit_expensive_div() doesn't need a return value.

>> +;;  Integer logical Operations
>> +
>> +(define_code_iterator LOGICAL [and ior xor])
>> +(define_code_attr logical_asm [(and "and") (ior "or") (xor "xor")])
>> +
>> +(define_insn "<code>si3"
>> +  [(set (match_operand:SI 0 "register_operand"             "=r,r,r")
>> +        (LOGICAL:SI (match_operand:SI 1 "register_operand" "%r,r,r")
>> +                    (match_operand:SI 2 "logical_operand"  "rM,J,K")))]
>> +  ""
>> +  "@
>> +    <logical_asm>\\t%0, %1, %z2
>> +    <logical_asm>%i2\\t%0, %1, %2
>> +    <logical_asm>h%i2\\t%0, %1, %U2"
>> +  [(set_attr "type" "alu")])
>> +
>> +(define_insn "*norsi3"
>> +  [(set (match_operand:SI 0 "register_operand"                  "=r")
>> +        (and:SI (not:SI (match_operand:SI 1 "register_operand"  "%r"))
>> +                (not:SI (match_operand:SI 2 "reg_or_0_operand"  "rM"))))]
>> +  ""
>> +  "nor\\t%0, %1, %z2"
>> +  [(set_attr "type" "alu")])
> 
> M constraints (for const0_rtx) and reg_or_0 seem unnecessary, no such
> RTL should make it to this point.

Such RTL does appear under -O0. Removing the 'M' will also
require a bit of re-working the operand printing mechanics; not a lot of
work, but I'd rather keep it as is. The behavior of using the zero
register for a 0-value is also more expected in nios2, I think.

>> +;; Floating point instructions
> 
> Ok, all this stuff is scary, but it seems to be dictated by the hardware.
> 
>> +  "* return nios2_fpu_insn_asm (n2fpu_f<fop3><f>);"
> 
> Minor point - that's an older form, personally I prefer
> {
>   return nios2_fpu_insn_asm (n2fpu_f<fop3><f>);
> }
> over C-in-a-string, but I'm not sure there's any sort of consensus that
> we shouldn't be doing the former.

I originally used the "* ..." form because I wanted to shorten this part
to within a single line, and looks like { ... } works as well. I have
changed those to use the brace form now.

>> +(define_insn "return"
>> +  [(return)]
>> +  "nios2_can_use_return_insn ()"
>> +  "ret"
>> +)
> 
> Could try to also define "simple_return" to get shrink-wrapping. Move
> closing paren to previous line, here and elsewhere. Define a type?

Shrink wrap implemented. My experience was that the dwarf2 generation
pass often borked if shrink-wrap was enabled while epilogue unwinding
wasn't implemented, so the latter was done as well.

>> +(define_expand "call"
>> +  [(parallel [(call (match_operand 0 "" "")
>> +                    (match_operand 1 "" ""))
>> +              (clobber (reg:SI 31))])]
>> +  ""
>> +  "nios2_adjust_call_address (&operands[0]);"
>> +)
> 
> Weird. Why isn't r31 (return address) in CALL_USED_REGS? Should do that
> and lose the clobber unless there's a very good reason.

I now have added r31 to CALL_USED_REGS, which somewhat improved the code
a bit, thanks for catching that.

As for whether the call insn pattern should clobber the RA register,
looking around I see several other targets with this practice; if it
wasn't clobbered, then RA would actually not show up in
df_regs_ever_live_p(), although a workaround would be to add a check of
crtl->is_leaf, like I see in the c6x port.

If you don't object, I'll keep the clobbers there for now.

>> +  [(set_attr "type" "control,control")])
> 
> I think just "control" would be better for the type. Multiple instances.

Done.

>> +;; Integer comparisons
>> +
>> +(define_code_iterator EQNE [eq ne])
>> +(define_insn "nios2_cmp<code>"
>> +  [(set (match_operand:SI 0 "register_operand"           "=r")
>> +        (EQNE:SI (match_operand:SI 1 "reg_or_0_operand" "%rM")
>> +                 (match_operand:SI 2 "arith_operand"     "rI")))]
>> +  ""
>> +  "cmp<code>%i2\\t%0, %z1, %z2"
>> +  [(set_attr "type" "alu")])
> 
> Once again, using reg_or_0 and "M" seems pointless.

The compares don't support all operations, with only the second operand
capable of an immediate. Using 'rM' should theoretically allow more
commutative swapping.

>> +(define_insn "trap"
>> +  [(unspec_volatile [(const_int 0)] UNSPECV_TRAP)]
> 
> There's a trap_if rtl code.

Changed to use a trap_if pattern now.

>> +(define_insn "stack_overflow_detect_and_trap"
>> +  [(unspec_volatile [(const_int 0)] UNSPECV_STACK_OVERFLOW_DETECT_AND_TRAP)]
> 
> Likewise. Might even be able to represent the condition.

Done. Changed the stack limit checking to use a ctrap pattern.

>> +/* Local prototypes.  */
> 
> I'd much prefer not to have any of those. Achieve this by putting
>> +struct gcc_target targetm = TARGET_INITIALIZER;
> along with all the necessary definitions at the end of the file (and
> reordering some other functions).

I would rather keep it that way. The ARM backend is another example of this.

>> +/* ??? Might want to redefine TARGET_RETURN_IN_MSB here to handle
>> +   big-endian case; depends on what ABI we choose.  */
> 
> This comment needs to be addressed and deleted.

Comment deleted. Nios2 doesn't yet officially support big-endian, though
we did keep any bits of past attempted BE support code intact.

>> +/* Stack Layout and Calling Conventions */
> 
> Slightly Too Many Caps I Think. Period and two spaces to end comment.
> Elsewhere too for these kinds of section comments.
> 
>> +#define TOO_BIG_OFFSET(X) ((X) > ((1 << 15) - 1))
> 
> Nitpicky, but I don't like the name. I'd rather have a
> VALID_ADD_OFFSET_P macro and use its negation.

I am now using !SMALL_INT instead.

>> +static void
>> +save_reg (int regno, unsigned offset)
> 
> Strictly speaking these functions should all have comments describing
> their args and such.

Done.

>> +void
>> +expand_prologue (void)
> 
> This and other globally-visible functions need renaming to have a nios2_
> prefix to avoid confusion.

Done.

>> +{
>> +  int ix;
>> +  HOST_WIDE_INT total_frame_size = compute_frame_size ();
> 
>> +  total_frame_size = compute_frame_size ();
> 
> Done twice. Maybe nios2_compute_frame_layout would be a better name
> since it does more than compute the size.

Done.

>> +  for (ix = 32; ix--;)
>> +    if (save_mask & ((unsigned HOST_WIDE_INT)1 << ix))
> 
> You shouldn't need to cast to HOST_WIDE_INT, just 1u will do (and
> save_mask can be unsigned int).

Reorganized this.

>> +      emit_insn
>> +	(gen_rtx_SET (Pmode, tmp,
>> +		      gen_int_mode (cfun->machine->save_regs_offset,
>> +				    Pmode)));
> 
> Shouldn't have a mode on the SET, but really should just call
> emit_move_insn. Similar cases elsewhere, search for "gen_rtx_SET (Pmode".

I've removed the modes on SET, though I prefer the more bare generation
of the insns in some contexts; emit_move_insn() seems to have a lot
under the hood.

>> +nios2_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
> 
> There's now an ARG_UNUSED macro; I'm not sure whether we're supposed to
> use that or just omit the arg name now that we're using C++.

Grepping around, I see ARG_UNUSED() rarely used currently. I guess
ATTRIBUTE_UNUSED is okay.

>> +/* Return true if REGNO should be saved in a prologue.  */
> 
> "the" prologue?

For the record, I didn't write this comment :)

>> +/* Return the bytes needed to compute the frame pointer from the current
>> +   stack pointer.  */
>> +static HOST_WIDE_INT
>> +compute_frame_size (void)
> 
> Needs renaming to at least prepend nios2_; see above. Comment needs
> expanding since the function does more than what it says.
> 
>> +  HOST_WIDE_INT var_size;       /* # of var. bytes allocated.  */
> 
> Not the first time they occur in this file, but I suppose I should
> mention that these in-line comments are probably just outside our coding
> guidelines.

Deleted the comments outside the struct defs.

>> +  /* Calculate space needed for gp registers.  */
>> +  for (regno = 0; GP_REGNO_P (regno); regno++)
>> +    if (save_reg_p (regno))
>> +      {
>> +	save_mask |= (unsigned HOST_WIDE_INT)1 << regno;
>> +	save_reg_size += 4;
>> +      }
> 
> Not sure I like GP_REGNO_P (regno) as the loop exit condition here.

Changed a bit, see if you like it now :)

>> +  /* If we call eh_return, we need to save the EH data registers.  */
>> +  if (crtl->calls_eh_return)
> 
> Fold this case into save_reg_p maybe? Doesn't matter too much.

Keeping as is...

>> +/* Return nonzero if this function is known to have a null epilogue.
>> +   This allows the optimizer to omit jumps to jumps if no stack
>> +   was created.  */
>> +int
>> +nios2_can_use_return_insn (void)
>> +{
>> +  if (!reload_completed)
>> +    return 0;
>> +
>> +  if (df_regs_ever_live_p (RA_REGNO) || crtl->profile)
>> +    return 0;
>> +
>> +  if (cfun->machine->initialized)
> 
> I think this could be an assert and the if branch unconditional.
> 
>> +    return cfun->machine->total_size == 0;

I simply deleted the "if (cfun->machine->initialized)" clause. An
equivalent check is already in the compute frame routine. The
df_regs_ever_live_p(RA) check here seems unneeded as well.

>> +static void
>> +nios2_set_fpu_custom_code (enum n2fpu_code code, int N, bool override_p)
> 
> Again, add comments for all these. I haven't looked too hard at the
> custom code stuff since it seems unavoidably horrid but doesn't really
> affect anything outside it.
> 
> Parameter N should be lowercase.
> 
>> +  if (!strncasecmp (cfg, "60-1", 4))
> 
> strcmp, several times. At least judging by the docs allowing "60-1fish"
> is unintentional?

I changed them to use strncmp instead. This routine has to work on a
possibly longer target attribute string, hence the 'n' variants.

>> +  /* Check for unsupported options.  */
>> +  if (flag_pic && !TARGET_LINUX_ABI)
>> +    error ("position-independent code requires the Linux ABI");
> 
> Use sorry instead?

Any guidelines on error() vs. sorry()? I don't feel a strong need to
change...

>> +/* Return true if CST is a constant within range of movi/movui/movhi.  */
>> +static bool
>> +nios2_simple_const_p (const_rtx cst)
>> +{
>> +  HOST_WIDE_INT val = INTVAL (cst);
>> +  return (SMALL_INT (val) || SMALL_INT_UNSIGNED (val) || UPPER16_INT (val));
>> +}
> 
> Unnecessary parens around return value. Several cases in this file.

Grepping across the backends, I see numerous such parens around return
value, so I'm leaning on keeping that...

>> +      case CONST_INT:
>> +        if (INTVAL (x) == 0)
>> +          {
>> +            *total = COSTS_N_INSNS (0);
>> +            return true;
>> +          }
>> +        else if (nios2_simple_const_p (x))
>> +          {
>> +            *total = COSTS_N_INSNS (2);
>> +            return true;
> 
> Hmm, really? What about insns that accept a constant as operand,
> shouldn't it be zero cost for those?
> 
>> +      case MULT:
>> +        {
>> +          *total = COSTS_N_INSNS (1);
>> +          return false;
>> +        }
> 
> Really?

This was not really re-worked since the inherited Altera code, so some
are probably sort of weird, but I'd like to work on this cost tuning later.

>> +static rtx
>> +nios2_call_tls_get_addr (rtx ti)
>> +{
>> +  rtx arg = gen_rtx_REG (Pmode, FIRST_ARG_REGNO);
>> +  rtx ret = gen_rtx_REG (Pmode, FIRST_RETVAL_REGNO);
>> +  rtx fn, insn;
>> +  
>> +  if (!nios2_tls_symbol)
>> +    nios2_tls_symbol = init_one_libfunc ("__tls_get_addr");
>> +
>> +  emit_insn (gen_rtx_SET (Pmode, arg, ti));
>> +  fn = gen_rtx_MEM (QImode, nios2_tls_symbol);
>> +  insn = emit_call_insn (gen_call_value (ret, fn, const0_rtx));
>> +  RTL_CONST_CALL_P (insn) = 1;
>> +  use_reg (&CALL_INSN_FUNCTION_USAGE (insn), ret);
>> +  use_reg (&CALL_INSN_FUNCTION_USAGE (insn), arg);
>> +
>> +  return ret;
>> +}
> 
> No special relocs to mark such a call?

I don't quite understand what you mean here? The "call_value" pattern
should handle everything correctly.

>> +int
>> +nios2_emit_move_sequence (rtx *operands, enum machine_mode mode)
> 
> Hmm, I would have guessed that most targets would call this
> nios2_expand_move, but grep shows that the two variants are about
> equally common.
> 
>> +/* Divide Support */
>> +
>> +/*
>> +  If -O3 is used, we want to output a table lookup for
> 
> Watch comment formatting.
> 
>> +  divides between small numbers (both num and den >= 0
>> +  and < 0x10).  The overhead of this method in the worse
>> +  case is 40 bytes in the text section (10 insns) and
> 
> "worst case", everywhere.
> 
>> +  256 bytes in the data section.  Additional divides do
>> +  not incur additional penalties in the data section.
>> +
>> +  Code speed is improved for small divides by about 5x
>> +  when using this method in the worse case (~9 cycles
>> +  vs ~45).  And in the worse case divides not within the
>> +  table are penalized by about 10% (~5 cycles vs ~45).
>> +  However in the typical case the penalty is not as bad
>> +  because doing the long divide in only 45 cycles is
>> +  quite optimistic.
>> +
>> +  ??? It would be nice to have some benchmarks other
>> +  than Dhrystone to back this up.
> 
> Um, ok. This is an optimization for dhrystone? That's... interesting.
> 
>> +  ??? Ideally I would like the emit libcall block to contain
> 
> Lose "emit".

Shaped up the comment formatting a bit.

>> +void
>> +nios2_adjust_call_address (rtx *call_op)
>> +{
>> +  rtx addr;
>> +  gcc_assert (MEM_P (*call_op));
>> +  addr = XEXP (*call_op, 0);
>> +  if (flag_pic && CONSTANT_P (addr))
>> +    {
>> +      rtx reg = gen_reg_rtx (Pmode);
>> +      emit_move_insn (reg, nios2_load_pic_address (addr, UNSPEC_PIC_CALL_SYM));
>> +      XEXP (*call_op, 0) = reg;
>> +    }
>> +}
>> +
>> +
>> +
> 
> Some excess newlines here.
> 
>> +/* Branches/Compares.  */
>> +
>> +/* Return in *ALT_CODE and *ALT_OP, an alternate equivalent constant
>> +   comparision, e.g. >= 1 into > 0.  */
> 
> "comparison".
> 
>> +static void
>> +nios2_alternate_compare_const (enum rtx_code code, rtx op,
>> +			       enum rtx_code *alt_code, rtx *alt_op,
>> +			       enum machine_mode mode)
>> +{
>> +  HOST_WIDE_INT opval = INTVAL (op);
>> +  enum rtx_code scode = signed_condition (code);
>> +  *alt_code = ((code == EQ || code == NE) ? code
>> +	       /* The required conversion between [>,>=] and [<,<=] is captured
>> +		  by a reverse + swap of condition codes.  */
>> +	       : reverse_condition (swap_condition (code)));
>> +  *alt_op = ((scode == LT || scode == GE) ? gen_int_mode (opval - 1, mode)
>> +	     : (scode == LE || scode == GT) ? gen_int_mode (opval + 1, mode)
> 
> That's not really equivalent in all cases, right? I don't see anything
> dealing with boundary conditions. It might be that the cases where it
> would fail are already optimized to always-true or always-false, but
> still...
> Also, it doesn't look like this does the right thing for alt_op in the
> csae of unsigned comparisons?

The boundary conditions do seem to always be optimized into
always-true/false or EQ cases, even under -O0.  I have changed the code
to add an assertion to check over/underflows for INTVAL(op) +1/-1.
Supposedly, the code should never need to handle this.

>> +/* Checks if the FPU comparison in *CMP, *OP1, and *OP2 can be supported in
>> +   the current configuration. Perform modifications if MODIFY_P is true.
> 
> Double space. Multiple occurrences in this file.

Fixed across.

>> +/* Return true if register REGNO is a valid index register.
>> +   STRICT_P is true if REG_OK_STRICT is in effect.  */
> 
> It's a bit strange to have definitions for index registers given that
> MAX_REGS_PER_ADDRESS is defined to 1 for this port. One or the other
> must be wrong.

Thanks for catching this, I've made REGNO_OK_FOR_INDEX_P return 0, and
INDEX_REG_CLASS as NO_REGS now.

>> +    case LABEL_REF:
>> +    case CONST_INT:
>> +    case CONST:
>> +    case CONST_DOUBLE:
>> +      /* ??? In here I need to add gp addressing.  */
>> +      ret_val = false;
> 
> Address this comment.
> 
>> +  return ret_val;
> 
> Could really just return directly from the switch cases without the
> extra variable.

Done.

>> +      /* ??? these string names need moving into
>> +         an array in some header file */
> 
> Don't think so. Lose comment or fix it up to conform to coding style.
> 
>> +/* Implement TARGET_ENCODE_SECTION_INFO.  */
> 
> This function together with
> 
>> +/* Set if this has a weak declaration.  */
>> +#define SYMBOL_FLAG_WEAK_DECL   (1 << SYMBOL_FLAG_MACH_DEP_SHIFT)
>> +#define SYMBOL_REF_WEAK_DECL_P(RTX) \
>> +  ((SYMBOL_REF_FLAGS (RTX) & SYMBOL_FLAG_WEAK_DECL) != 0)
> 
> seems a little odd, can the latter not simply be
>   SYMBOL_REF_DECL (RTX) ? DECL_WEAK (SYMBOL_REF_DECL (RTX)) : 0
> rather than mucking about with symbol flags?

Refactored all related code; TARGET_ENCODE_SECTION_INFO is no longer
implemented.

>> +/* Print the operand OP to file stream
>> +   FILE modified by LETTER. LETTER
>> +   can be one of:
> 
> Strange line breaks.
> 
>> +      if (CONSTANT_P (op) && (op != const0_rtx))
> 
> Lose extra parens.
> 
>> +  fprintf (stderr, "Missing way to print (%c) ", letter);
>> +  debug_rtx (op);
>> +  gcc_unreachable ();
>> +}
> 
> and
> 
>> +  fprintf (stderr, "Missing way to print address\n");
>> +  debug_rtx (op);
>> +  gcc_unreachable ();
> 
> Should use output_operand_lossage since this can be caused by user error
> in asms.

Done.

>> +static const char*
> 
> Space. Multiple occurrences.

Fixed across.

>> +/* Instruction scheduler related.  */
>> +
>> +static int
>> +nios2_issue_rate (void)
>> +{
>> +#ifdef MAX_DFA_ISSUE_RATE
>> +  return MAX_DFA_ISSUE_RATE;
>> +#else
>> +  return 1;
>> +#endif
>> +}
> 
> Hmm. If it's just 1 in reality then the default definition is sufficient
> and we don't need this function.

I didn't notice that MAX_DFA_ISSUE_RATE has been deprecated for so long.
I have removed this function now.

>> +  if (mode == BLKmode)
>> +    {
>> +      param_size = int_size_in_bytes (type);
>> +      gcc_assert (param_size >= 0);
>> +      /*
>> +      if (param_size < 0)
>> +        internal_error
>> +          ("Do not know how to handle large structs or variable length types");
>> +      */
> 
> Deal with the problem if necessary then remove both copies of the
> commented-out code.

Argh, thanks for catching this.

>> +  return;
> 
> Lose this (in arg_advance).

Deleted.

>> +  /* ??? Do we need to treat floating point specially, ala MIPS?  */
> 
> Do you?

Comment deleted.

>> +  return (regno == FIRST_RETVAL_REGNO);
> 
> Parens.
> 
>> +/* ??? It may be possible to eliminate the copyback and implement
>> +   my own va_arg type, but that is more work for now.  */
> 
> Function could use a different comment.

Slightly modified the phrasing.

>> +  /* This way of constructing the function tree types will slightly 
>> +     overlap with the N2_FTYPES list used by other builtins.  */
> 
> What does that mean?

I use a looped description for constructing the function tree types used
by the XnXX builtins, where n is an int, and each X may be an int,
pointer, or float; the first X means the return type.

Some of these function types may overlap with the ones built for the
other "fixed" functions, which I believe shouldn't matter, but noting in
the comment. I'll try to re-phrase the wording a bit.

>> +	  if (!custom_insn_opcode (value, VOIDmode))
>> +	    error ("Custom instruction opcode must be compile time "
>> +		   "constant in the range 0-255 for __builtin_custom_%s",
>> +		   custom_builtin_name[index]);
> 
> I think errors should start with lower case.

Done.

>> +  emit_insn (insn);
>> +  return has_target_p ? target : const0_rtx;
>> +}
>> +
>> +
>> +
>> +
> 
> More excess newlines.
> 
>> +/* Implement TARGET_INIT_BUILTINS.  */
> 
> It looks like in this area we're missing a definition of
> TARGET_BUILTIN_DECL. AFAIR not defining that breaks LTO, do you see such
> a problem on this target?

Done adding a TARGET_BUILTIN_DECL implementation.

>> +nios2_expand_builtin_insn (const struct nios2_builtin_desc *d, int n,
>> +			   struct expand_operand* ops, bool has_target_p)
> 
> Again, space before "*", not after.
> 
>> +static rtx
>> +nios2_expand_ldstio_builtin  (tree exp, rtx target,
>> +			      const struct nios2_builtin_desc *d)
> 
> Excess space here.
> 
>> +      /* stxio  */
>> +      /* ldxio */
> 
> Comment style.
> 
>> +/* Implement TARGET_INIT_LIBFUNCS.  */
>> +static void
>> +nios2_init_libfuncs (void)
>> +{
>> +  /* For Linux, we have access to kernel support for atomic operations.  */
>> +  if (TARGET_LINUX_ABI)
>> +    init_sync_libfuncs (UNITS_PER_WORD);
>> +}
>> +
>> +
>> +
>> +
> 
> Newlines.

Done.

>> +	  /* Code conflicts between different __builtin_custom_xnxx calls
>> +	     do not seem to be checked. ???  */
> 
> Is that a problem?

It's just a note of the behavior, I've re-phrased it.

>> +/* Remember the last target of nios2_set_current_function.  */
>> +static GTY(()) tree nios2_previous_fndecl;
> 
> Ok, I don't know any of this stuff. It looks sufficiently similar to
> what i386 is doing that I guess it's ok.
> 
>> +
>> +/* Basic Characteristics of Registers:
>> +Register Number
> 
> Comment formatting.
> 
>> +#define SC_REGNO (12)
>> +#define STATIC_CHAIN_REGNUM SC_REGNO
> 
> There are a lot of these pairs, and it looks like unnecessary double
> indirection. Lose the former in all cases. No parens around constants.

I've reorganized a lot of this in nios2.h.

>> +  D00_REG,
>> +  D01_REG,
>> +  D02_REG,
> 
> See previous comment about these classes.
> 
>> +#define GP_REGNO_FIRST 0
>> +#define GP_REGNO_LAST RA_REGNO
>> +#define GP_NREGS (GP_REGNO_LAST - GP_REGNO_FIRST + 1)
>> +#define GP_REGNO_P(REGNO) ((unsigned) ((REGNO) - GP_REGNO_FIRST) < GP_NREGS)
> 
> Seems a little overly complex.

Simplified a bit.

>> +#define REGNO_REG_CLASS(REGNO) (GP_REGNO_P (REGNO) ? GP_REGS : ALL_REGS)
> 
> Well, if you do decide to keep the Dxx_REG classes... strictly speaking
> they should be returned, although it shouldn't really matter and this
> definition is probably better in practice.

>> +#define INDEX_REG_CLASS ALL_REGS
> 
> See previous comment about MAX_REGS_PER_ADDRESS.
> 
>> +#define SMALL_INT(X) ((X) >= -0x8000 && (X) < 0x8000)
>> +#define SMALL_INT_UNSIGNED(X) ((X) >= 0 && (X) < 0x10000)
>> +#define UPPER16_INT(X) (((X) & 0xffff) == 0)
>> +#define SHIFT_INT(X) ((X) >= 0 && (X) <= 31)
>> +#define RDWRCTL_INT(X) ((X) >= 0 && (X) <= 31)
>> +#define CUSTOM_INSN_OPCODE(X) ((X) >= 0 && (X) <= 255)
> 
> Minor, but there's not really a consistent naming convention apparent
> for these.
> 
>> +/* Stack Layout and Calling Conventions.  */
>> +
>> +/* The downward variants are used by the compiler,
>> +   the upward ones serve as documentation.  */
>> +#define STACK_GROWS_DOWNWARD
>> +#define FRAME_GROWS_UPWARD
>> +#define ARGS_GROW_UPWARD
> 
> Ugh, no. If you must, #define FRAME_GROWS_DOWNWARD 0, but do not define
> bogus macros.

Cleaned this off.

>> +/* Treat LOC as a byte offset from the stack pointer and round it up
>> +   to the next fully-aligned offset.  */
>> +#define STACK_ALIGN(LOC)                                                \
>> +  (((LOC) + ((PREFERRED_STACK_BOUNDARY / 8) - 1))			\
>> +   & ~((PREFERRED_STACK_BOUNDARY / 8) - 1))
> 
> Should move to nios2.c near where it is actually used.

Done.

>> +/* Calling convention definitions.  */
>> +typedef struct nios2_args
>> +{
>> +  int regs_used;
>> +} CUMULATIVE_ARGS;
> 
> Minor, but could be typedef int CUMULATIVE_ARGS if you want.
> 
>> +/* This is to initialize the above unused CUM data type.  */
>> +#define INIT_CUMULATIVE_ARGS(CUM, FNTYPE, LIBNAME, FNDECL, N_NAMED_ARGS) \
>> +  (nios2_init_cumulative_args (&CUM, FNTYPE, LIBNAME, FNDECL, N_NAMED_ARGS))
> 
> Could be done in-line since it only sets a single value.

I've changed INIT_CUMULATIVE_ARGS to in-line now.

>> +#define SLOW_BYTE_ACCESS 1
> 
> Is this intentional?

Another bit inherited from Altera code. As I noted earlier, we are not
really doing a lot of performance related tuning at this time.

>> +/* It is as good to call a constant function address as to call an address
>> +   kept in a register.
>> +   ??? Not true anymore really. Now that call cannot address full range
>> +   of memory callr may need to be used */
> 
> Hmm.

Sandra will be implementing indirect call trampolines in BFD, the
compiler will not really be involved in this. In any case, indirect
calls are indeed slightly slower. '???' part of comment deleted.

>> +@item T
>> +A @code{const} wrapped @code{UNSPEC} expression,
>> +representing a supported PIC or TLS relocation.
> 
> Might want to place this inside an @ifset INTERNALS, since there
> probably isn't a way for user code to make use of this.

Okay.

>> -The @code{target} attribute is not implemented in GCC versions earlier
>> -than 4.4 for the i386/x86_64 and 4.6 for the PowerPC back ends.  It is
>> -not currently implemented for other back ends.
> 
> I suppose the paragraph should be extended for nios (as it is in the
> pragma documentation) rather than lost.

It is now.

>> +or void if absent. The @code{n} represnts the first parameter
>> +The letters reprsent the following data types:
> 
> "represent", twice.

Fixed, thanks for catching.

Updated ChangeLog:

2013-11-16  Chung-Lin Tang  <cltang@codesourcery.com>
            Sandra Loosemore  <sandra@codesourcery.com>
            Based on patches from Altera Corporation

        * config.gcc (nios2-*-*): Add nios2 config targets.
        * configure.ac (TLS_SECTION_ASM_FLAG): Add nios2 case.
        ("$cpu_type"): Add nios2 as new cpu type.
        * configure: Regenerate.
        * config/nios2/nios2.c: New file.
        * config/nios2/nios2.h: New file.
        * config/nios2/nios2-opts.h: New file.
        * config/nios2/nios2-protos.h: New file.
        * config/nios2/elf.h: New file.
        * config/nios2/elf.opt: New file.
        * config/nios2/linux.h: New file.
        * config/nios2/nios2.opt: New file.
        * config/nios2/nios2.md: New file.
        * config/nios2/predicates.md: New file.
        * config/nios2/constraints.md: New file.
        * config/nios2/t-nios2: New file.
        * common/config/nios2/nios2-common.c: New file.
        * doc/invoke.texi (Nios II options): Document Nios II specific
        options.
        * doc/md.texi (Nios II family): Document Nios II specific
        constraints.
        * doc/extend.texi (Function Specific Option Pragmas): Document
        Nios II supported target pragma functionality.

[1/3] Re-submission of Altera Nios II port, gcc parts

Commit Message

Comments

Patch