Patchwork Extend widening_mul pass to handle fixed-point types

login
register
mail settings
Submitter Ramana Radhakrishnan
Date July 27, 2010, 12:18 p.m.
Message ID <1280233130.18689.17.camel@e102325-lin.cambridge.arm.com>
Download mbox | patch
Permalink /patch/59998/
State New
Headers show

Comments

Ramana Radhakrishnan - July 27, 2010, 12:18 p.m.
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 ? 

cheers
Ramana

2010-07-27  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

        PR bootstrap/45067
        * optabs.c (expand_widen_pattern_expr): Initialize
widen_pattern_optab correctly for WIDEN_MULT_PLUS_EXPR and
WIDEN_MULT_MINUS_EXPR.

   xmode0 = insn_data[icode].operand[1].mode;
 



> 
> 
> Bernd
Bernd Schmidt - July 27, 2010, 3:51 p.m.
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
Ramana Radhakrishnan - July 28, 2010, 3:40 p.m.
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

Patch

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);