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

[Chung-Lin Tang]

On 2013/12/28 02:29 PM, Chung-Lin Tang wrote:
 On 13/12/23 12:54 AM, Chung-Lin Tang wrote:
 Other than these two, I think this can go in.
 Bernd
 Attached is the updated patch for the compiler.

 Since Bernd is a Global Reviewer, am I clear for committing the port
 now? (including the testsuite and libgcc parts)
 
 I will be taking Bernd's prior mail as an approval. For avoidance of
 doubt, unless there are more comments raised, I will be committing the
 port to trunk next week.

The nios2 port was just committed. Thanks to all that gave time and
effort to review this.

Thanks,
Chung-Lin

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

[Chung-Lin Tang]

On 13/12/23 12:54 AM, Chung-Lin Tang wrote:
 Other than these two, I think this can go in.
  Bernd
 Attached is the updated patch for the compiler.
 
 Since Bernd is a Global Reviewer, am I clear for committing the port
 now? (including the testsuite and libgcc parts)

I will be taking Bernd's prior mail as an approval. For avoidance of
doubt, unless there are more comments raised, I will be committing the
port to trunk next week.

Thanks,
Chung-Lin

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

[Bernd Schmidt]

On 11/26/2013 07:45 AM, Chung-Lin Tang wrote:

 +(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))]

Didn't you say you'd removed the J alternative?

 +error (only register based stack limit is supported);

This one also ought to be a sorry.

Other than these two, I think this can go in.


Bernd

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

[Chung-Lin Tang]

Ping x3.

On 13/12/10 12:57 PM, Chung-Lin Tang wrote:
 Ping x2.
 
 On 2013/12/5 12:19 PM, Chung-Lin Tang wrote:
 Ping.

 On 2013/11/26 02:45 PM, Chung-Lin Tang wrote:
 Hi Bernd,
 I've updated the patch again, please see if it looks fit for approval
 now. Including ChangeLog again for completeness.

 Thanks,
 Chung-Lin

 2013-11-26  Chung-Lin Tang  clt...@codesourcery.com
 Sandra Loosemore  san...@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.


 

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

[Chung-Lin Tang]

Ping x2.

On 2013/12/5 12:19 PM, Chung-Lin Tang wrote:
 Ping.
 
 On 2013/11/26 02:45 PM, Chung-Lin Tang wrote:
 Hi Bernd,
 I've updated the patch again, please see if it looks fit for approval
 now. Including ChangeLog again for completeness.

 Thanks,
 Chung-Lin

 2013-11-26  Chung-Lin Tang  clt...@codesourcery.com
 Sandra Loosemore  san...@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.

 

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

[Chung-Lin Tang]

Ping.

On 2013/11/26 02:45 PM, Chung-Lin Tang wrote:
 Hi Bernd,
 I've updated the patch again, please see if it looks fit for approval
 now. Including ChangeLog again for completeness.
 
 Thanks,
 Chung-Lin
 
 2013-11-26  Chung-Lin Tang  clt...@codesourcery.com
 Sandra Loosemore  san...@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.
 

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

[Bernd Schmidt]

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

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

[Chung-Lin Tang]

On 13/11/22 10:31 PM, Bernd Schmidt wrote:
 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.

I'll check again, but I remember df_regs_ever_live_p doesn't include the
RA reg if the call pattern doesn't have the clobber.

I see c6x doesn't have that clobber either, but checks crtl-is_leaf
instead in the frame layout. Looking across the backends, adding a
clobber appears to be the more usual style.

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

This has to work on target attribute strings, which it may be in the
middle of the string. But your right that the single option form may
allow the extended suffix, I'll fix this next.

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

Argh, forgot to delete that. Thanks for catching.

I'll address the rest in the next revision.

Thanks,
Chung-Lin

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

[Chung-Lin Tang]

On 2013/11/21 03:25 PM, Richard Henderson wrote:
 On 11/21/2013 02:41 PM, Chung-Lin Tang wrote:
 I'm not saying we tolerate wrong RTL form, but rather that, it should
 be an issue of the RTL passes, not the backend. The backend should just
 be as much as possible, a  machine description.
 
 Matching non-canonical rtl does nothing but slow down the compiler and cause
 confusion for the poor person that has to maintain the code later.

As I mentioned in the last mail, I will modify the constraints as
suggested for the *norsi3 and EQ/NE cmp patterns in the final patch.

Chung-Lin

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

[Chung-Lin Tang]

On 13/11/20 1:34 AM, Richard Sandiford wrote:
 Chung-Lin Tang clt...@codesourcery.com writes:
 +;;  Integer logical Operations
 +
 +(define_code_iterator LOGICAL [and ior xor])
 +(define_code_attr logical_asm [(and and) (ior or) (xor xor)])
 +
 +(define_insn codesi3
 +  [(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_asmh%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.

Will any RTL-level passes, say for example, propagate a zero-constant
into the pattern, without also simplifying it to a NOT pattern? (that's
a question, I don't really know)

Personally, I haven't really seen many cases optimizable to NOR, but I
think keeping the 'M' there is pretty harmless.

 Hmm, but if we get (not (const_int 0)) then that sounds like a bug,
 since (and (not X) (not (const_int 0))) should reduce to (not X).
 IMO target-independent code shouldn't try to create the nor-with-0
 form and backends shouldn't match it.
 
 Why would removing 'M' affect the printing mechanism?  Naively I'd
 have expected:

The *norsi3 here is of course straightforward. I was referring to other
cases, like the AND/IOR/XOR pattern above, where I wanted to combine
them into a single alternative. That needs a bit more work to reorganize
the nios2_print_operand() cases.

 (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 register_operand  r]
   
   nor\\t%0, %1, %2
   [(set_attr type alu)])
 
 to just work.

That will definitely work, though I don't think the zero case does any harm.

 +;; Integer comparisons
 +
 +(define_code_iterator EQNE [eq ne])
 +(define_insn nios2_cmpcode
 +  [(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)))]
 +  
 +  cmpcode%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.
 
 But rtl-wise, we should never generate an EQ or NE with two constants.
 And if one operand is constant then it's supposed to be the second.
 
 The % should give commutativity on its own, without the M.

For EQ/NE I guess that's the case, for the other comparisons I'm not so
sure; I'm not familiar enough with the details of the expander machinery
to claim anything.

If this doesn't carry to other comparisons, I intend to keep it in line
with the other cmp patterns. I experimented a bit with the generated
code, nothing is affected.

 +  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.
 
 There shouldn't be anything to be afraid of though.  Target-independent
 code would use emit_move_insn for this though, so it needs to just work.

It will work, and I did use it in some places, though I did not
exhaustively search-and-replace.

For (subtle) performance reasons, emit_move_insn() really does too much
as a backend utility. Usually backend code is already very precise on
what we want to generate.

 +  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.
 
 FWIW, I think it was more that comments should be above the field rather
 than tagged on the right.  (One of the big problems with right-column comments
 is that people tend to make them too short.)

I will change to use the 

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

[Richard Sandiford]

Chung-Lin Tang clt...@codesourcery.com writes:
 On 13/11/20 1:34 AM, Richard Sandiford wrote:
 Chung-Lin Tang clt...@codesourcery.com writes:
 +;;  Integer logical Operations
 +
 +(define_code_iterator LOGICAL [and ior xor])
 +(define_code_attr logical_asm [(and and) (ior or) (xor xor)])
 +
 +(define_insn codesi3
 +  [(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_asmh%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.

 Will any RTL-level passes, say for example, propagate a zero-constant
 into the pattern, without also simplifying it to a NOT pattern? (that's
 a question, I don't really know)

No, they shouldn't, because only the simplified form is expected to match.
E.g. forwprop goes to some lengths to do this properly.

 Personally, I haven't really seen many cases optimizable to NOR, but I
 think keeping the 'M' there is pretty harmless.

That's not the way to look at it though.  There should be no
unsimplified rtl in the instruction stream, just like there should
be no non-canonical rtl.  We don't have rules for canonical rtl
because the other forms are harmful (in the sense of generating wrong
code or whatever).  We have them so that there aren't too many possible
representations of the same thing, and so that code dealing with existing
rtl patterns can validly expect one shape over another.  If we see
something that's generating the wrong rtl form, we should fix it rather
than pander to it.

IMO having (and (not (const_int 0)) (not X)) as an alternative
representation of (not X) falls directly into that category.

 +;; Integer comparisons
 +
 +(define_code_iterator EQNE [eq ne])
 +(define_insn nios2_cmpcode
 +  [(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)))]
 +  
 +  cmpcode%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.
 
 But rtl-wise, we should never generate an EQ or NE with two constants.
 And if one operand is constant then it's supposed to be the second.
 
 The % should give commutativity on its own, without the M.

 For EQ/NE I guess that's the case, for the other comparisons I'm not so
 sure; I'm not familiar enough with the details of the expander machinery
 to claim anything.

Sure, but the point is that EQ and NE _are_ commutative, and rtl.texi
says that:

  For commutative binary operations, constants should be placed in the
  second operand.

So...

 If this doesn't carry to other comparisons, I intend to keep it in line
 with the other cmp patterns. I experimented a bit with the generated
 code, nothing is affected.

...no, it doesn't carry over to other comparisons, but it's not supposed to.
Just like you can't add % to the other comparisons.

The patterns are including constraints whose only purpose is to match
non-canonical rtl.  That shouldn't happen.

 +  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.
 
 There shouldn't be anything to be afraid of though.  Target-independent
 code would use emit_move_insn for this though, so it needs to just work.

 It will work, and I did use it in some places, though I did not
 exhaustively search-and-replace.

 For (subtle) performance reasons, emit_move_insn() really does too much
 as a backend utility. Usually backend code is already very precise on
 what we want to generate.

Hmm, this sounds a bit FUDish.  What subtle reasons do you mean?

If moving a constant into a register is valid as a simple SET
then that's what 

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

[Chung-Lin Tang]

On 13/11/21 7:21 AM, Richard Sandiford wrote:
 Chung-Lin Tang clt...@codesourcery.com writes:
 On 13/11/20 1:34 AM, Richard Sandiford wrote:
 Chung-Lin Tang clt...@codesourcery.com writes:
 +;;  Integer logical Operations
 +
 +(define_code_iterator LOGICAL [and ior xor])
 +(define_code_attr logical_asm [(and and) (ior or) (xor xor)])
 +
 +(define_insn codesi3
 +  [(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_asmh%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.

 Will any RTL-level passes, say for example, propagate a zero-constant
 into the pattern, without also simplifying it to a NOT pattern? (that's
 a question, I don't really know)
 
 No, they shouldn't, because only the simplified form is expected to match.
 E.g. forwprop goes to some lengths to do this properly.
 
 Personally, I haven't really seen many cases optimizable to NOR, but I
 think keeping the 'M' there is pretty harmless.
 
 That's not the way to look at it though.  There should be no
 unsimplified rtl in the instruction stream, just like there should
 be no non-canonical rtl.  We don't have rules for canonical rtl
 because the other forms are harmful (in the sense of generating wrong
 code or whatever).  We have them so that there aren't too many possible
 representations of the same thing, and so that code dealing with existing
 rtl patterns can validly expect one shape over another.  If we see
 something that's generating the wrong rtl form, we should fix it rather
 than pander to it.
 IMO having (and (not (const_int 0)) (not X)) as an alternative
 representation of (not X) falls directly into that category.

I'm not saying we tolerate wrong RTL form, but rather that, it should
be an issue of the RTL passes, not the backend. The backend should just
be as much as possible, a  machine description.

Saying you don't need to describe a particular thing in the backend
because GCC doesn't need that sounds like a spill-over of internal
details. In theory, a backend writer shouldn't need to care for such things.

 +;; Integer comparisons
 +
 +(define_code_iterator EQNE [eq ne])
 +(define_insn nios2_cmpcode
 +  [(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)))]
 +  
 +  cmpcode%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.

 But rtl-wise, we should never generate an EQ or NE with two constants.
 And if one operand is constant then it's supposed to be the second.

 The % should give commutativity on its own, without the M.

 For EQ/NE I guess that's the case, for the other comparisons I'm not so
 sure; I'm not familiar enough with the details of the expander machinery
 to claim anything.
 
 Sure, but the point is that EQ and NE _are_ commutative, and rtl.texi
 says that:
 
   For commutative binary operations, constants should be placed in the
   second operand.
 
 So...
 
 If this doesn't carry to other comparisons, I intend to keep it in line
 with the other cmp patterns. I experimented a bit with the generated
 code, nothing is affected.
 
 ...no, it doesn't carry over to other comparisons, but it's not supposed to.
 Just like you can't add % to the other comparisons.
 
 The patterns are including constraints whose only purpose is to match
 non-canonical rtl.  That shouldn't happen.

My argument is the same as above.


Having that said, I'll edit the *norsi3 and EQNE patterns to remove the
'M' like you suggested in the final patch. This is too trivial an issue
to get postponed by.

 +  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 

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

[Richard Henderson]

On 11/21/2013 02:41 PM, Chung-Lin Tang wrote:
 I'm not saying we tolerate wrong RTL form, but rather that, it should
 be an issue of the RTL passes, not the backend. The backend should just
 be as much as possible, a  machine description.

Matching non-canonical rtl does nothing but slow down the compiler and cause
confusion for the poor person that has to maintain the code later.

I'm with Richard S on this one.


r~

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

[Joseph S. Myers]

On Tue, 19 Nov 2013, Chung-Lin Tang wrote:

 Okay, then. I've updated the GCC backend patch to put the
 TARGET_INITIALIZER stuff at the end.  I've shuffled some of the routines
 to reduce the starting forward declarations, though some do remain.
 
 The ChangeLog is the same, so I haven't duplicated that here. Please see
 if this is now suitable for committing.

I have no comments on this version, though I'll leave review to others.

-- 
Joseph S. Myers
jos...@codesourcery.com

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

[Richard Sandiford]

Chung-Lin Tang clt...@codesourcery.com writes:
 +;;  Integer logical Operations
 +
 +(define_code_iterator LOGICAL [and ior xor])
 +(define_code_attr logical_asm [(and and) (ior or) (xor xor)])
 +
 +(define_insn codesi3
 +  [(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_asmh%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.

Hmm, but if we get (not (const_int 0)) then that sounds like a bug,
since (and (not X) (not (const_int 0))) should reduce to (not X).
IMO target-independent code shouldn't try to create the nor-with-0
form and backends shouldn't match it.

Why would removing 'M' affect the printing mechanism?  Naively I'd
have expected:

(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 register_operand  r]
  
  nor\\t%0, %1, %2
  [(set_attr type alu)])

to just work.

 +;; Integer comparisons
 +
 +(define_code_iterator EQNE [eq ne])
 +(define_insn nios2_cmpcode
 +  [(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)))]
 +  
 +  cmpcode%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.

But rtl-wise, we should never generate an EQ or NE with two constants.
And if one operand is constant then it's supposed to be the second.

The % should give commutativity on its own, without the M.

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

There shouldn't be anything to be afraid of though.  Target-independent
code would use emit_move_insn for this though, so it needs to just work.

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

FWIW, I think it was more that comments should be above the field rather
than tagged on the right.  (One of the big problems with right-column comments
is that people tend to make them too short.)

Thanks,
Richard

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

[Joseph S. Myers]

On Sat, 16 Nov 2013, Chung-Lin Tang wrote:

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

I agree with Bernd's preference of topologically sorting static functions 
/ variables so forward declarations are only needed in cases of recursion.

I sometimes think it should be possible to convert many target macros to 
hooks, including generating function definitions from the macro 
definitions in .h files, with a lot more automation than I think has been 
used for that before.  Some back ends using a style rather requires 
forward function declarations is a needless complication for that sort of 
thing (indeed, if anyone were working on automated target macro to hook 
conversion, I'd suggest an early automated change should be making all 
back ends define targetm at the end of the file and avoid forward static 
declarations where possible).

-- 
Joseph S. Myers
jos...@codesourcery.com

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

[Bernd Schmidt]

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.

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

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

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

 +;; 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?

 +  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)?

 +;; 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_operandrI)))]
 +  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.

 +(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...)?

 +;;  Integer logical Operations
 +
 +(define_code_iterator LOGICAL [and ior xor])
 +(define_code_attr logical_asm [(and and) (ior or) (xor xor)])
 +
 +(define_insn codesi3
 +  [(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)))]
 +  
 +  @
 +

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

[Nathan Sidwell]

On 09/02/13 13:23, Sebastian Huber wrote:


what is the blocking point for GCC integration?  It was accepted by the SC and
all issues of the last review have been addressed (at least this is my
impression).  Is it that none of the persons with global write permission seems
to be responsible?


That is correct.  No GWM has blessed the port from a technical POV.  The SC 
acceptance is for the subsequent maintainership.  AFAICT, Chung-Lin has 
addressed the initial review comments and been diligently pinging this latest 
patch set for a couple of months.


nathan

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

[Chung-Lin Tang]

Ping.

On 13/8/20 10:57 AM, Chung-Lin Tang wrote:
 Ping.
 
 BTW, the SC has approved the Nios II port already:
 http://gcc.gnu.org/ml/gcc/2013-07/msg00434.html
 
 The port is still awaiting technical review.
 
 Thanks,
 Chung-Lin
 
 On 13/7/14 下午3:54, 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.

 The changes accumulated since the original post include:

 1) Several bug fixes related to built-in function expanding.
 2) A few holes in hard-float FPU code generation was plugged.
 3) Support for parsing white-spaces in target attributes.
 4) Revision of consistency check behavior of codes in custom instruction
 built-ins.
 5) Some new testcases.

 The issues raised by Joseph in the first round of reviewing have been
 addressed. Testing has been re-done on both 32-bit and 64-bit hosts.

 PR55035 appears to not have been resolved yet, which affects nios2 among
 several other targets, thus configured with --enable-werror-always still
 does not build.

 As before, Sandra and me will serve as nios2 port maintainers.

 Attached is the patch for the compiler-proper.

 Thanks,
 Chung-Lin

 2013-07-14  Chung-Lin Tang  clt...@codesourcery.com
 Sandra Loosemore  san...@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.

 

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

[Sebastian Huber]

Hello,

what is the blocking point for GCC integration?  It was accepted by the SC and 
all issues of the last review have been addressed (at least this is my 
impression).  Is it that none of the persons with global write permission seems 
to be responsible?  The Binutils port is available since 2013-02-06.  The 
Newlib port is available since 2013-05-06.  The last step of an official 
FSF/Newlib tool chain for the Altera Nios II is the GCC part.


On 2013-09-02 11:38, Chung-Lin Tang wrote:

Ping.

On 13/8/20 10:57 AM, Chung-Lin Tang wrote:

Ping.

BTW, the SC has approved the Nios II port already:
http://gcc.gnu.org/ml/gcc/2013-07/msg00434.html

The port is still awaiting technical review.

Thanks,
Chung-Lin

On 13/7/14 下午3:54, 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.

The changes accumulated since the original post include:

1) Several bug fixes related to built-in function expanding.
2) A few holes in hard-float FPU code generation was plugged.
3) Support for parsing white-spaces in target attributes.
4) Revision of consistency check behavior of codes in custom instruction
built-ins.
5) Some new testcases.

The issues raised by Joseph in the first round of reviewing have been
addressed. Testing has been re-done on both 32-bit and 64-bit hosts.

PR55035 appears to not have been resolved yet, which affects nios2 among
several other targets, thus configured with --enable-werror-always still
does not build.

As before, Sandra and me will serve as nios2 port maintainers.

Attached is the patch for the compiler-proper.

Thanks,
Chung-Lin



--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail  : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

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

[Chung-Lin Tang]

Ping.

BTW, the SC has approved the Nios II port already:
http://gcc.gnu.org/ml/gcc/2013-07/msg00434.html

The port is still awaiting technical review.

Thanks,
Chung-Lin

On 13/7/14 下午3:54, 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.
 
 The changes accumulated since the original post include:
 
 1) Several bug fixes related to built-in function expanding.
 2) A few holes in hard-float FPU code generation was plugged.
 3) Support for parsing white-spaces in target attributes.
 4) Revision of consistency check behavior of codes in custom instruction
 built-ins.
 5) Some new testcases.
 
 The issues raised by Joseph in the first round of reviewing have been
 addressed. Testing has been re-done on both 32-bit and 64-bit hosts.
 
 PR55035 appears to not have been resolved yet, which affects nios2 among
 several other targets, thus configured with --enable-werror-always still
 does not build.
 
 As before, Sandra and me will serve as nios2 port maintainers.
 
 Attached is the patch for the compiler-proper.
 
 Thanks,
 Chung-Lin
 
 2013-07-14  Chung-Lin Tang  clt...@codesourcery.com
 Sandra Loosemore  san...@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.