Commit: MSP430: Pass -md on to assembler

On Sep 23, 2013, at 8:24 AM, nick clifton <nickc@redhat.com> wrote:
>> +(define_insn "extendpsisi2"
>> +  [(set (match_operand:SI 0 "nonimmediate_operand" "=r")
>> +	(sign_extend:SI (match_operand:PSI 1 "nonimmediate_operand" "0")))]
>> +  "TARGET_LARGE"
>> +  "# extend psi to si in %0"
>> +)
>> +
>>  ;; Look for cases where integer/pointer conversions are suboptimal due
> 
> Works like a charm.  The strange thing though is that the pattern never has to generate any code.  (I added a gcc_unreachable() to detect if the pattern was ever used to generate assembler).

> I must say though, it seems wrong to have to provide a sign-extend pointer pattern when pointers (on the MSP430) are unsigned.

Agreed.  If we instead ask, is it sane for gcc to ever want to signed extend in this case, the answer appears to be no.  Why does it, ptr_mode is SImode, and expand_builtin_next_arg is used to perform the addition in this mode.  It 'just' knows that is can be signed extended… and just does it that way.  This seems like it is wrong.


would fix this problem.  If this is done, the unmodified test case then doesn't abort.  Arguably, the extension should be done as the port directs.  It isn't clear to me why they do not.

Ok?

> If we instead ask, is it sane for gcc to ever want to signed extend
> in this case,

IIRC I've seen this due to the fact that pointer math is always
signed, and since gcc has no way of having a PSImode-sized size_t, all
pointer math is done in signed SImode, then the result is truncated to
PSImode.

Hi Mike,

>> I must say though, it seems wrong to have to provide a sign-extend pointer pattern when pointers (on the MSP430) are unsigned.
>
> Agreed.  If we instead ask, is it sane for gcc to ever want to signed extend in this case, the answer appears to be no.  Why does it, ptr_mode is SImode, and expand_builtin_next_arg is used to perform the addition in this mode.  It 'just' knows that is can be signed extended… and just does it that way.  This seems like it is wrong.
>
> Index: builtins.c
> ===================================================================
> --- builtins.c	(revision 202634)
> +++ builtins.c	(working copy)
> @@ -4094,7 +4094,7 @@ expand_builtin_next_arg (void)
>     return expand_binop (ptr_mode, add_optab,
>   		       crtl->args.internal_arg_pointer,
>   		       crtl->args.arg_offset_rtx,
> -		       NULL_RTX, 0, OPTAB_LIB_WIDEN);
> +		       NULL_RTX, POINTERS_EXTEND_UNSIGNED > 0, OPTAB_LIB_WIDEN);
>   }
>
>   /* Make it easier for the backends by protecting the valist argument
>
> would fix this problem.  If this is done, the unmodified test case then doesn't abort.  Arguably, the extension should be done as the port directs.  It isn't clear to me why they do not.
>
> Ok?
>

OK by me, although I cannot approve that particular patch.

I did eventually find some test cases that exercised the sign-extend 
pointer pattern, so I was able to check the generated code - it worked OK.

But I ran into a very strange problem.  With your PARTIAL_INT_MODE_NAME 
patch applied GCC started erroneously eliminating NULL function pointer 
checks!  This was particularly noticeable in libgcc/crtstuff.c where for 
example:

   static void __attribute__((used))
   frame_dummy (void)
   {
     static struct object object;
     if (__register_frame_info)
       __register_frame_info (__EH_FRAME_BEGIN__, &object);

(this is a simplified version of the real code) ... is compiled as if it 
had be written as:

   static void __attribute__((used))
   frame_dummy (void)
   {
     static struct object object;
     __register_frame_info (__EH_FRAME_BEGIN__, &object);


This only happens for the LARGE model (when pointers are PSImode) but I 
was baffled as to where it could be happening. Have you come across 
anything like this ?

Cheers
   Nick