Patchwork RFC: PR rtl-optimization/44695: [4.6 Regression] ice in simplify_subreg, at simplify-rtx.c:5117

login
register
mail settings
Submitter H.J. Lu
Date June 30, 2010, 6:45 p.m.
Message ID <AANLkTilRqA8xdQlrTmf-GfZrYgkpYvDZTmU7TLhN3Bxk@mail.gmail.com>
Download mbox | patch
Permalink /patch/57446/
State New
Headers show

Comments

H.J. Lu - June 30, 2010, 6:45 p.m.
On Wed, Jun 30, 2010 at 4:53 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
> On 06/29/2010 09:12 PM, H.J. Lu wrote:
>>
>> After revision 161329, x86 backend may generate:
>>
>> (ior:HI (ashift:HI (zero_extend:HI (umod:QI (reg:HI 68)
>>                 (reg:QI 61 [ D.2750 ])))
>>         (const_int 8 [0x8]))
>>     (zero_extend:HI (udiv:QI (reg:HI 68)
>>             (reg:QI 61 [ D.2750 ]))))
>>
>> combine calls simplify_subreg and leads to
>>
>> #2  0x00000000009ad621 in simplify_subreg (outermode=HImode,
>>     op=0x7ffff7facd40, innermode=QImode, byte=0)
>>     at /space/rguenther/src/svn/trunk/gcc/simplify-rtx.c:5116
>> 5116      gcc_assert (GET_MODE (op) == innermode
>> 5117                  || GET_MODE (op) == VOIDmode);
>> (gdb) call debug_rtx (op)
>> (reg:HI 68)
>>
>> This patch adds simplify_gen_subreg_for_combine to deal with invalid
>> combination of outermode, op and innermode generated by combine.  Any
>> comments?
>
> I commented in the PR about the correct fix.  Sorry for not spotting it in
> the review (I had a doubt, but in the end I didn't look it up and left the
> final call to Uros...).
>

Here is a patch to change 8bit div/mod to HImode.  OK for trunk?

Thanks.
Paolo Bonzini - July 1, 2010, 6:59 a.m.
This is wrong. GCC may use this for a 16 to 8 bit division and cause a
divide overflow.

My comment in the PR is based on how GCC handles extension of the
dividend for 16/32/64 bit operands.

Paolo
H.J. Lu - July 1, 2010, 1:50 p.m.
On Wed, Jun 30, 2010 at 11:59 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
> This is wrong. GCC may use this for a 16 to 8 bit division and cause a
> divide overflow.

The old divide pattern is

(define_insn "<u>divqi3"
  [(set (match_operand:QI 0 "register_operand" "=a")
        (any_div:QI
          (match_operand:HI 1 "register_operand" "0")
          (match_operand:QI 2 "nonimmediate_operand" "qm")))
   (clobber (reg:CC FLAGS_REG))]
  "TARGET_QIMODE_MATH"
  "<sgnprefix>div{b}\t%2"
  [(set_attr "type" "idiv")
   (set_attr "mode" "QI")])

Why is it OK to have HImode on operand 1? The current one is

(define_insn "divmodhiqi3"
  [(set (match_operand:HI 0 "register_operand" "=a")
        (ior:HI
          (ashift:HI
            (zero_extend:HI
              (mod:QI (match_operand:HI 1 "register_operand" "0")
                      (match_operand:QI 2 "nonimmediate_operand" "qm")))
            (const_int 8))
          (zero_extend:HI (div:QI (match_dup 1) (match_dup 2)))))
   (clobber (reg:CC FLAGS_REG))]
  "TARGET_QIMODE_MATH"
  "idiv{b}\t%2"
  [(set_attr "type" "idiv")
   (set_attr "mode" "QI")])

Why isn't HImode on operand 1 OK?

> My comment in the PR is based on how GCC handles extension of the
> dividend for 16/32/64 bit operands.
>

Your suggestion generate a pattern unsupported by hardware
before reload and introduces an extra register move during split
to fix it.
Paolo Bonzini - July 1, 2010, 2:18 p.m.
> Why isn't HImode on operand 1 OK?

Because modes don't match between the div and its operands.

>> My comment in the PR is based on how GCC handles extension of the
>> dividend for 16/32/64 bit operands.
>>
>
> Your suggestion generate a pattern unsupported by hardware
> before reload

That's what 16/32/64 bit operands do too

(define_expand "divmod<mode>4"
   [(parallel [(set (match_operand:SWIM248 0 "register_operand" "")
                    (div:SWIM248
                      (match_operand:SWIM248 1 "register_operand" "")
                      (match_operand:SWIM248 2 "nonimmediate_operand" "")))
               (set (match_operand:SWIM248 3 "register_operand" "")
                    (mod:SWIM248 (match_dup 1) (match_dup 2)))
               (clobber (reg:CC FLAGS_REG))])]
   ""
   "")


> and introduces an extra register move during split to fix it.

I think you mean one extra extension?  That's possible, though it could 
be fixed in combine using nonzero_bits or num_sign_bit_copies.

Anyway, can you please post the patch and the result?  I think you 
should go one step at a time and first fix this bug, then optimize it.

Paolo

Patch

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index e361fd7..1bbb0dd 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -7364,15 +7364,18 @@ 
 ;; Divide AX by r/m8, with result stored in
 ;; AL <- Quotient
 ;; AH <- Remainder
+;; Change div/mod to HImode and extend the second argument to HImode
+;; so that mode of div/mod matches with mode of arguments.  Otherwise
+;; combine may fail.
 (define_insn "divmodhiqi3"
   [(set (match_operand:HI 0 "register_operand" "=a")
 	(ior:HI
 	  (ashift:HI
-	    (zero_extend:HI
-	      (mod:QI (match_operand:HI 1 "register_operand" "0")
+	    (mod:HI (match_operand:HI 1 "register_operand" "0")
+		    (sign_extend:HI
 		      (match_operand:QI 2 "nonimmediate_operand" "qm")))
 	    (const_int 8))
-	  (zero_extend:HI (div:QI (match_dup 1) (match_dup 2)))))
+	  (div:HI (match_dup 1) (sign_extend:HI (match_dup 2)))))
    (clobber (reg:CC FLAGS_REG))]
   "TARGET_QIMODE_MATH"
   "idiv{b}\t%2"
@@ -7383,11 +7386,11 @@ 
   [(set (match_operand:HI 0 "register_operand" "=a")
 	(ior:HI
 	  (ashift:HI
-	    (zero_extend:HI
-	      (umod:QI (match_operand:HI 1 "register_operand" "0")
+	    (umod:HI (match_operand:HI 1 "register_operand" "0")
+		     (zero_extend:HI
 		       (match_operand:QI 2 "nonimmediate_operand" "qm")))
 	    (const_int 8))
-	  (zero_extend:HI (udiv:QI (match_dup 1) (match_dup 2)))))
+	  (udiv:HI (match_dup 1) (zero_extend:HI (match_dup 2)))))
    (clobber (reg:CC FLAGS_REG))]
   "TARGET_QIMODE_MATH"
   "div{b}\t%2"
--- /dev/null	2010-06-16 10:11:06.602750711 -0700
+++ gcc/gcc/testsuite/gcc.dg/torture/pr44695.c	2010-06-29 11:49:12.186942482 -0700
@@ -0,0 +1,15 @@ 
+/* { dg-do compile } */
+
+typedef unsigned char uint8_t;
+
+static uint8_t
+safe_div_func_uint8_t_u_u (uint8_t ui1, uint8_t ui2)
+{
+  return ui2 ? ui2 : (ui1 / ui2);
+}
+
+int
+int81 (int x)
+{
+  return safe_div_func_uint8_t_u_u (1, 8 & x);
+}