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