Patchwork [cilk,C++] Fix cilk testsuite failure

login
register
mail settings
Submitter Steve Ellcey
Date Jan. 15, 2014, 6:50 p.m.
Message ID <5676f26c-f6d8-4798-8bdc-2b7b07f08925@BAMAIL02.ba.imgtec.org>
Download mbox | patch
Permalink /patch/311243/
State New
Headers show

Comments

Steve Ellcey - Jan. 15, 2014, 6:50 p.m.
The test c-c++-common/cilk-plus/AN/builtin_func_double2.c is failing on MIPS
when compiled with C++ but not when compiled with C because GCC is generating
an illegal assembly language instruction during the C++ compilation.  The
reason this works in C but fails for C++ is because the
__sec_reduce_any_nonzero call is creating a conditional assignment expression
and in C the condition used in that expression is type integer but in C++
it is type boolean.

In c/c-array-notation.c we have:

    case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO:
    case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
    case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
    case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
      new_var_type = integer_type_node;
      break;

new_var_type is used as the type of a variable used as the condition 'C' in
"foo = C ? 1 : 0".

But in C++ we use a boolean type instead of an integer type for the
condition.  Thus, in cp/cp-array-notation.c we have:

    case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO:
    case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
    case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
    case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
      new_var_type = boolean_type_node;
      break;

The reason this matters is that on MIPS a floating point conditional
register ($fcc0) can hold a boolean type but not an integer type
but this register cannot be used in a conditional move.  So when the
C++ code is generated GCC will create an instruction that looks like:

	movz $4,$6,$fcc0

And this is illegal.  I would like to fix this by using integer_type_node
for C++ like C is doing and I have attached a tested patch that does
this.  It fixes the bug and caused no regressions on mips-mti-linux-gnu.

One could argue that a better fix would be checking the legality of the
conditional move in expr.c and not generating an illegal one, however the
current GCC conditional move code checks for the type (mode) of the value
being moved but has no check on the type or mode of the conditional being
used to control the move so doing this would probably mean adding a new
target predicate.  Since no other tests are failing with this problem I
would rather tweak the cilk code instead.

Tested on mips-mti-linux-gnu with no regressions. 

OK to checkin?

Steve Ellcey
sellcey@mips.com


2014-01-15  Steve Ellcey  <sellcey@mips.com>

	PR target/59462
	* cp-array-notation.c (expand_sec_reduce_builtin): Change conditional
	type.
Andrew Pinski - Jan. 16, 2014, 4:23 a.m.
On Wed, Jan 15, 2014 at 10:50 AM, Steve Ellcey <sellcey@mips.com> wrote:
> The test c-c++-common/cilk-plus/AN/builtin_func_double2.c is failing on MIPS
> when compiled with C++ but not when compiled with C because GCC is generating
> an illegal assembly language instruction during the C++ compilation.  The
> reason this works in C but fails for C++ is because the
> __sec_reduce_any_nonzero call is creating a conditional assignment expression
> and in C the condition used in that expression is type integer but in C++
> it is type boolean.
>
> In c/c-array-notation.c we have:
>
>     case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO:
>     case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
>     case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
>     case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
>       new_var_type = integer_type_node;
>       break;
>
> new_var_type is used as the type of a variable used as the condition 'C' in
> "foo = C ? 1 : 0".
>
> But in C++ we use a boolean type instead of an integer type for the
> condition.  Thus, in cp/cp-array-notation.c we have:
>
>     case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO:
>     case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
>     case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
>     case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
>       new_var_type = boolean_type_node;
>       break;
>
> The reason this matters is that on MIPS a floating point conditional
> register ($fcc0) can hold a boolean type but not an integer type
> but this register cannot be used in a conditional move.  So when the
> C++ code is generated GCC will create an instruction that looks like:
>
>         movz $4,$6,$fcc0
>
> And this is illegal.  I would like to fix this by using integer_type_node
> for C++ like C is doing and I have attached a tested patch that does
> this.  It fixes the bug and caused no regressions on mips-mti-linux-gnu.
>
> One could argue that a better fix would be checking the legality of the
> conditional move in expr.c and not generating an illegal one, however the
> current GCC conditional move code checks for the type (mode) of the value
> being moved but has no check on the type or mode of the conditional being
> used to control the move so doing this would probably mean adding a new
> target predicate.  Since no other tests are failing with this problem I
> would rather tweak the cilk code instead.
>
> Tested on mips-mti-linux-gnu with no regressions.
>
> OK to checkin?

No, this really should be fixed in the target side.  In fact for MIPS
there is an instruction which does a conditional move based on the FP
CC register (movt/movf).  So you need to look into why the wrong
pattern is being selected instead.

Thanks,
Andrew Pinski

>
> Steve Ellcey
> sellcey@mips.com
>
>
> 2014-01-15  Steve Ellcey  <sellcey@mips.com>
>
>         PR target/59462
>         * cp-array-notation.c (expand_sec_reduce_builtin): Change conditional
>         type.
>
>
> diff --git a/gcc/cp/cp-array-notation.c b/gcc/cp/cp-array-notation.c
> index 65b8bcb..6cba0b4 100644
> --- a/gcc/cp/cp-array-notation.c
> +++ b/gcc/cp/cp-array-notation.c
> @@ -274,7 +274,7 @@ expand_sec_reduce_builtin (tree an_builtin_fn, tree *new_var)
>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
> -      new_var_type = boolean_type_node;
> +      new_var_type = integer_type_node;
>        break;
>      case BUILT_IN_CILKPLUS_SEC_REDUCE_MAX_IND:
>      case BUILT_IN_CILKPLUS_SEC_REDUCE_MIN_IND:
>
Jeff Law - Jan. 16, 2014, 5:47 a.m.
On 01/15/14 21:23, Andrew Pinski wrote:
> On Wed, Jan 15, 2014 at 10:50 AM, Steve Ellcey <sellcey@mips.com> wrote:
>> The test c-c++-common/cilk-plus/AN/builtin_func_double2.c is failing on MIPS
>> when compiled with C++ but not when compiled with C because GCC is generating
>> an illegal assembly language instruction during the C++ compilation.  The
>> reason this works in C but fails for C++ is because the
>> __sec_reduce_any_nonzero call is creating a conditional assignment expression
>> and in C the condition used in that expression is type integer but in C++
>> it is type boolean.
>>
>> In c/c-array-notation.c we have:
>>
>>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO:
>>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
>>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
>>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
>>        new_var_type = integer_type_node;
>>        break;
>>
>> new_var_type is used as the type of a variable used as the condition 'C' in
>> "foo = C ? 1 : 0".
>>
>> But in C++ we use a boolean type instead of an integer type for the
>> condition.  Thus, in cp/cp-array-notation.c we have:
>>
>>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO:
>>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
>>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
>>      case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
>>        new_var_type = boolean_type_node;
>>        break;
>>
>> The reason this matters is that on MIPS a floating point conditional
>> register ($fcc0) can hold a boolean type but not an integer type
>> but this register cannot be used in a conditional move.  So when the
>> C++ code is generated GCC will create an instruction that looks like:
>>
>>          movz $4,$6,$fcc0
>>
>> And this is illegal.  I would like to fix this by using integer_type_node
>> for C++ like C is doing and I have attached a tested patch that does
>> this.  It fixes the bug and caused no regressions on mips-mti-linux-gnu.
>>
>> One could argue that a better fix would be checking the legality of the
>> conditional move in expr.c and not generating an illegal one, however the
>> current GCC conditional move code checks for the type (mode) of the value
>> being moved but has no check on the type or mode of the conditional being
>> used to control the move so doing this would probably mean adding a new
>> target predicate.  Since no other tests are failing with this problem I
>> would rather tweak the cilk code instead.
>>
>> Tested on mips-mti-linux-gnu with no regressions.
>>
>> OK to checkin?
>
> No, this really should be fixed in the target side.  In fact for MIPS
> there is an instruction which does a conditional move based on the FP
> CC register (movt/movf).  So you need to look into why the wrong
> pattern is being selected instead.
Or maybe during expansion.

jeff
Steve Ellcey - Jan. 16, 2014, 8:37 p.m.
On Wed, 2014-01-15 at 22:47 -0700, Jeff Law wrote:
> On 01/15/14 21:23, Andrew Pinski wrote:

> >> Tested on mips-mti-linux-gnu with no regressions.
> >>
> >> OK to checkin?
> >
> > No, this really should be fixed in the target side.  In fact for MIPS
> > there is an instruction which does a conditional move based on the FP
> > CC register (movt/movf).  So you need to look into why the wrong
> > pattern is being selected instead.

> Or maybe during expansion.
> 
> jeff

Well, I thought it was worth a shot.  Looking at the instruction causing
the problem we have the RTX:

            (insn:TI 76 79 98 (set (reg:SI 2 $2 [orig:228 D.1939+-3 ] [228])
                    (if_then_else:SI (ne:SI (reg:CC 67 $fcc0)
                            (const_int 0 [0]))
                        (reg:SI 2 $2 [orig:228 D.1939+-3 ] [228])
                        (reg:SI 4 $4 [246]))) 609 {*movsi_on_cc}
                 (nil))

And it is being handled by the define_insn:

;; MIPS4 Conditional move instructions.

(define_insn "*mov<GPR:mode>_on_<MOVECC:mode>"
  [(set (match_operand:GPR 0 "register_operand" "=d,d")
        (if_then_else:GPR
         (match_operator 4 "equality_operator"
                [(match_operand:MOVECC 1 "register_operand" "<MOVECC:reg>,<MOVECC:reg>")
                 (const_int 0)])
         (match_operand:GPR 2 "reg_or_0_operand" "dJ,0")
         (match_operand:GPR 3 "reg_or_0_operand" "0,dJ")))]
  "ISA_HAS_CONDMOVE"
  "@
    mov%T4\t%0,%z2,%1
    mov%t4\t%0,%z3,%1"
  [(set_attr "type" "condmove")
   (set_attr "mode" "<GPR:MODE>")])

%T4 is using the mode of operand 4 to determine if it should generate
movz or movf.  If that mode is CC it uses movf/movt and otherwise it uses
movz/movn.  I tried tweaking the conditional move define_insn to use
%T1 and %t1 instead of %T4 and %t4 but that caused regressions because the
%T code is also looking at the node to see if it is a NE or a EQ.

Given that we have a CC reg type is it reasonable/correct that the ne
comparision is SI mode and not CC mode?

Steve Ellcey
sellcey@mips.com
Andrew Pinski - Jan. 16, 2014, 9:16 p.m.
On Thu, Jan 16, 2014 at 12:37 PM, Steve Ellcey <sellcey@mips.com> wrote:
> On Wed, 2014-01-15 at 22:47 -0700, Jeff Law wrote:
>> On 01/15/14 21:23, Andrew Pinski wrote:
>
>> >> Tested on mips-mti-linux-gnu with no regressions.
>> >>
>> >> OK to checkin?
>> >
>> > No, this really should be fixed in the target side.  In fact for MIPS
>> > there is an instruction which does a conditional move based on the FP
>> > CC register (movt/movf).  So you need to look into why the wrong
>> > pattern is being selected instead.
>
>> Or maybe during expansion.
>>
>> jeff
>
> Well, I thought it was worth a shot.  Looking at the instruction causing
> the problem we have the RTX:
>
>             (insn:TI 76 79 98 (set (reg:SI 2 $2 [orig:228 D.1939+-3 ] [228])
>                     (if_then_else:SI (ne:SI (reg:CC 67 $fcc0)
>                             (const_int 0 [0]))
>                         (reg:SI 2 $2 [orig:228 D.1939+-3 ] [228])
>                         (reg:SI 4 $4 [246]))) 609 {*movsi_on_cc}
>                  (nil))
>
> And it is being handled by the define_insn:
>
> ;; MIPS4 Conditional move instructions.
>
> (define_insn "*mov<GPR:mode>_on_<MOVECC:mode>"
>   [(set (match_operand:GPR 0 "register_operand" "=d,d")
>         (if_then_else:GPR
>          (match_operator 4 "equality_operator"
>                 [(match_operand:MOVECC 1 "register_operand" "<MOVECC:reg>,<MOVECC:reg>")
>                  (const_int 0)])
>          (match_operand:GPR 2 "reg_or_0_operand" "dJ,0")
>          (match_operand:GPR 3 "reg_or_0_operand" "0,dJ")))]
>   "ISA_HAS_CONDMOVE"
>   "@
>     mov%T4\t%0,%z2,%1
>     mov%t4\t%0,%z3,%1"
>   [(set_attr "type" "condmove")
>    (set_attr "mode" "<GPR:MODE>")])
>
> %T4 is using the mode of operand 4 to determine if it should generate
> movz or movf.  If that mode is CC it uses movf/movt and otherwise it uses
> movz/movn.  I tried tweaking the conditional move define_insn to use
> %T1 and %t1 instead of %T4 and %t4 but that caused regressions because the
> %T code is also looking at the node to see if it is a NE or a EQ.
>
> Given that we have a CC reg type is it reasonable/correct that the ne
> comparision is SI mode and not CC mode?

I think the following patch to mips.c should fix the issue:
@@ -8092,7 +8125,7 @@ mips_print_operand (FILE *file, rtx op, int letter)
     case 't':
       {
        int truth = (code == NE) == (letter == 'T');
-       fputc ("zfnt"[truth * 2 + (GET_MODE (op) == CCmode)], file);
+       fputc ("zfnt"[truth * 2 + (GET_MODE (XEXP (op, 0)) == CCmode)], file);
       }
       break;

----- CUT ----

So this bug was introduced by:
2013-11-18  Andrew Pinski <apinski@cavium.com>
            Steve Ellcey  <sellcey@mips.com>

        PR target/56552
        * config/mips/mips.md (*mov<GPR:mode>_on_<MOVECC:mode>): Remove
        type restriction from equality_operator on conditonal move.
        (*mov<SCALARF:mode>_on_<MOVECC:mode>): Ditto.
        (*mov<GPR:mode>_on_<GPR2:mode>_ne): New.

I forgot when I submitted that I had fixed up the hard float case
later on as I put the fix as part of my octeon3 (the octeon which fp)
patch.

Thanks,
Andrew Pinski

>
> Steve Ellcey
> sellcey@mips.com
>
>
Steve Ellcey - Jan. 16, 2014, 9:19 p.m.
On Wed, 2014-01-15 at 22:47 -0700, Jeff Law wrote:

> > No, this really should be fixed in the target side.  In fact for MIPS
> > there is an instruction which does a conditional move based on the FP
> > CC register (movt/movf).  So you need to look into why the wrong
> > pattern is being selected instead.

> Or maybe during expansion.
> 
> jeff

It looks like this is going wrong in combine.  Before combine I have:


(insn 75 74 76 4 (set (reg:SI 240)
        (if_then_else:SI (ne:CC (reg:CC 67 $fcc0)
                (const_int 0 [0]))
            (reg:SI 228 [ D.1939+-3 ])
            (reg:SI 246))) 609 {*movsi_on_cc}
     (expr_list:REG_DEAD (reg:SI 228 [ D.1939+-3 ])
        (expr_list:REG_DEAD (reg:CC 67 $fcc0)
            (expr_list:REG_EQUAL (if_then_else:SI (ne:CC (reg:CC 67 $fcc0)
                        (const_int 0 [0]))
                    (reg:SI 228 [ D.1939+-3 ])
                    (const_int 1 [0x1]))
                (nil)))))
(insn 76 75 77 4 (set (reg:SI 228 [ D.1939+-3 ])
        (zero_extend:SI (subreg:QI (reg:SI 240) 3))) 185 {*zero_extendqisi2}
     (expr_list:REG_DEAD (reg:SI 240)
        (nil)))


After combine I have:

(note 75 74 76 4 NOTE_INSN_DELETED)
(insn 76 75 77 4 (set (reg:SI 228 [ D.1939+-3 ])
        (if_then_else:SI (ne:SI (reg:CC 67 $fcc0)
                (const_int 0 [0]))
            (reg:SI 228 [ D.1939+-3 ])
            (reg:SI 246))) 609 {*movsi_on_cc}
     (expr_list:REG_DEAD (reg:CC 67 $fcc0)
        (nil)))

The ne:CC in instruction 75 has become a ne:SI in instruction 76 after the combine.

Steve Ellcey
sellcey@mips.com
Steve Ellcey - Jan. 16, 2014, 11:22 p.m.
On Thu, 2014-01-16 at 13:16 -0800, Andrew Pinski wrote:

> I think the following patch to mips.c should fix the issue:
> @@ -8092,7 +8125,7 @@ mips_print_operand (FILE *file, rtx op, int letter)
>      case 't':
>        {
>         int truth = (code == NE) == (letter == 'T');
> -       fputc ("zfnt"[truth * 2 + (GET_MODE (op) == CCmode)], file);
> +       fputc ("zfnt"[truth * 2 + (GET_MODE (XEXP (op, 0)) == CCmode)], file);
>        }
>        break;

Yes, that patch fixed my problem and caused no regressions with my
mips-mti-linux-gnu target.  Can you submit a formal patch for
checkin?  Or would you like me to do that?

Steve Ellcey
sellcey@mips.com
Andrew Pinski - Jan. 16, 2014, 11:48 p.m.
On Thu, Jan 16, 2014 at 3:22 PM, Steve Ellcey <sellcey@mips.com> wrote:
> On Thu, 2014-01-16 at 13:16 -0800, Andrew Pinski wrote:
>
>> I think the following patch to mips.c should fix the issue:
>> @@ -8092,7 +8125,7 @@ mips_print_operand (FILE *file, rtx op, int letter)
>>      case 't':
>>        {
>>         int truth = (code == NE) == (letter == 'T');
>> -       fputc ("zfnt"[truth * 2 + (GET_MODE (op) == CCmode)], file);
>> +       fputc ("zfnt"[truth * 2 + (GET_MODE (XEXP (op, 0)) == CCmode)], file);
>>        }
>>        break;
>
> Yes, that patch fixed my problem and caused no regressions with my
> mips-mti-linux-gnu target.  Can you submit a formal patch for
> checkin?  Or would you like me to do that?

Can you do it as I am busy with many other internal projects?

Thanks,
Andrew


>
> Steve Ellcey
> sellcey@mips.com
>
>

Patch

diff --git a/gcc/cp/cp-array-notation.c b/gcc/cp/cp-array-notation.c
index 65b8bcb..6cba0b4 100644
--- a/gcc/cp/cp-array-notation.c
+++ b/gcc/cp/cp-array-notation.c
@@ -274,7 +274,7 @@  expand_sec_reduce_builtin (tree an_builtin_fn, tree *new_var)
     case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
     case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
     case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
-      new_var_type = boolean_type_node;
+      new_var_type = integer_type_node;
       break;
     case BUILT_IN_CILKPLUS_SEC_REDUCE_MAX_IND:
     case BUILT_IN_CILKPLUS_SEC_REDUCE_MIN_IND: