Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)

[Richard Biener <rguenther at suse dot de>]

On Tue, 2 Jul 2019, Tamar Christina wrote:

> 
> Hi All,
> 
> Here's an updated patch with the changes processed from the previous review.
> 
> I've bootstrapped and regtested on aarch64-none-linux-gnu and x86_64-pc-linux-gnu and no issues.
> 
> Ok for trunk?

+     (if (TYPE_OVERFLOW_WRAPS (TREE_TYPE (@1)))
+       (op @1 (convert @2))
+       (with { tree utype = unsigned_type_for (TREE_TYPE (@1)); }
+       (convert (op (convert:utype @1)

indenting is off, one space more for (convert (it's inside the with)

+                   (convert:utype @2)))))
+      (with { tree arg0 = strip_float_extensions (@1);

indenting is off here, one space less for the (with please.

you'll run into strip_float_extensions for integer types as well so
please move the FLOAT_TYPE_P (type) check before the with like

     (if (FLOAT_TYPE_P (type)
          && DECIMAL_FLOAT_TYPE_P (TREE_TPE (@0)) == DECIMAL_FLOAT_TYPE_P 
(type))
      (with { tree arg0 = strip_float_extensions (@1);

+         (if ((newtype == dfloat32_type_node
+               || newtype == dfloat64_type_node
+               || newtype == dfloat128_type_node)
+             && newtype == type)
+           (convert:newtype (op (convert:newtype @1) (convert:newtype 
@2)))

I think you want to elide the outermost convert:newtype here and use

            (op (convert:newtype @1) (convert:newtype @2))

newtype == type check you also want to write types_match_p (newtype, type)

+             (if (TYPE_PRECISION (newtype) < TYPE_PRECISION (itype)
+                  && (flag_unsafe_math_optimizations
+                      || (TYPE_PRECISION (newtype) == TYPE_PRECISION 
(type)
+                          && real_can_shorten_arithmetic (TYPE_MODE 
(itype),
+                                                          TYPE_MODE 
(type))
+                          && !excess_precision_type (newtype))))
+                (convert:newtype (op (convert:newtype @1)
+                                     (convert:newtype @2)))

here the outermost convert looks bogus - you need to build an
expression of type 'type' thus

   (convert:type (op (convert:newtype @1) (convert:newtype @2)))

I think you also want to avoid endless recursion by adding a

             && !types_match_p (itype, newtype)

since in that case you've re-created the original expression.

OK with those changes.

Thanks,
Richard.

> Thanks,
> Tamar
> 
> The 07/02/2019 11:20, Richard Biener wrote:
> > On Tue, 2 Jul 2019, Tamar Christina wrote:
> > 
> > > Hi Richi,
> > > 
> > > The 06/25/2019 10:02, Richard Biener wrote:
> > > > 
> > > > This looks like a literal 1:1 translation plus merging with the
> > > > existing pattern around integers.  You changed
> > > > (op:s@0 (convert@3 @1) (convert?@4 @2)) to
> > > > (op:s@0 (convert1?@3 @1) (convert2?@4 @2)) where this now also
> > > > matches if there are no inner conversions at all - was that a
> > > > desired change or did you merely want to catch when the first
> > > > operand is not a conversion but the second is, possibly only
> > > > for the RDIV_EXPR case?
> > > >
> > > 
> > > Yes, the ? ? is for the RDIV case, I really only want (c a) `op` (c b),
> > > a `op` (c b) and (c a) `op` b.  But didn't find a way to do this.
> > 
> > One way would be to do
> > 
> >  (simplify
> >   (convert (op:sc@0 (convert @1) (convert? @2)))
> > 
> > but that doesn't work for RDIV.  Using :C is tempting but you
> > do not get to know the original operand order which you of
> > course need.  So I guess the way you do it is fine - you
> > could guard all of the code with a few types_match () checks
> > but I'm not sure it is worth the trouble.
> > 
> > Richard.
> > 
> > > The only thing I can think of that gets this is without overmatching is
> > > to either duplicate the code or move this back to a C helper function and
> > > call that from match.pd.  But I was hoping to have it all in match.pd
> > > instead of hiding it away in a C call.
> > > 
> > > Do you have a better way of doing it or a preference to an approach?
> > >  
> > > > +(for op (plus minus mult rdiv)
> > > > + (simplify
> > > > +   (convert (op:s@0 (convert1?@3 @1) (convert2?@4 @2)))
> > > > +   (with { tree arg0 = strip_float_extensions (@1);
> > > > +          tree arg1 = strip_float_extensions (@2);
> > > > +          tree itype = TREE_TYPE (@0);
> > > > 
> > > > you now unconditionally call strip_float_extensions on each operand
> > > > even for the integer case, please sink stuff only used in one
> > > > case arm.  I guess keeping the integer case first via
> > > > 
> > > 
> > > Done, Initially didn't think it would be an issue since I don't use the value it
> > > creates in the integer case. But I re-ordered it.
> > >  
> > > > should work (with the 'with' being in the ifs else position).
> > > > 
> > > > +      (if (code == REAL_TYPE)
> > > > +       /* Ignore the conversion if we don't need to store intermediate
> > > > +          results and neither type is a decimal float.  */
> > > > +         (if (!(flag_float_store
> > > > +              || DECIMAL_FLOAT_TYPE_P (type)
> > > > +              || DECIMAL_FLOAT_TYPE_P (itype))
> > > > +             && types_match (ty1, ty2))
> > > > +           (convert (op (convert:ty1 @1) (convert:ty2 @2)))))
> > > > 
> > > > this looks prone to the same recursion issue you described above.
> > > 
> > > It's to break the recursion when you don't match anything. Indeed don't need it if
> > > I change the match condition above.
> > > Thanks,
> > > Tamar
> > > > 
> > > > 'code' is used exactly once, using SCALAR_FLOAT_TYPE_P (itype)
> > > > in the above test would be clearer.  Also both ifs can be combined.
> > > > The snipped above also doesn't appear in the convert.c code you
> > > > remove and the original one is
> > > > 
> > > >   switch (TREE_CODE (TREE_TYPE (expr)))
> > > >     {
> > > >     case REAL_TYPE:
> > > >       /* Ignore the conversion if we don't need to store intermediate
> > > >          results and neither type is a decimal float.  */
> > > >       return build1_loc (loc,
> > > >                          (flag_float_store
> > > >                           || DECIMAL_FLOAT_TYPE_P (type)
> > > >                           || DECIMAL_FLOAT_TYPE_P (itype))
> > > >                          ? CONVERT_EXPR : NOP_EXPR, type, expr);
> > > > 
> > > > which as far as I can see doesn't do anything besides
> > > > exchanging CONVERT_EXPR for NOP_EXPR which is a noop to the IL.
> > > > So it appears this shouldn't be moved to match.pd at all?
> > > > It's also not a 1:1 move since you are changing 'expr'.
> > > > 
> > > > Thanks,
> > > > Richard.
> > > > 
> > > > > > Thanks,
> > > > > > Tamar
> > > > > > 
> > > > > > Concretely it makes both these cases behave the same
> > > > > > 
> > > > > >   float e = (float)a * (float)b;
> > > > > >   *c = (_Float16)e;
> > > > > > 
> > > > > > and
> > > > > > 
> > > > > >   *c = (_Float16)((float)a * (float)b);
> > > > > > 
> > > > > > Thanks,
> > > > > > Tamar
> > > > > > 
> > > > > > gcc/ChangeLog:
> > > > > > 
> > > > > > 2019-06-25  Tamar Christina  <tamar.christina@arm.com>
> > > > > > 
> > > > > > 	* convert.c (convert_to_real_1): Move part of conversion code...
> > > > > > 	* match.pd: ...To here.
> > > > > > 
> > > > > > gcc/testsuite/ChangeLog:
> > > > > > 
> > > > > > 2019-06-25  Tamar Christina  <tamar.christina@arm.com>
> > > > > > 
> > > > > > 	* gcc.dg/type-convert-var.c: New test.
> > > > > > 
> > > > > > --
> > > > > 
> > > > 
> > > > -- 
> > > > Richard Biener <rguenther@suse.de>
> > > > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
> > > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
> > > 
> > > 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)