| Message ID | 5676f26c-f6d8-4798-8bdc-2b7b07f08925@BAMAIL02.ba.imgtec.org |
|---|---|
| State | New |
| Headers | show |
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: >
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
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
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 > >
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
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
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 > >
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:
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.