Re: Move "X +- C1 CMP C2 to X CMP C2 -+ C1" to match.pd

[Marc Glisse <marc dot glisse at inria dot fr>]

On Tue, 26 Apr 2016, Richard Biener wrote:

> On Sun, Apr 24, 2016 at 7:14 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> > Hello,
> >
> > trying to move a first pattern from fold_comparison.
> >
> > I first tried without single_use. It brought the number of 'free' in
> > g++.dg/tree-ssa/pr61034.C down to 11, changed gcc.dg/sms-6.c to only 2 SMS
> > (I don't think the generated code was worse, maybe even better, but I don't
> > know ppc asm), broke Wstrict-overflow-18.c (the optimization moved from VRP
> > to CCP if I remember correctly), and caused IVOPTS to make a mess in
> > guality/pr54693-2.c (much longer code, and many <optimized> debug
> > variables). If someone wants to drop the single_use, they can work on that
> > after this patch is in.
> >
> > The conditions do not exactly match the ones in fold-const.c, but I guess
> > they are close. The warning in the constant case was missing in
> > fold_comparison, but present in VRP, so I had to add it not to regress.
> >
> > I don't think we were warning much from match.pd. I can't say that I am a
> > big fan of those strict overflow warnings, but I expect people would
> > complain if we just dropped the existing ones when moving the transforms to
> > match.pd?
>
> I just dropped them for patterns I moved (luckily we didn't have
> testcases sofar ;))
>
> If we really want to warn from match.pd then you should do the defer/undefer
> stuff in fold_stmt itself (same condition I guess) and defer/undefer without
> warning in gimple_fold_stmt_to_constant_1.

Moving it to fold_stmt_1 seems like a good idea, much better than putting 
it in forwprop. Looking at gimple_fold_stmt_to_constant_1 on the other 
hand, I have some concerns. If we do not warn for 
gimple_fold_stmt_to_constant_1, we are going to miss some warnings (I 
believe there is at least one testcase that will break, where VRP 
currently warns but CCP will come first). On the other hand, 
gimple_fold_stmt_to_constant_1 doesn't do much validation on its return 
value, each caller has their own idea of which results are acceptable, so 
it is only in the (many) callers that we can defer/undefer, or we might 
warn for transformations that we don't actually perform. CCP already has 
the defer/undefer calls around ccp_fold and thus 
gimple_fold_stmt_to_constant_1.

> So you actually ran into a testcase that expected the warning?

Several of them if I remember correctly...

> > I wanted to restrict the equality case to TYPE_OVERFLOW_WRAPS ||
> > TYPE_OVERFLOW_UNDEFINED, but that broke 20041114-1.c at -O1 (no strict
> > overflow), so I went with some kind of complement we use elsewhere.
>
> I think I prefer to move things 1:1 (unless sth regresses) and fix bugs in the
> fold-const.c variant as followup (possibly also adding testcases).

Well, yes, but things do indeed regress quite regularly when moving things 
1:1 :-( At least unless we add a big (if (GENERIC) ...) around the 
transformation.

> IMHO -fno-strict-overflow needs to go.  It has very wary designed semantics
> (ops neither wrap nor have undefined overflow).

Maybe make it an alias for -fwrapv?

--
Marc Glisse