Patchwork Commit: MSP430: Pass -md on to assembler

login
register
mail settings
Submitter Mike Stump
Date Sept. 27, 2013, 6:21 p.m.
Message ID <4C64BA97-4A8A-42C7-9290-23906F3E807A@comcast.net>
Download mbox | patch
Permalink /patch/278664/
State New
Headers show

Comments

Mike Stump - Sept. 27, 2013, 6:21 p.m.
On Sep 27, 2013, at 1:48 AM, nick clifton <nickc@redhat.com> wrote:
> OK by me, although I cannot approve that particular patch.

I know, the intent is for someone that can, to approve it.

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


fixes this problem.  The problem is that we treat the maximum value of PSImode as -1, and then later we do:

          case EQ:
            /* x == y is always false for y out of range.  */
  =>        if (val < mmin || val > mmax)
B             return const0_rtx;
            break;

and the answer to the question is 0 > -1, is no, so the entire test is eliminated as never able to be true.  After the fix, in your case, we get:

(gdb) p mmin
$72 = -524288
(gdb) p mmax
$73 = 524287

and the test becomes if (0 < -524288 || 0 > 524287), which is not true, then the test isn't eliminated as never true.

Here, we see the test that protects this code uses GET_MODE_PRECISION:

(gdb) macro expand HWI_COMPUTABLE_MODE_P (PSImode)
expands to: ((((enum mode_class) mode_class[PSImode]) == MODE_INT || ((enum mode_class) mode_class[PSIm\
ode]) == MODE_PARTIAL_INT) && mode_precision[PSImode] <= (8 * 8))

so, clearly, someone was thinking about GET_MODE_PRECISION being in range, not GET_MODE_BITSIZE.

Ok?  [ for the rtl people ]
Mike Stump - Oct. 28, 2013, 8:20 p.m.
Ping?

On Sep 27, 2013, at 11:21 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Sep 27, 2013, at 1:48 AM, nick clifton <nickc@redhat.com> wrote:
>> OK by me, although I cannot approve that particular patch.
> 
> I know, the intent is for someone that can, to approve it.
> 
>> 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:
> 
> Index: stor-layout.c
> ===================================================================
> --- stor-layout.c	(revision 202634)
> +++ stor-layout.c	(working copy)
> @@ -2821,7 +2821,7 @@ get_mode_bounds (enum machine_mode mode,
> 		 enum machine_mode target_mode,
> 		 rtx *mmin, rtx *mmax)
> {
> -  unsigned size = GET_MODE_BITSIZE (mode);
> +  unsigned size = GET_MODE_PRECISION (mode);
>   unsigned HOST_WIDE_INT min_val, max_val;
> 
>   gcc_assert (size <= HOST_BITS_PER_WIDE_INT);
> 
> fixes this problem.  The problem is that we treat the maximum value of PSImode as -1, and then later we do:
> 
>          case EQ:
>            /* x == y is always false for y out of range.  */
>  =>        if (val < mmin || val > mmax)
> B             return const0_rtx;
>            break;
> 
> and the answer to the question is 0 > -1, is no, so the entire test is eliminated as never able to be true.  After the fix, in your case, we get:
> 
> (gdb) p mmin
> $72 = -524288
> (gdb) p mmax
> $73 = 524287
> 
> and the test becomes if (0 < -524288 || 0 > 524287), which is not true, then the test isn't eliminated as never true.
> 
> Here, we see the test that protects this code uses GET_MODE_PRECISION:
> 
> (gdb) macro expand HWI_COMPUTABLE_MODE_P (PSImode)
> expands to: ((((enum mode_class) mode_class[PSImode]) == MODE_INT || ((enum mode_class) mode_class[PSIm\
> ode]) == MODE_PARTIAL_INT) && mode_precision[PSImode] <= (8 * 8))
> 
> so, clearly, someone was thinking about GET_MODE_PRECISION being in range, not GET_MODE_BITSIZE.
> 
> Ok?  [ for the rtl people ]

Patch

Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 202634)
+++ stor-layout.c	(working copy)
@@ -2821,7 +2821,7 @@  get_mode_bounds (enum machine_mode mode,
 		 enum machine_mode target_mode,
 		 rtx *mmin, rtx *mmax)
 {
-  unsigned size = GET_MODE_BITSIZE (mode);
+  unsigned size = GET_MODE_PRECISION (mode);
   unsigned HOST_WIDE_INT min_val, max_val;
 
   gcc_assert (size <= HOST_BITS_PER_WIDE_INT);