| Message ID | 1280233130.18689.17.camel@e102325-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
On 07/27/2010 02:18 PM, Ramana Radhakrishnan wrote: > > On Mon, 2010-07-19 at 23:53 +0200, Bernd Schmidt wrote: >> That's pretty much what I had in mind. Ok if it passes testing. > > This broke bootstraps on arm-linux-gnueabi and caused > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45067 . > > Taking a brief look at it this morning, I think the problem here is that > the call to optab_for_tree_code is made with the wrong type of the > expression. The presence of a widening optab for a WIDEN_MULT_PLUS_EXPR > should be based on the type of ops->op2 rather than opnd0 because you > have something like intval0 = shortval1 * shortval2 + intval1 whereas > the expander is testing for a widening mult and plus where the results > are into a HImode value and thus effectively for a widening operation > into an HImode value. > > The assert that fails is because > gcc_assert (icode != CODE_FOR_nothing); > > Something like this fixes the problem for the test-cases under question > or would you prefer something else. A full bootstrap and test is > underway. > > Ok to commit if no regressions ? I think this is consistent with the code in tree-ssa-math-opts (there we use the type of the lhs, which should match that of operand 2). So, OK - I think the ARM testsuite has some widening-macc cases so you should notice if there's something wrong. > + { > + widen_pattern_optab = > + optab_for_tree_code (ops->code, TREE_TYPE (ops->op2), > optab_default); > + icode = (int) optab_handler (widen_pattern_optab, > TYPE_MODE (TREE_TYPE (ops->op2))); > + } > else > - icode = (int) optab_handler (widen_pattern_optab, tmode0); > + { > + widen_pattern_optab = > + optab_for_tree_code (ops->code, TREE_TYPE (oprnd0), > optab_default); > + icode = (int) optab_handler (widen_pattern_optab, tmode0); > + } This can be written slightly more simply assigning the type to some variable and then using that when computing widen_pattern_optab and icode. It would be nice to change that but I won't require it. Bernd
On Tue, 2010-07-27 at 17:51 +0200, Bernd Schmidt wrote: > On 07/27/2010 02:18 PM, Ramana Radhakrishnan wrote: > > > > On Mon, 2010-07-19 at 23:53 +0200, Bernd Schmidt wrote: > >> That's pretty much what I had in mind. Ok if it passes testing. > > > > This broke bootstraps on arm-linux-gnueabi and caused > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45067 . > > > > Taking a brief look at it this morning, I think the problem here is that > > the call to optab_for_tree_code is made with the wrong type of the > > expression. The presence of a widening optab for a WIDEN_MULT_PLUS_EXPR > > should be based on the type of ops->op2 rather than opnd0 because you > > have something like intval0 = shortval1 * shortval2 + intval1 whereas > > the expander is testing for a widening mult and plus where the results > > are into a HImode value and thus effectively for a widening operation > > into an HImode value. > > > > The assert that fails is because > > gcc_assert (icode != CODE_FOR_nothing); > > > > Something like this fixes the problem for the test-cases under question > > or would you prefer something else. A full bootstrap and test is > > underway. > > > > Ok to commit if no regressions ? > > I think this is consistent with the code in tree-ssa-math-opts (there we > use the type of the lhs, which should match that of operand 2). So, OK > - I think the ARM testsuite has some widening-macc cases so you should > notice if there's something wrong. > > > + { > > + widen_pattern_optab = > > + optab_for_tree_code (ops->code, TREE_TYPE (ops->op2), > > optab_default); > > + icode = (int) optab_handler (widen_pattern_optab, > > TYPE_MODE (TREE_TYPE (ops->op2))); > > + } > > else > > - icode = (int) optab_handler (widen_pattern_optab, tmode0); > > + { > > + widen_pattern_optab = > > + optab_for_tree_code (ops->code, TREE_TYPE (oprnd0), > > optab_default); > > + icode = (int) optab_handler (widen_pattern_optab, tmode0); > > + } > > This can be written slightly more simply assigning the type to some > variable and then using that when computing widen_pattern_optab and > icode. It would be nice to change that but I won't require it. 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. After these 2 patches to fix the ICEs on top of r162301 - I see breakages in the testsuite with execute/arith-rand.c and execute/arith-rand-ll.c . The difference in code generated is in the following case for -O2 is the following for arith-rand.c smlabb sl, r0, r7, sl | mla sl, r7, r0, sl with the failing case on the left. The problem appears to be as though the gimple pass for generating widening macs now generates a widening multiply and add for the case under question here while it didn't earlier. D.2130_36 = WIDEN_MULT_PLUS_EXPR <r1_30, yy_29, D.2129_35>; 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. Ideas ? I haven't looked at this in any great depth but the essential breakages in the C only testsuite are as below [2]. The native compiler has been configured with the following options - you can see the same result using a cross-compiler with the following parameters. --with-cpu=cortex-a9 --with-fpu=vfpv3-d16 --with-float=softfp cheers Ramana [1] Trunk as of this morning gives me a miscompare in stage2 and stage3 when my patches are applied which indicates a different underlying problem, the testsuite breakages are 162301 + the two patches applied. [2] FAIL: gcc.c-torture/execute/builtins/abs-1.c execution, -O2 -fwhopr FAIL: gcc.c-torture/execute/builtins/strstr-asm.c execution, -O2 -fwhopr FAIL: gcc.c-torture/execute/arith-rand-ll.c execution, -O2 FAIL: gcc.c-torture/execute/arith-rand-ll.c execution, -O3 -fomit-frame-pointer FAIL: gcc.c-torture/execute/arith-rand-ll.c execution, -O3 -fomit-frame-pointer -funroll-loops FAIL: gcc.c-torture/execute/arith-rand-ll.c execution, -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions FAIL: gcc.c-torture/execute/arith-rand-ll.c execution, -O3 -g FAIL: gcc.c-torture/execute/arith-rand-ll.c execution, -Os FAIL: gcc.c-torture/execute/arith-rand-ll.c execution, -O2 -flto FAIL: gcc.c-torture/execute/arith-rand-ll.c execution, -O2 -fwhopr FAIL: gcc.c-torture/execute/arith-rand.c execution, -O2 FAIL: gcc.c-torture/execute/arith-rand.c execution, -O3 -fomit-frame-pointer FAIL: gcc.c-torture/execute/arith-rand.c execution, -O3 -fomit-frame-pointer -funroll-loops FAIL: gcc.c-torture/execute/arith-rand.c execution, -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions FAIL: gcc.c-torture/execute/arith-rand.c execution, -O3 -g FAIL: gcc.c-torture/execute/arith-rand.c execution, -Os FAIL: gcc.c-torture/execute/arith-rand.c execution, -O2 -flto FAIL: gcc.c-torture/execute/arith-rand.c execution, -O2 -fwhopr FAIL: gcc.dg/c99-stdint-1.c (test for excess errors) FAIL: gcc.dg/c99-stdint-7.c (test for excess errors) FAIL: gcc.dg/cproj-fails-with-broken-glibc.c execution test FAIL: gcc.dg/uninit-13.c unconditional (test for warnings, line 8) FAIL: gcc.dg/uninit-13.c (test for excess errors) FAIL: gcc.dg/graphite/pr44391.c (test for excess errors) FAIL: gcc.dg/lto/20081111 c_lto_20081111_0.o-c_lto_20081111_1.o execute -O2 -fwhopr FAIL: gcc.dg/lto/20081112 c_lto_20081112_0.o-c_lto_20081112_1.o execute -O2 -fwhopr FAIL: gcc.dg/lto/ipareference2 c_lto_ipareference2_0.o-c_lto_ipareference2_1.o execute -O1 -fwhopr -fwhole-program FAIL: gcc.dg/torture/fp-int-convert-double.c -O1 execution test FAIL: gcc.dg/torture/fp-int-convert-long-double.c -O1 execution test FAIL: gcc.dg/torture/fp-int-convert-timode.c -O1 execution test FAIL: gcc.dg/torture/pr43879_1.c -O2 -fwhopr execution test FAIL: gcc.dg/tree-ssa/pr42585.c scan-tree-dump-times optimized "struct _fat_ptr _ans" 0 FAIL: gcc.dg/tree-ssa/pr42585.c scan-tree-dump-times optimized "struct _fat_ptr _T2" 0 FAIL: gcc.dg/vect/pr43430-1.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/vect-35.c scan-tree-dump-times vect "vectorized 2 loops" 1 FAIL: gcc.dg/vect/vect-peel-3.c scan-tree-dump-times vect "Vectorizing an unaligned access" 1 FAIL: gcc.dg/vect/vect-peel-4.c scan-tree-dump-times vect "Vectorizing an unaligned access" 1 FAIL: gcc.dg/vect/slp-reduc-3.c scan-tree-dump-times vect "vectorizing stmts using SLP" 1 FAIL: gcc.dg/vect/slp-reduc-6.c scan-tree-dump-times vect "vectorized 1 loops" 2 > > > Bernd
Index: gcc/optabs.c =================================================================== --- gcc/optabs.c (revision 162526) +++ gcc/optabs.c (working copy) @@ -511,14 +511,20 @@ oprnd0 = ops->op0; tmode0 = TYPE_MODE (TREE_TYPE (oprnd0)); - widen_pattern_optab = - optab_for_tree_code (ops->code, TREE_TYPE (oprnd0), optab_default); if (ops->code == WIDEN_MULT_PLUS_EXPR || ops->code == WIDEN_MULT_MINUS_EXPR) - icode = (int) optab_handler (widen_pattern_optab, + { + widen_pattern_optab = + optab_for_tree_code (ops->code, TREE_TYPE (ops->op2), optab_default); + icode = (int) optab_handler (widen_pattern_optab, TYPE_MODE (TREE_TYPE (ops->op2))); + } else - icode = (int) optab_handler (widen_pattern_optab, tmode0); + { + widen_pattern_optab = + optab_for_tree_code (ops->code, TREE_TYPE (oprnd0), optab_default); + icode = (int) optab_handler (widen_pattern_optab, tmode0); + } gcc_assert (icode != CODE_FOR_nothing);