PATCH: x86_64-pc-mingw32 target for gcc

Kai Tietz Kai.Tietz@onevision.com
Wed Mar 21 10:35:00 GMT 2007


Richard Henderson <rth@redhat.com> wrote on 20.03.2007 20:23:50:

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

It is allready seperated.

> This does not follow the gnu coding conventions for identifiers.

In the updated patch this names are changed.

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

I corrected this in the updated patch allready, too.

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

> Eh?  Can't you use cpp_define and cpp_undef, rather than
> cpp_push_buffer nonsense?
I tried, but the do not worked as expected, there for I used the brute 
force variant. The fixed array size I replaced in the updated patch 
allready to use alloca for buffer size.

> > +#ifndef OUTGOING_REG_PARM_STACK_SPACE
> > +#define OUTGOING_REG_PARM_STACK_SPACE 0
> > +#endif
>
> This belongs in defaults.h.

This is true.
 
> > -#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.
Because the type is different to TARGET_64BIT. Linux uses integer type of 
long for 8 byte size, but for MS the long type remains a 4 byte integer. 
The type for x86 code uses the int type for pointer size representation. 
May you are compiling a cross where headers are shared ?
> Similarly PTRDIFF_TYPE.
Same as above.

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

Yes, the cygwin alloca implementation is just a first implementation and 
can be optimized for sure. The most important thing of this rough 
implementation is, that the incoming register as argument is rcx, not rax, 
as for x86. Feel free to optimize it.

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

The small model or even the medium PIC models, have currently a lack by 
using for a constant address push defined within near objects the wrong 
address memonics, because the code (at least for dll's) may needs to be 
rebased.
E.g: gcc produces currently direct address moves as "movl LC0, si", which 
isn't rebase save. There are two diffent solutions for that, first is 
using a large model for it - so it gets movq LC0, rsi -, or using rip 
based addressing - as "lea (LCO-.+rip), rsi".

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

True but as the fastcall issue is equivalent to standard calling 
convention it doesn't hurt. Sure in next update, the fastcall, have to be 
removed from support for this ABI.

> >        /* 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.
Same as above. I prevent to enter the linux part of calling conventions 
for this case. A simple short-cut.

> >    /* 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.
 
> >  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.
As I see from my native gcc, things with calling convention are doing even 
for floating points very well. ?

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

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

Yes, as far as I see, this path isn't taken at all. This change is 
irrelevant.

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

I agree.

> > -#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.
See above about pic code.

> >  #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.
Why ? It is defined by default in expr.h to word_mode, which fits fine.

> > +;; 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 {                     \
May this is a good solution for the problem, because the other idea is to 
make a code model out of this ABI.

> > +#undef POINTER_SIZE
> > +#define POINTER_SIZE (TARGET_64BIT ? 64 : 32)
No, it is defined by default to BITS_PER_WORD, which is in fact a word of 
type long and not a type of long long as needed.

> 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.
You are right, a long long (8-byte value) should be enough.

I got the suggestion to pack the new pragma commands within libcpp, which 
makes from my opionion sense too. What do you think about that ?

Thank you for your reviewing and your hard work.
(Sorry, this trailer I am recommented to use it)
Cheers,
 i.A. Kai Tietz

------------------------------------------------------------------------------------------
  OneVision Software Entwicklungs GmbH & Co. KG
  Dr.-Leo-Ritter-Straße 9 - 93049 Regensburg
  Tel: +49.(0)941.78004.0 - Fax: +49.(0)941.78004.489 - www.OneVision.com
  Commerzbank Regensburg - BLZ 750 400 62 - Konto 6011050
  Handelsregister: HRA 6744, Amtsgericht Regensburg
  Komplementärin: OneVision Software Entwicklungs Verwaltungs GmbH
  Dr.-Leo-Ritter-Straße 9 – 93049 Regensburg
  Handelsregister: HRB 8932, Amtsgericht Regensburg - Geschäftsführer: 
Ulrike Döhler, Manuela Kluger



More information about the Gcc-patches mailing list