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

[Bernd Schmidt <bernds at codesourcery dot com>]

On 11/16/2013 11:01 AM, Chung-Lin Tang wrote:
> 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.

I agree with all the comments Richard has been making, so I'll just add
a few other points.

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

If they serve no purpose (and I think they don't), they should go.

>>> >> +      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.

emit_move_insn is the right interface for this. It's the clearest way to
write what you want to do.

>>> >> +  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.

I don't understand this. Using strncmp matches "60-1" and any other
string beginning with that prefix, but doesn't distinguish between them.
If that really is the desired behaviour, it needs a comment, but it
still looks to me as if this is just lacking proper error checking.

>>> >> +  /* 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...

sorry is for unimplemented features, so I think it's the right function
to use.

>>> >> +/* 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...

It's just poor C style, and occurrences in other backends don't change
that. We do use parens like this if the expression spans multiple lines
to help the editor, but that's the only case where they should be used.

>>> >> +static rtx
>>> >> +nios2_call_tls_get_addr (rtx ti)
>> > 
>> > 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.

I was wondering whether such a call needs to be marked with a special
reloc for the linker to be able to eliminate it in some cases. ISTR some
targets do that, but if nios2 doesn't, then no need to change anything.

>>> >> +#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.

I'd prefer the new #if 0 blocks there to go away.


Bernd