On Mon, 10 Oct 2016, Marc Glisse wrote:
> On Fri, 7 Oct 2016, Richard Biener wrote:
>
> > On Thu, 6 Oct 2016, Marc Glisse wrote:
> >
> > > On Wed, 5 Oct 2016, Richard Biener wrote:
> > >
> > > > > The following will fix PR77826, the issue that in match.pd matching
> > > > > up two things uses operand_equal_p which is too lax about the type
> > > > > of the toplevel entity (at least for integer constants).
> > > > >
> > > > > Bootstrap / regtest pending on x86_64-unknown-linux-gnu.
> > > >
> > > > The following is what I have applied.
> > > >
> > > > Richard.
> > > >
> > > > 2016-10-05 Richard Biener <rguent...@suse.de>
> > > >
> > > > PR middle-end/77826
> > > > * genmatch.c (dt_operand::gen_match_op): Amend operand_equal_p
> > > > with types_match for GIMPLE code gen to handle type mismatched
> > > > constants properly.
> > >
> > > I don't understand the disparity between generic and gimple here. Why let
> > > (char)1 and (long)1 match in generic but not in gimple? And there are
> > > probably
> > > several transformations in match.pd that could do with an update if
> > > constants
> > > don't match anymore. Or did I misunderstand what the patch does?
> >
> > The disparity is mostly that with GENERIC unfolded trees such as (char)1
> > are a bug while in GIMPLE the fact that the match.pd machinery does
> > valueization makes those a "feature" we have to deal with. Originally
> > I've restricted GENERIC as well but with its types_match_p implementation
> > it resulted in too many missed matches.
>
> I shouldn't have written (long)1, I meant the fact that 1 (as a char constant)
> and 1 (as a long constant) will now be matching captures in generic and not in
> gimple. If we are going in the direction of not matching constants of
> different types, I'd rather do it consistently and update the patterns as
> needed to avoid the missed optimizations. The missed matches exist in gimple
> as well, and generic optimization seems less important than gimple to me.
Yes, I agree. I'll see on looking at the GENERIC fallout in more detail
(fixing patterns).
> An example that regressed at -O (looking at the .optimized dump)
>
> int f(int a, unsigned b){
> a &= 1;
> b &= 1;
> return a&b;
> }
Yeah, but it usually also shows a badly written pattern:
/* (X & Y) & (X & Z) -> (X & Y) & Z
(X | Y) | (X | Z) -> (X | Y) | Z */
(for op (bit_and bit_ior)
(simplify
(op:c (convert1?@3 (op:c@4 @0 @1)) (convert2?@5 (op:c@6 @0 @2)))
(if (tree_nop_conversion_p (type, TREE_TYPE (@1))
&& tree_nop_conversion_p (type, TREE_TYPE (@2)))
so how could we ever match the @0s when we have either of the two
conversions not present? We could only do this then facing constants
(due to using operand_equal_p). We for example fail to handle
(X & Y) & (unsigned) ((singed)X & Z)
> If we stick to the old behavior, maybe we could have some genmatch magic to
> help with the constant capture weirdness. With matching captures, we could
> select which operand (among those supposed to be equivalent) is actually
> captured more cleverly, either with an explicit marker, or by giving priority
> to the one that is not immediatly below convert? in the pattern.
This route is a difficult one to take -- what would be possible is to
capture a specific operand. Like allow one to write
(op (op @0@4 @1) (op @0@3 @2))
and thus actually have three "names" for @0. We have this internally
already when you write
(convert?@0 @1)
for the case where there is no conversion. @0 and @1 are the same
in this case.
Not sure if this helps enough cases.
I still think that having two things matched that are not really
the same is werid (and a possible source of errors as in, ICEs,
rather than missed optimizations).
> And if we move to stricter matching, maybe genmatch could be updated so
> convert could also match integer constants in some cases.
You mean when trying to match @0 ... (convert @0) and @0 is an INTEGER_CST
allow then (convert ...) case to match an INTEGER_CST of different type?
That's an interesting idea (not sure how to implement that though).
> > I agree that some transforms would need updates - I've actually tried
> > to implement a warning for genmatch whenever seeing a match with
> > (T)@0 but there isn't any good existing place to sneak that in.
>
>
> > > > * match.pd ((X /[ex] A) * A -> X): Properly handle converted
> > > > and constant A.
> > >
> > > This regressed
> > > int f(int*a,int*b){return 4*(int)(b-a);}
> >
> > This is because (int)(b-a) could be a truncation in which case
> > multiplying with 4 might not result in the same value as
> > b-a truncated(?). The comment before the unpatched patterns
> > said "sign-changing conversions" but nothign actually verified this.
> > Might be that truncations are indeed ok now that I think about it.
>
> 2015-05-22 Marc Glisse <marc.gli...@inria.fr>
>
> PR tree-optimization/63387
> * match.pd ((X /[ex] A) * A -> X): Remove unnecessary condition.
>
> Apparently I forgot to remove the comment at that time :-(
Ok. I'm now testing a patch to remove the restriction again (and adjust
the comment).
> > Btw, do you have a better suggestion as to how to handle the original
> > issue rather than not relying on operand_equal_p for constants?
>
> In previous cases, in order to get the right version of a matching capture, we
> used non-matching captures and an explicit call to operand_equal_p, for
> instance:
>
> /* X - (X / Y) * Y is the same as X % Y. */
> (simplify
> (minus (convert1? @2) (convert2? (mult:c (trunc_div @0 @1) @1)))
> /* We cannot use matching captures here, since in the case of
> constants we really want the type of @0, not @2. */
> (if (operand_equal_p (@0, @2, 0)
> && (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type)))
> (convert (trunc_mod @0 @1))))
>
> That seems to match the solutions that Jakub and you were discussing in the
> PR, something like it should work (we can discuss the exact code).
Yes. As more of those cases popped up I thought this isn't really
a viable way of going forward (to fix the ICEs). So we'll see the
above for re-instantiating missed optimizations instead.
> I don't know if it is better. The old behavior of matching captures with
> inconsistent types was confusing. A behavior with strict matching may
> complicate things (will we duplicate patterns, or write operand_equal_p
> explicitly to mimic the old behavior?). The recent inconsistency between
> generic and gimple doesnt appeal to me much...
Yeah, so as first I'm going to fix that disparity.
Thanks for the comments,
Richard.
