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: