On Tue, 25 Jun 2019, Tamar Christina wrote:
> Adding some maintainers
>
> > -----Original Message-----
> > From: gcc-patches-ow...@gcc.gnu.org <gcc-patches-ow...@gcc.gnu.org> On
> > Behalf Of Tamar Christina
> > Sent: Tuesday, June 25, 2019 09:31
> > To: gcc-patches@gcc.gnu.org
> > Cc: nd <n...@arm.com>
> > Subject: [GCC][middle-end] Add rules to strip away unneeded type casts
> > in expressions (2nd patch)
> >
> > Hi All,
> >
> > This is an updated version of my GCC-9 patch which moves part of the
> > type conversion code from convert.c to match.pd because match.pd is
> > able to apply this transformation in the presence of intermediate
> > temporary variables.
> >
> > The previous patch was only regtested on aarch64-none-linux-gnu and I
> > hadn't done a regression on x86_64-pc-linux-gnu only a bootstrap. The
> > previous patch was approved
> >
> > here https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00116.html
> > but before committing I ran a x86_64-pc-linux-gnu regtest to be sure
> > and this showed an issue with a DFP test. I Have fixed this by
> > removing the offending convert.
> > The convert was just saying "keep the type as is" but match.pd looped
> > here as it thinks the match did something and would try other
> > patterns, causing it to match itself again.
> >
> > Instead when there's nothing to update, I just don't do anything.
> >
> > The second change was to merge this with the existing pattern for
> > integer conversion in order to silence a warning from match.pd which
> > though that the two patterns overlaps because their match conditions
> > are similar (they have different conditions inside the ifs but
> > match.pd doesn't check those of course.).
> >
> > Regtested and bootstrapped on aarch64-none-linux-gnu and x86_64-pc-
> > linux-gnu and no issues.
> >
> > Ok for trunk?
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?
+(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
(if (INTEGRAL_TYPE_P (type)
...
(with { tree arg0 = strip_float_extensions (@1);
...
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.
'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.christ...@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.christ...@arm.com>
> >
> > * gcc.dg/type-convert-var.c: New test.
> >
> > --
>
--
Richard Biener <rguent...@suse.de>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
