[patch] Fix failure of ACATS c45503c at -O2

[patch] Fix failure of ACATS c45503c at -O2

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Hi,

this test started to fail recently as the result of the work of Richard S., 
but the underlying issue had been latent for a long time.  It boils down to 
this excerpt from the VRP1 dump file:

Found new range for _9: [0, 12]
marking stmt to be not simulated again

Visiting statement:
_3 = _9 %[fl] _11;
Found new range for _3: [0, +INF]

which is plain wrong since the sign of FLOOR_MOD_EXPR is the sign of the 
divisor and not that of the dividend (the latter is for TRUNC_MOD_EXPR).

I have attached a fix for mainline and another one for the 4.9/5 branches.

Tested on x86_64-suse-linux, OK for all active branches?


2015-10-20  Eric Botcazou  <ebotcazou@adacore.com>

	* fold-const.c (tree_binary_nonnegative_warnv_p) <FLOOR_MOD_EXPR>:
	Recurse on operand #1 instead of operand #0.
	<CEIL_MOD_EXPR>: Do not recurse.
	<ROUND_MOD_EXPR>: Likewise.

-- 
Eric Botcazou

[-- Attachment #2: p1.diff --]
[-- Type: text/x-patch, Size: 722 bytes --]

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 229022)
+++ fold-const.c	(working copy)
@@ -12982,11 +12982,15 @@ tree_binary_nonnegative_warnv_p (enum tr
       return RECURSE (op0) && RECURSE (op1);
 
     case TRUNC_MOD_EXPR:
-    case CEIL_MOD_EXPR:
-    case FLOOR_MOD_EXPR:
-    case ROUND_MOD_EXPR:
+      /* The sign of the remainder is that of the dividend.  */
       return RECURSE (op0);
 
+    case FLOOR_MOD_EXPR:
+      /* The sign of the remainder is that of the divisor.  */
+      return RECURSE (op1);
+
+    case CEIL_MOD_EXPR:
+    case ROUND_MOD_EXPR:
     default:
       return tree_simple_nonnegative_warnv_p (code, type);
     }

[-- Attachment #3: p2.diff --]
[-- Type: text/x-patch, Size: 806 bytes --]

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 228932)
+++ fold-const.c	(working copy)
@@ -14853,11 +14853,17 @@ tree_binary_nonnegative_warnv_p (enum tr
 						strict_overflow_p));
 
     case TRUNC_MOD_EXPR:
-    case CEIL_MOD_EXPR:
-    case FLOOR_MOD_EXPR:
-    case ROUND_MOD_EXPR:
+      /* The sign of the remainder is that of the dividend.  */
       return tree_expr_nonnegative_warnv_p (op0,
 					    strict_overflow_p);
+
+    case FLOOR_MOD_EXPR:
+      /* The sign of the remainder is that of the divisor.  */
+      return tree_expr_nonnegative_warnv_p (op1,
+					    strict_overflow_p);
+
+    case CEIL_MOD_EXPR:
+    case ROUND_MOD_EXPR:
     default:
       return tree_simple_nonnegative_warnv_p (code, type);
     }

Re: [patch] Fix failure of ACATS c45503c at -O2

[Jeff Law]

On 10/20/2015 10:33 AM, Eric Botcazou wrote:
> Hi,
>
> this test started to fail recently as the result of the work of Richard S.,
> but the underlying issue had been latent for a long time.  It boils down to
> this excerpt from the VRP1 dump file:
>
> Found new range for _9: [0, 12]
> marking stmt to be not simulated again
>
> Visiting statement:
> _3 = _9 %[fl] _11;
> Found new range for _3: [0, +INF]
>
> which is plain wrong since the sign of FLOOR_MOD_EXPR is the sign of the
> divisor and not that of the dividend (the latter is for TRUNC_MOD_EXPR).
>
> I have attached a fix for mainline and another one for the 4.9/5 branches.
>
> Tested on x86_64-suse-linux, OK for all active branches?
>
>
> 2015-10-20  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* fold-const.c (tree_binary_nonnegative_warnv_p) <FLOOR_MOD_EXPR>:
> 	Recurse on operand #1 instead of operand #0.
> 	<CEIL_MOD_EXPR>: Do not recurse.
> 	<ROUND_MOD_EXPR>: Likewise.
Isn't this a function of the language and in some cases isn't it 
implementation defined (true for C/C++ until C++11)?

Even with that in mind, I think FLOOR_MOD_EXPR and TRUNC_MOD_EXPR are 
more correct with your patch.  I'm just not sure about CEIL & ROUND.

Jeff

Re: [patch] Fix failure of ACATS c45503c at -O2

[Joseph Myers]

On Tue, 20 Oct 2015, Jeff Law wrote:

> > 2015-10-20  Eric Botcazou  <ebotcazou@adacore.com>
> > 
> > 	* fold-const.c (tree_binary_nonnegative_warnv_p) <FLOOR_MOD_EXPR>:
> > 	Recurse on operand #1 instead of operand #0.
> > 	<CEIL_MOD_EXPR>: Do not recurse.
> > 	<ROUND_MOD_EXPR>: Likewise.
> Isn't this a function of the language and in some cases isn't it
> implementation defined (true for C/C++ until C++11)?

The language determines *which* *_DIV_EXPR or *_MOD_EXPR is used.  The 
semantics of each of them should be language-independent.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Fix failure of ACATS c45503c at -O2

[Jeff Law]

On 10/20/2015 04:00 PM, Joseph Myers wrote:
> On Tue, 20 Oct 2015, Jeff Law wrote:
>
>>> 2015-10-20  Eric Botcazou  <ebotcazou@adacore.com>
>>>
>>> 	* fold-const.c (tree_binary_nonnegative_warnv_p) <FLOOR_MOD_EXPR>:
>>> 	Recurse on operand #1 instead of operand #0.
>>> 	<CEIL_MOD_EXPR>: Do not recurse.
>>> 	<ROUND_MOD_EXPR>: Likewise.
>> Isn't this a function of the language and in some cases isn't it
>> implementation defined (true for C/C++ until C++11)?
>
> The language determines *which* *_DIV_EXPR or *_MOD_EXPR is used.  The
> semantics of each of them should be language-independent.
You're, of course, correct.

So the only question is whether or not the CEIL_MOD_EXPR and 
ROUND_MOD_EXPR bits are right.  I'm confident the change to 
FLOOR_MOD_EXPR is right.

Do we have any reasonable way to test CEIL_MOD_EXPR & ROUND_MOD_EXPR?

Jeff

Re: [patch] Fix failure of ACATS c45503c at -O2

[Eric Botcazou]

> So the only question is whether or not the CEIL_MOD_EXPR and
> ROUND_MOD_EXPR bits are right.  I'm confident the change to
> FLOOR_MOD_EXPR is right.

OK.

> Do we have any reasonable way to test CEIL_MOD_EXPR & ROUND_MOD_EXPR?

Note that the patch makes the function punt on those 2 so it can do no harm.
The sign of CEIL_MOD_EXPR is predictable (opposite of that of the divisor) but 
you cannot use that in the context; and finally the sign of ROUND_MOD_EXPR 
isn't predictable.

I can add a few more comments:

Index: fold-const.c
===================================================================
--- fold-const.c        (revision 229022)
+++ fold-const.c        (working copy)
@@ -12982,11 +12982,18 @@ tree_binary_nonnegative_warnv_p (enum tr
       return RECURSE (op0) && RECURSE (op1);
 
     case TRUNC_MOD_EXPR:
-    case CEIL_MOD_EXPR:
-    case FLOOR_MOD_EXPR:
-    case ROUND_MOD_EXPR:
+      /* The sign of the remainder is that of the dividend.  */
       return RECURSE (op0);
 
+    case FLOOR_MOD_EXPR:
+      /* The sign of the remainder is that of the divisor.  */
+      return RECURSE (op1);
+
+    case CEIL_MOD_EXPR:
+       /* The sign of the remainder is the opposite of that of the divisor,
+         but this cannot be used in this context.  */
+    case ROUND_MOD_EXPR:
+      /* The sign of the remainder is not predictable.  */
     default:
       return tree_simple_nonnegative_warnv_p (code, type);
     }


-- 
Eric Botcazou

Re: [patch] Fix failure of ACATS c45503c at -O2

[Richard Biener]

On Wed, Oct 21, 2015 at 9:24 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> So the only question is whether or not the CEIL_MOD_EXPR and
>> ROUND_MOD_EXPR bits are right.  I'm confident the change to
>> FLOOR_MOD_EXPR is right.
>
> OK.
>
>> Do we have any reasonable way to test CEIL_MOD_EXPR & ROUND_MOD_EXPR?
>
> Note that the patch makes the function punt on those 2 so it can do no harm.
> The sign of CEIL_MOD_EXPR is predictable (opposite of that of the divisor) but
> you cannot use that in the context; and finally the sign of ROUND_MOD_EXPR
> isn't predictable.
>
> I can add a few more comments:

Maybe add the comments to tree.def instead.

> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 229022)
> +++ fold-const.c        (working copy)
> @@ -12982,11 +12982,18 @@ tree_binary_nonnegative_warnv_p (enum tr
>        return RECURSE (op0) && RECURSE (op1);
>
>      case TRUNC_MOD_EXPR:
> -    case CEIL_MOD_EXPR:
> -    case FLOOR_MOD_EXPR:
> -    case ROUND_MOD_EXPR:
> +      /* The sign of the remainder is that of the dividend.  */
>        return RECURSE (op0);
>
> +    case FLOOR_MOD_EXPR:
> +      /* The sign of the remainder is that of the divisor.  */
> +      return RECURSE (op1);
> +
> +    case CEIL_MOD_EXPR:
> +       /* The sign of the remainder is the opposite of that of the divisor,
> +         but this cannot be used in this context.  */
> +    case ROUND_MOD_EXPR:
> +      /* The sign of the remainder is not predictable.  */
>      default:
>        return tree_simple_nonnegative_warnv_p (code, type);
>      }
>
>
> --
> Eric Botcazou

Re: [patch] Fix failure of ACATS c45503c at -O2

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

> Maybe add the comments to tree.def instead.

Good idea, revised patch attached.


	* tree.def (CEIL_DIV_EXPR, FLOOR_DIV_EXPR, ROUND_DIV_EXPR): Tweak
	comments.
	(TRUNC_MOD_EXPR, CEIL_MOD_EXPR, FLOOR_MOD_EXPR, ROUND_MOD_EXPR):
	Add comments on sign of the result.
        * fold-const.c (tree_binary_nonnegative_warnv_p) <FLOOR_MOD_EXPR>:
        Recurse on operand #1 instead of operand #0.
        <CEIL_MOD_EXPR>: Do not recurse.
        <ROUND_MOD_EXPR>: Likewise.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 2259 bytes --]

Index: tree.def
===================================================================
--- tree.def	(revision 229123)
+++ tree.def	(working copy)
@@ -685,19 +685,27 @@ DEFTREECODE (MULT_HIGHPART_EXPR, "mult_h
 /* Division for integer result that rounds the quotient toward zero.  */
 DEFTREECODE (TRUNC_DIV_EXPR, "trunc_div_expr", tcc_binary, 2)
 
-/* Division for integer result that rounds the quotient toward infinity.  */
+/* Division for integer result that rounds it toward plus infinity.  */
 DEFTREECODE (CEIL_DIV_EXPR, "ceil_div_expr", tcc_binary, 2)
 
-/* Division for integer result that rounds toward minus infinity.  */
+/* Division for integer result that rounds it toward minus infinity.  */
 DEFTREECODE (FLOOR_DIV_EXPR, "floor_div_expr", tcc_binary, 2)
 
-/* Division for integer result that rounds toward nearest integer.  */
+/* Division for integer result that rounds it toward nearest integer.  */
 DEFTREECODE (ROUND_DIV_EXPR, "round_div_expr", tcc_binary, 2)
 
-/* Four kinds of remainder that go with the four kinds of division.  */
+/* Four kinds of remainder that go with the four kinds of division:  */
+
+/* The sign of the remainder is that of the dividend.  */
 DEFTREECODE (TRUNC_MOD_EXPR, "trunc_mod_expr", tcc_binary, 2)
+
+/* The sign of the remainder is the opposite of that of the divisor.  */
 DEFTREECODE (CEIL_MOD_EXPR, "ceil_mod_expr", tcc_binary, 2)
+
+/* The sign of the remainder is that of the divisor.  */
 DEFTREECODE (FLOOR_MOD_EXPR, "floor_mod_expr", tcc_binary, 2)
+
+/* The sign of the remainder is not predictable.  */
 DEFTREECODE (ROUND_MOD_EXPR, "round_mod_expr", tcc_binary, 2)
 
 /* Division for real result.  */
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 229123)
+++ fold-const.c	(working copy)
@@ -12929,11 +12929,13 @@ tree_binary_nonnegative_warnv_p (enum tr
       return RECURSE (op0) && RECURSE (op1);
 
     case TRUNC_MOD_EXPR:
-    case CEIL_MOD_EXPR:
-    case FLOOR_MOD_EXPR:
-    case ROUND_MOD_EXPR:
       return RECURSE (op0);
 
+    case FLOOR_MOD_EXPR:
+      return RECURSE (op1);
+
+    case CEIL_MOD_EXPR:
+    case ROUND_MOD_EXPR:
     default:
       return tree_simple_nonnegative_warnv_p (code, type);
     }

Re: [patch] Fix failure of ACATS c45503c at -O2

[Richard Biener]

On Wed, Oct 21, 2015 at 3:06 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Maybe add the comments to tree.def instead.
>
> Good idea, revised patch attached.

Ok.

Thanks,
Richard.

>
>         * tree.def (CEIL_DIV_EXPR, FLOOR_DIV_EXPR, ROUND_DIV_EXPR): Tweak
>         comments.
>         (TRUNC_MOD_EXPR, CEIL_MOD_EXPR, FLOOR_MOD_EXPR, ROUND_MOD_EXPR):
>         Add comments on sign of the result.
>         * fold-const.c (tree_binary_nonnegative_warnv_p) <FLOOR_MOD_EXPR>:
>         Recurse on operand #1 instead of operand #0.
>         <CEIL_MOD_EXPR>: Do not recurse.
>         <ROUND_MOD_EXPR>: Likewise.
>
> --
> Eric Botcazou

Re: [patch] Fix failure of ACATS c45503c at -O2

[Richard Sandiford]

Eric Botcazou <ebotcazou@adacore.com> writes:
>> Maybe add the comments to tree.def instead.
>
> Good idea, revised patch attached.
>
>
> 	* tree.def (CEIL_DIV_EXPR, FLOOR_DIV_EXPR, ROUND_DIV_EXPR): Tweak
> 	comments.
> 	(TRUNC_MOD_EXPR, CEIL_MOD_EXPR, FLOOR_MOD_EXPR, ROUND_MOD_EXPR):
> 	Add comments on sign of the result.
>         * fold-const.c (tree_binary_nonnegative_warnv_p) <FLOOR_MOD_EXPR>:
>         Recurse on operand #1 instead of operand #0.
>         <CEIL_MOD_EXPR>: Do not recurse.
>         <ROUND_MOD_EXPR>: Likewise.

Thanks for fixing this and sorry for the breakage.  I do normally test
Ada but must have used the wrong baseline.

Thanks,
Richard

Re: [patch] Fix failure of ACATS c45503c at -O2

[Eric Botcazou]

> Isn't this a function of the language and in some cases isn't it
> implementation defined (true for C/C++ until C++11)?

I don't think that C/C++ use FLOOR_MOD_EXPR, only Ada does AFAIK.  In any 
case, I don't see how this can be implementation-defined given:

/* Division for integer result that rounds the quotient toward zero.  */
DEFTREECODE (TRUNC_DIV_EXPR, "trunc_div_expr", tcc_binary, 2)

/* Division for integer result that rounds the quotient toward infinity.  */
DEFTREECODE (CEIL_DIV_EXPR, "ceil_div_expr", tcc_binary, 2)

/* Division for integer result that rounds toward minus infinity.  */
DEFTREECODE (FLOOR_DIV_EXPR, "floor_div_expr", tcc_binary, 2)

/* Division for integer result that rounds toward nearest integer.  */
DEFTREECODE (ROUND_DIV_EXPR, "round_div_expr", tcc_binary, 2)

/* Four kinds of remainder that go with the four kinds of division.  */
DEFTREECODE (TRUNC_MOD_EXPR, "trunc_mod_expr", tcc_binary, 2)
DEFTREECODE (CEIL_MOD_EXPR, "ceil_mod_expr", tcc_binary, 2)
DEFTREECODE (FLOOR_MOD_EXPR, "floor_mod_expr", tcc_binary, 2)
DEFTREECODE (ROUND_MOD_EXPR, "round_mod_expr", tcc_binary, 2)

This isn't immediate but, given the definition of TRUNC_DIV_EXPR, you can 
prove that the sign of TRUNC_MOD_EXPR is that of the dividend; similarly, 
given the definition of FLOOR_DIV_EXPR, you can prove that the sign of 
FLOOR_MOD_EXPR is that of the divisor; given the definition of CEIL_DIV_EXPR, 
you can prove that the sign of CEIL_MOD_EXPR is the opposite of that of the 
divisor (but you cannot use the latter result in this context).

-- 
Eric Botcazou

Re: [patch] Fix failure of ACATS c45503c at -O2

[Joseph Myers]

On Wed, 21 Oct 2015, Eric Botcazou wrote:

> > Isn't this a function of the language and in some cases isn't it
> > implementation defined (true for C/C++ until C++11)?
> 
> I don't think that C/C++ use FLOOR_MOD_EXPR, only Ada does AFAIK.  In any 
> case, I don't see how this can be implementation-defined given:

And, given that RTL semantics are as well-defined and target-independent 
as GENERIC / GIMPLE semantics here, a corollary is that the MMIX option 
-mknuthdiv is fundamentally ill-conceived (affecting as it does the 
semantics of RTL operations whose semantics are assumed by the rest of the 
compiler to be target-independent).

-- 
Joseph S. Myers
joseph@codesourcery.com