[Bug middle-end/56363] New: over aggressive division folding ignores sign conversion

[Bug middle-end/56363] New: over aggressive division folding ignores sign conversion

[jay.krell at cornell dot edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56363

             Bug #: 56363
           Summary: over aggressive division folding ignores sign
                    conversion
    Classification: Unclassified
           Product: gcc
           Version: 4.7.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jay.krell@cornell.edu


Our frontend is slightly unusual in that it uses FLOOR_DIV_EXPR instead of C's
more common TRUNC_DIV_EXPR. (For years it generated calls to helper functions
for div and mod, but then I noticed this builtin support.)
So it hits perhaps not well covered code?


Based on debugging this years ago (in an older version of gcc), I think gcc is
a bit overaggressive at folding constants in division.


4.7.2/gcc/fold-const.c has this code:


tree
fold_binary_loc (location_t loc,
         enum tree_code code, tree type, tree op0, tree op1)
.
.
.
  arg0 = op0;
  arg1 = op1;
.
.
.
      STRIP_NOPS (arg0);
      STRIP_NOPS (arg1);
.
.
.
      /* If arg0 is a multiple of arg1, then rewrite to the fastest div
     operation, EXACT_DIV_EXPR.

     Note that only CEIL_DIV_EXPR and FLOOR_DIV_EXPR are rewritten now.
     At one time others generated faster code, it's not clear if they do
     after the last round to changes to the DIV code in expmed.c.  */
      if ((code == CEIL_DIV_EXPR || code == FLOOR_DIV_EXPR)
      && multiple_of_p (type, arg0, arg1))
    return fold_build2_loc (loc, EXACT_DIV_EXPR, type, arg0, arg1);


I think this is wrong if strip_nops stripped some signedness or other
conversions.


I added this condition:
      && arg0 == op0 && arg1 == op1 


Given the comment, that the code is not applied to C's TRUNC_DIV_EXPR, maybe
this can/should just be removed?

[Bug middle-end/56363] over aggressive division folding ignores sign conversion

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56363

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-02-18
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-18 10:54:03 UTC ---
Well, it should simply look at op0/op1 instead.

[Bug middle-end/56363] over aggressive division folding ignores sign conversion

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56363

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-18 11:31:06 UTC ---
Do you mean in the && multiple_of_p check, or rather just in
return fold_build2_loc (loc, EXACT_DIV_EXPR, type, arg0, arg1);
call?  Anyway, it would be better to see a testcase.

[Bug middle-end/56363] over aggressive division folding ignores sign conversion

[jay.krell at cornell dot edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56363

--- Comment #3 from Jay <jay.krell at cornell dot edu> 2013-02-18 20:50:44 UTC ---
(In reply to comment #2)
> call?  Anyway, it would be better to see a testcase.


I'm not sure I can construct one in C.
It might be too painful for you to take what I have -- a general frontend that
accepts binary files.
I can try debugging it again.
I had/have a large test vector, including most positive/negativen numbers.

 - Jay

[Bug middle-end/56363] over aggressive division folding ignores sign conversion

[jay.krell at cornell dot edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56363

--- Comment #4 from Jay <jay.krell at cornell dot edu> 2013-02-19 16:18:02 UTC ---
ah, here is more info; I reported the bug better years ago but it was never
looked at:


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46679


There I don't claim bad code, but a failure with -enable-checking.


Something, like, you are supposed to have
type = type / type  

we start with
int64 = int64 / int64

but end up with
int64 = uint64 / uint64

which "enable-checking" doesn't like.
It is possible enable-checking has been relaxed in the years since I checked. I
can try again.


 - Jay

[Bug middle-end/56363] over aggressive division folding ignores sign conversion

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56363

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-19 16:23:03 UTC ---
At least from what is described in that PR, it would look like it is just fine
to use EXACT_DIV_EXPR, but that we should pass op0 and op1 to the
fold_build2_loc call, instead of arg0/arg1.

[Bug middle-end/56363] over aggressive division folding ignores sign conversion

[jay.krell at cornell dot edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56363

--- Comment #6 from Jay <jay.krell at cornell dot edu> 2013-02-19 21:52:31 UTC ---
 > should pass op0 and op1 to the fold_build2_loc call, instead of arg0/arg1


I don't disagree, but I really I don't know.
Clearly that means almost but not exactly the same thing.


I am running a build "now" (i.e. when I left home this morning) with the bogus
change from the other bug, where I change all div forms here, with
-enable-checking, to see if it still triggers. I can also try our frontend
against 4.7.2 with -enable-checking, i.e. w/o the bogosity of changing all div
forms, and confirm there is still a problem.

[Bug middle-end/56363] over aggressive division folding ignores sign conversion

[jay.krell at cornell dot edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56363

--- Comment #7 from Jay <jay.krell at cornell dot edu> 2013-02-20 08:51:49 UTC ---
Here is the "successful experiment", where I changed the code to operate on all
the div variants. Of course I am NOT suggesting this change be made. Nor is it
100% conclusive. But it is very much indicative of a problem.


 mkdir /obj
/gcc.4
 cd /obj/gcc.4  

 /src/gcc-4.7.2/configure -prefix=$HOME/gcc.4 -enable-checking=fold
-enable-languages=c
make

...

/obj/gcc.4/./prev-gcc/g++ -B/obj/gcc.4/./prev-gcc/ ...
/src/gcc-4.7.2/gcc/fold-const.c -o fold-const.o
/src/gcc-4.7.2/gcc/fold-const.c: In function ‘tree_node*
fold_comparison(location_t, tree_code, tree, tree, tree)’:
/src/gcc-4.7.2/gcc/fold-const.c:8765:1: error: type mismatch in binary
expression
long long int

long long unsigned int

long long int

D.64567 = D.64566 /[ex] 8;

/src/gcc-4.7.2/gcc/fold-const.c:8765:1: error: type mismatch in binary
expression
long long int

long long unsigned int

long long int

D.64594 = D.64593 /[ex] 8;

/src/gcc-4.7.2/gcc/fold-const.c:8765: confused by earlier errors, bailing out
make[3]: *** [fold-const.o] Error 1
make[3]: Leaving directory `/Users/jay/obj/gcc.4/gcc'
make[2]: *** [all-stage2-gcc] Error 2
make[2]: Leaving directory `/Users/jay/obj/gcc.4'
make[1]: *** [stage2-bubble] Error 2
make[1]: Leaving directory `/Users/jay/obj/gcc.4'
make: *** [all] Error 2


jbook2:gcc-4.7.2 jay$ diff -ur /src/orig/gcc-4.7.2 /src/gcc-4.7.2
diff -ur /src/orig/gcc-4.7.2/gcc/fold-const.c /src/gcc-4.7.2/gcc/fold-const.c
--- /src/orig/gcc-4.7.2/gcc/fold-const.c    2012-06-01 10:03:19.000000000 -0700
+++ /src/gcc-4.7.2/gcc/fold-const.c    2013-02-19 08:19:31.000000000 -0800
@@ -12048,7 +12048,7 @@
      Note that only CEIL_DIV_EXPR and FLOOR_DIV_EXPR are rewritten now.
      At one time others generated faster code, it's not clear if they do
      after the last round to changes to the DIV code in expmed.c.  */
-      if ((code == CEIL_DIV_EXPR || code == FLOOR_DIV_EXPR)
+      if ((code == TRUNC_DIV_EXPR || code == ROUND_DIV_EXPR || code ==
FLOOR_DIV_EXPR || code == CEIL_DIV_EXPR)
       && multiple_of_p (type, arg0, arg1))
     return fold_build2_loc (loc, EXACT_DIV_EXPR, type, arg0, arg1);



Again, this is NOT a proposed patch. I am not that dumb. It helps SUGGEST that
there is a problem.


Perhaps I'll try with trunk.


 - Jay