Hi Richard, On Wed, 2010-07-28 at 21:15 +0100, Richard Sandiford wrote: > Ramana Radhakrishnan writes: > > Hmmm there are regressions after my patch was applied. I had to do some > > rebuilds because trunk appears to be broken for other reasons at the > > minute for ARM which I shall investigate next. > > Yeah, I think it's the other way about: expand_widen_pattern_expr > is using the right type and convert_plusminus_to_widen is using > the wrong one. The type we pass to optab_for_tree_code determines > the _sign_ of the multiplication, not the size, so we want to pass > in the unwidened type. Thanks for looking at this. Ah - ok. I see what you mean here. > > > We weren't generating any widening operations before this patch (and I > > suspect my 2 patches are just for fixing the ICE's and doing the right > > thing). If it were just adding support for fixed point arithmetic then > > this is just broken because it shouldn't have changed the way in which > > code was generated for normal integer operations. > > For the record, the patch wasn't just about adding fixed-point support. > It was also about allowing multiply-accumulate to be used where no plain > multiplication exists. So the problem for ARM was that the use of the > wrong type in convert_plusminus_to_widen was being hidden by the use of > the right type in convert_mult_widen (i.e. the patch exposed a latent bug). Thanks for the clarification , that makes more sense now. > > Could you give the attached patch a try? It also fixes a case where > the code could create mixed sign-and-unsigned maccs (a bug that existed > before my patch) and a botched call to is_widening_mult_p (a bug > introduced by my patch, sorry). Your patch didn't apply cleanly to pristine trunk or to 162431 and I had to do a manual merge so I'm not sure if I got all parts of it. Just to be absolutely clear, you still need to check for is_gimple_assign before using the stmt in gimple_assign_lhs in is_widening_mult_p or do you expect that to get subsumed by your patch ? Otherwise the compiler ICEs with this testcase [ice-on-gimple-phi.c]. Here's what I came up with that includes a manual merge of your patch assuming I got all the hunks, and my original trivial hunk to is_widening_mult_p which appeared to fix all the problems with simple testcases on trunk with a cross-compiler and what I'm bootstrapping and testing right now. cheers Ramana gcc/ * tree-ssa-math-opts.c (convert_plusminus_to_widen): Fix type used in the call to optab_for_tree_code. Fix the second is_widening_mult_p call. Check that both unwidened operands have the same sign. (is_widening_mult_p): Return false if stmt is not a GIMPLE_ASSIGN. [1] ./xgcc -B`pwd` -S -O2 -mcpu=cortex-a9 ~/ice-on-gimple-phi.c assignment./home/ramrad01/ice-on-gimple-phi.c:57:1: internal compiler error: gimple check: expected gimple_assign(error_mark), have gimple_phi() in gimple_assign_lhs, at gimple.h:1724 > > Richard > > > gcc/ > * tree-ssa-math-opts.c (convert_plusminus_to_widen): Fix type > used in the call to optab_for_tree_code. Fix the second > is_widening_mult_p call. Check that both unwidened operands > have the same sign. > > Index: gcc/tree-ssa-math-opts.c > =================================================================== > --- gcc/tree-ssa-math-opts.c 2010-07-28 21:09:59.000000000 +0100 > +++ gcc/tree-ssa-math-opts.c 2010-07-28 21:10:00.000000000 +0100 > @@ -1414,13 +1414,6 @@ convert_plusminus_to_widen (gimple_stmt_ > else > wmult_code = WIDEN_MULT_PLUS_EXPR; > > - /* Verify that the machine can perform a widening multiply > - accumulate in this mode/signedness combination, otherwise > - this transformation is likely to pessimize code. */ > - this_optab = optab_for_tree_code (wmult_code, type, optab_default); > - if (optab_handler (this_optab, TYPE_MODE (type)) == CODE_FOR_nothing) > - return false; > - > rhs1 = gimple_assign_rhs1 (stmt); > rhs2 = gimple_assign_rhs2 (stmt); > > @@ -1447,37 +1440,49 @@ convert_plusminus_to_widen (gimple_stmt_ > if (!is_widening_mult_p (rhs1_stmt, &type1, &mult_rhs1, > &type2, &mult_rhs2)) > return false; > - mult_rhs1 = fold_convert (type1, mult_rhs1); > - mult_rhs2 = fold_convert (type2, mult_rhs2); > add_rhs = rhs2; > } > else if (rhs2_code == MULT_EXPR) > { > - if (!is_widening_mult_p (rhs1_stmt, &type1, &mult_rhs1, > + if (!is_widening_mult_p (rhs2_stmt, &type1, &mult_rhs1, > &type2, &mult_rhs2)) > return false; > - mult_rhs1 = fold_convert (type1, mult_rhs1); > - mult_rhs2 = fold_convert (type2, mult_rhs2); > add_rhs = rhs1; > } > else if (code == PLUS_EXPR && rhs1_code == WIDEN_MULT_EXPR) > { > mult_rhs1 = gimple_assign_rhs1 (rhs1_stmt); > mult_rhs2 = gimple_assign_rhs2 (rhs1_stmt); > + type1 = TREE_TYPE (mult_rhs1); > + type2 = TREE_TYPE (mult_rhs2); > add_rhs = rhs2; > } > else if (rhs2_code == WIDEN_MULT_EXPR) > { > mult_rhs1 = gimple_assign_rhs1 (rhs2_stmt); > mult_rhs2 = gimple_assign_rhs2 (rhs2_stmt); > + type1 = TREE_TYPE (mult_rhs1); > + type2 = TREE_TYPE (mult_rhs2); > add_rhs = rhs1; > } > else > return false; > > + if (TYPE_UNSIGNED (type1) != TYPE_UNSIGNED (type2)) > + return false; > + > + /* Verify that the machine can perform a widening multiply > + accumulate in this mode/signedness combination, otherwise > + this transformation is likely to pessimize code. */ > + this_optab = optab_for_tree_code (wmult_code, type1, optab_default); > + if (optab_handler (this_optab, TYPE_MODE (type)) == CODE_FOR_nothing) > + return false; > + > /* ??? May need some type verification here? */ > > - gimple_assign_set_rhs_with_ops_1 (gsi, wmult_code, mult_rhs1, mult_rhs2, > + gimple_assign_set_rhs_with_ops_1 (gsi, wmult_code, > + fold_convert (type1, mult_rhs1), > + fold_convert (type2, mult_rhs2), > add_rhs); > update_stmt (gsi_stmt (*gsi)); > return true;