Re: PATCH: x86_64-pc-mingw32 target for gcc

[Richard Henderson <rth at redhat dot com>]

On Wed, Mar 14, 2007 at 02:50:00PM +0100, Kai Tietz wrote:
> I deceided to pack even the the new pragma commands "push_macro" and 
> "pop_macro", because they are linked and the approval of a dependent patch 
> takes a bit long.

No, you need to keep these patches separate.  There's no way
I'll approve them all as a unit.

> +typedef struct sPragmaMacro GTY(())
> +{
> +	char *name;
> +	char *value;
> +	struct sPragmaMacro *prev;
> +} sPragmaMacro;
> +static GTY(()) struct sPragmaMacro *theMacroList = NULL;

This does not follow the gnu coding conventions for identifiers.

> +  if (token == CPP_CLOSE_PAREN)
> +    {
> +	  /* Silently ignore */
> +	  id = 0;
> +    }

Bad indentation.  Lots of it in here.  Also beware of braces
with one dependant statement.

> +	  else if (pragma_lex (&x) != CPP_EOF)
> +	    {
> +		  warning (OPT_Wpragmas, "junk at end of %<#pragma push_macro%>");
> +		  while(pragma_lex (&x) != CPP_EOF)
> +		    ;
> +	    }

Why the loop?  None of the other pragma handers use one.

> +		  val = cpp_macro_definition (dummy, cpp_lookup (dummy, (const unsigned char *)macroname, strlen (macroname)));

TREE_STRING_LENGTH instead of strlen on macroname.

> +			  char s[4096];
> +			  sprintf (s, "\n\n#undef %s\n", c->name);
> +			  if (c->value)
> +			    {
> +				  sprintf (s + strlen (s), "#define %s\n", c->value);
> +				  cpp_push_buffer (dummy,s, strlen (s), true);
> +				}
> +			  else
> +			  	  cpp_push_buffer(dummy, s, strlen(s), true);

Eh?  Can't you use cpp_define and cpp_undef, rather than
cpp_push_buffer nonsense?

> +#ifndef OUTGOING_REG_PARM_STACK_SPACE
> +#define OUTGOING_REG_PARM_STACK_SPACE 0
> +#endif

This belongs in defaults.h.

> +  if (!OUTGOING_REG_PARM_STACK_SPACE && reg_parm_stack_space > 0 && PUSH_ARGS)

Wrap long lines.

> -#ifndef OUTGOING_REG_PARM_STACK_SPACE
> -	      rtx push_size = GEN_INT (reg_parm_stack_space
> -				       + adjusted_args_size.constant);
> -#else
> -	      rtx push_size = GEN_INT (adjusted_args_size.constant);
> -#endif
> +	      rtx push_size = (!OUTGOING_REG_PARM_STACK_SPACE ? (GEN_INT (reg_parm_stack_space
> +				       + adjusted_args_size.constant)) : (GEN_INT (adjusted_args_size.constant)));

Long lines.  This could be better written as

	rtx push_size = GEN_INT (adjusted_args_size.constant
				 + (OUTGOING_REG_PARM_STACK_SPACE ? 0
				    : reg_parm_stack_space));

>  #define DBX_REGISTER_NUMBER(n) (write_symbols == DWARF2_DEBUG   \
> -                                ? svr4_dbx_register_map[n]      \
> -                                : dbx_register_map[n])
> +                                ? (TARGET_64BIT ? dbx64_register_map[n] : svr4_dbx_register_map[n]) \
> +                                : (TARGET_64BIT ? dbx64_register_map[n] : dbx_register_map[n]))

Long lines.

> -#define SIZE_TYPE "unsigned int"
> -#define PTRDIFF_TYPE "int"
> +#define SIZE_TYPE (TARGET_64BIT ? (TARGET_64BIT_MS_ABI ? "long long unsigned int" : "long unsigned int") : "unsigned int")

Why does mingw size_type have to depend on TARGET_64BIT_MS_ABI?
Why wouldn't you define it as "unsigned long long" all the time
for 64-bit?  This seems a point of unnecessary confusion.

Similarly PTRDIFF_TYPE.

> +	pushq  %rdx		/* save temp */

Surely you can find a call-clobbered register to use,
and don't need to save anything.  Perhaps r10?

> +	movq   %rsp,%rdx	/* get sp */
> +	addq   $0x10,%rdx	/* and point to return addr */

	leaq	0x10(%rsp), %rdx

> +	orq    $0x0,(%rdx)   		/* probe there */

Using orl would be smaller, and just as effective.

> -	ix86_cmodel = flag_pic ? CM_SMALL_PIC : CM_SMALL;
> +	ix86_cmodel = (TARGET_64BIT_MS_ABI ? (flag_pic ? CM_MEDIUM_PIC : CM_MEDIUM) : (flag_pic ? CM_SMALL_PIC : CM_SMALL));

Long lines, and I *really* don't like nested ?: when it can be avoided.

	if (TARGET_64BIT_MS_ABI)
	  {
	    ix86_cmodel = CM_SMALL_PIC;
	    flag_pic = 1;
	  }
	else
	  ix86_cmodel = flag_pic ? CM_SMALL_PIC : CM_SMALL;

And I think we should discuss the small vs medium thing more; from the
discussion, I think the above will probably work for you.

> -  if (TARGET_64BIT)
> +  if (TARGET_64BIT && !TARGET_64BIT_MS_ABI)
>      {
>        warning (OPT_Wattributes, "%qs attribute ignored",

I don't see any reference to stdcall and other such markers in the
Win64 documentation.  Are they supposed to be accepted and discarded
without comments?  Because I think quite a lot of your changes to 
make them be "handled" in some way is wrong.

>        /* Use register calling convention for local functions when possible.  */
> -      if (!TARGET_64BIT && !user_convention && decl
> +      if ((!TARGET_64BIT || TARGET_64BIT_MS_ABI) && !user_convention && decl

TARGET_64BIT_MS_ABI already uses a register calling convention.
I don't see the point of this change at all.

>    /* Lose any fake structure return argument if it is passed on the stack.  */
>    if (aggregate_value_p (TREE_TYPE (funtype), fundecl)
> -      && !TARGET_64BIT
> +      && (!TARGET_64BIT || TARGET_64BIT_MS_ABI)
>        && !KEEP_AGGREGATE_RETURN_POINTER)

Likewise.

> +  const int *x86_64_int_parameter_registers_ptr;

Please use a more sensible name for this, like "parameter_regs".
As is, one expects is to be a pointer to x86_64_int_parameter_registers,
which isn't (necessarily) true.

> @@ -3339,9 +3358,42 @@
>  
>  static rtx
>  gen_reg_or_parallel (enum machine_mode mode, enum machine_mode orig_mode,
> -		     unsigned int regno)
> +		     unsigned int regno,const int **intreg)

You've got the calling convention code completely wrong.

You shouldn't be reusing construct_container, or any of the other
x86_64 unix routines.  The ms conventions are *vastly* different,
and simpler.

> @@ -19630,7 +19780,10 @@
>        /* Put the this parameter into %eax.  */
>        xops[0] = this;
>        xops[1] = this_reg = gen_rtx_REG (Pmode, 0);
> +      if(Pmode==SImode)
>        output_asm_insn ("mov{l}\t{%0, %1|%1, %0}", xops);
> +      else
> +        output_asm_insn ("mov{q}\t{%0, %1|%1, %0}", xops);
>      }

This is surely wrong, since x86_this_parameter should
always return a register, and this path should never
be taken.

>    else
>      this_reg = NULL_RTX;
> @@ -19667,7 +19820,7 @@
>  	  if (lookup_attribute ("fastcall",
>  	      TYPE_ATTRIBUTES (TREE_TYPE (function))))
>  	    tmp_regno = 0 /* EAX */;
> -	  tmp = gen_rtx_REG (SImode, tmp_regno);
> +	  tmp = gen_rtx_REG ((TARGET_64BIT ? DImode : SImode), tmp_regno);

Likewise.

>  	}
>  
>        xops[0] = gen_rtx_MEM (Pmode, this_reg);
> @@ -19735,7 +19888,7 @@
>  	else
>  #endif /* TARGET_MACHO */
>  	{
> -	  tmp = gen_rtx_REG (SImode, 2 /* ECX */);
> +	  tmp = gen_rtx_REG ((TARGET_64BIT ? DImode : SImode), 2 /* ECX */);

Note that this code is already protected by TARGET_64BIT above.

> -#define LONG_TYPE_SIZE BITS_PER_WORD
> +#define LONG_TYPE_SIZE (TARGET_64BIT_MS_ABI ? 32 : BITS_PER_WORD)

This should be changed in cygming.h, not in i386.h.

>        gcc_assert (!flag_pic || LEGITIMATE_PIC_OPERAND_P (operands[1]));
> -      if (get_attr_mode (insn) == MODE_SI)
> +      if (get_attr_mode (insn) == MODE_SI && !TARGET_64BIT_MS_ABI)
>  	return "mov{l}\t{%k1, %k0|%k0, %k1}";

Definitely wrong.  The point here is to use "movl $N, %eax" to 
load the 32-bit constant N, reserving movabsq for true 64-bit 
constants.

>  #endif
> -    emit_insn (gen_allocate_stack_worker (copy_to_mode_reg (SImode,
> +    emit_insn (gen_allocate_stack_worker (copy_to_mode_reg (Pmode,
>  							    operands[1])));

Good, but not quite far enough.

> +(define_expand "allocate_stack64"

Bad.  This would never be used.

(define_expand "allocate_stack"
  [(match_operand 0 "register_operand" "")
   (match_operand 1 "general_operand" "")]
  "TARGET_STACK_PROBE"
{
#ifdef CHECK_STACK_LIMIT
  if (CONST_INT_P (operands[1])
      && INTVAL (operands[1]) < CHECK_STACK_LIMIT)
    {
      rtx x;
      x = expand_simple_binop (Pmode, MINUS, stack_pointer_rtx,
			       operands[1], stack_pointer_rtx,
			       1, OPTAB_DIRECT);
      gcc_assert (x == stack_pointer_rtx);
    }
  else
#endif
    emit_insn (gen_allocate_stack_worker (force_reg (Pmode, operands[1])));

  emit_move_insn (operands[0], virtual_stack_dynamic_rtx);
  DONE;
})

You also need to define STACK_SIZE_MODE as Pmode.  This can be done
in i386.h.

> +;; Support MS64 ABI
> +Mask(64BIT_MS_ABI)

There is no reason why this needs to be a bit in target_flags.
In fact, those are kinda rare, so we should *not* use one.

For the moment, I suggest simply using

#define TARGET_64BIT_MS_ABI  0

in i386.h, and

#undef TARGET_64BIT_MS_ABI
#define TARGET_64BIT_MS_ABI  TARGET_64BIT

in cygmingw.h.

> +	  if(TARGET_64BIT_MS_ABI) {					\
> +	  } else {							\

GNU coding standards.

> +#undef POINTER_SIZE
> +#define POINTER_SIZE (TARGET_64BIT ? 64 : 32)

This is identical to the default definition, and unneeded.

> +#ifdef _WIN64
> +  typedef int _sleb128_t __attribute__ ((mode (TI)));
> +  typedef unsigned int _uleb128_t __attribute__ ((mode (TI)));

This is incorrect.  You don't need a 128-bit type here.


r~