[OpenRISC] [PATCH v3 3/3] or1k: gcc: initial support for openrisc

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi Stafford,

Some minor comments.  I didn't read the atomics much, but I did look at
everything else, and it looks fine :-)

On Sat, Oct 27, 2018 at 01:37:02PM +0900, Stafford Horne wrote:
> +        case ${target} in
> +        or1k*-*-linux*)
> +                tm_file="${tm_file} gnu-user.h linux.h glibc-stdint.h"
> +                tm_file="${tm_file} or1k/linux.h"
> +                ;;

Spaces instead of tabs.

> +  /* Number of bytes saved on the stack for outgoing/sub-fucntion args.  */

Typo ("function").

> +  /* The sum of sizes: locals vars, called saved regs, stack pointer
> +   * and an optional frame pointer.
> +   * Used in expand_prologue () and expand_epilogue().  */

We don't use leading stars in comments (just spaces), normally.

> +  /* Set address to volitile to ensure the store doesn't get optimized out.  */

"volatile"


> +/* Helper for defining INITIAL_ELIMINATION_OFFSET.
> +   We allow the following eliminiations:
> +     FP -> HARD_FP or SP
> +     AP -> HARD_FP or SP
> +
> +   HFP and AP are the same which is handled below.  */
> +
> +HOST_WIDE_INT
> +or1k_initial_elimination_offset (int from, int to)

You could calculate this as  some_offset (from) - some_offset (to)  with
some_offset a simple helper function.  That gives you all possible
eliminations :-)

(Each offset is very cheap to compute in your case, so that's not a problem).

> +static rtx
> +or1k_function_value (const_tree valtype,
> +		     const_tree fn_decl_or_type ATTRIBUTE_UNUSED,
> +		     bool outgoing ATTRIBUTE_UNUSED)

Since we use C++ now you can write this as
		     bool /*outgoing*/)
or such, for enhanced readability.

> +   split.  Symbols are lagitimized using split relocations.  */

"legitimized"

> +void
> +or1k_expand_move (machine_mode mode, rtx op0, rtx op1)
> +{
> +  if (MEM_P (op0))
> +    {
> +      if (!const0_operand(op1, mode))

Space before (.

> +#undef TARGET_RTX_COSTS
> +#define TARGET_RTX_COSTS or1k_rtx_costs

You may want TARGET_INSN_COST as well (it is easier to get (more) correct).

> +      if (hi != 0)
> +	{
> +	  rtx scratch2 = gen_rtx_REG (Pmode, RV_REGNUM);
> +	  emit_move_insn (scratch2, GEN_INT (hi));
> +	  emit_insn (gen_add2_insn (scratch, scratch2));
> +        }

Tab instead of spaces.

> +  /* Generate a tail call to the target function.  */
> +  if (! TREE_USED (function))

No space after !.

> +#define TARGET_RETURN_IN_MEMORY	or1k_return_in_memory

> +#define	TARGET_PASS_BY_REFERENCE or1k_pass_by_reference

These two have a tab instead of a space (as the rest do).

> +#define WCHAR_TYPE_SIZE	32

And this.

> +   This ABI has no adjacent call-saved register, which means that
> +   DImode/DFmode pseudos cannot be call-saved and will always be
> +   spilled across calls.  To solve this without changing the ABI,
> +   remap the compiler internal register numbers to place the even
> +   call-saved registers r16-r30 in 24-31, and the odd call-clobbered
> +   registers r17-r31 in 16-23.  */

Ooh evilness :-)

> +#define Pmode	SImode
> +#define FUNCTION_MODE	SImode

Some more tabs.

> +#define FUNCTION_ARG_REGNO_P(r) (r >= 3 && r <= 8)

IN_RANGE ?

> + { ARG_POINTER_REGNUM,	 STACK_POINTER_REGNUM },	\

Weird tab here, too.

> +#define EH_RETURN_REGNUM	HW_TO_GCC_REGNO (23)

And here.

> +(define_insn "xorsi3"
> +  [(set (match_operand:SI 0 "register_operand" "=r,r")
> +	  (xor:SI
> +	   (match_operand:SI 1 "register_operand"   "%r,r")
> +	   (match_operand:SI 2 "reg_or_s16_operand" " r,I")))]
> +  ""
> +  "@
> +  l.xor\t%0, %1, %2
> +  l.xori\t%0, %1, %2")

Is this correct?  Should this be unsigned (u16 and K)?
https://sourceware.org/cgen/gen-doc/openrisc-insn.html suggest so, but I
don't know how up-to-date or relevant that is.  Well.  From the atomics
much below it seems to be correct, and signed is nice for making a bit
inverse.  Is there some better documentation?  Something to put at
https://gcc.gnu.org/readings.html (this is in the CVS repo, still see
https://gcc.gnu.org/about.html#cvs ).

> +(define_expand "mov<I:mode>"
> +  [(set (match_operand:I 0 "nonimmediate_operand" "")
> +	(match_operand:I 1 "general_operand" ""))]

You can completely leave out empty constraint strings, for example in the
expanders.

> +mhard-div
> +Target RejectNegative InverseMask(SOFT_DIV)
> +Use hardware divide instructions, use -msoft-div for emulation.
> +
> +mhard-mul
> +Target RejectNegative InverseMask(SOFT_MUL).
> +Use hardware multiply instructions, use -msoft-mul for emulation.

Maybe put the -msoft-* options near here then?


This was a lovely read.  Thank you!  Your port looks great.


Segher