[Bug tree-optimization/66726] New: missed optimization, factor conversion out of COND_EXPR

[Bug tree-optimization/66726] New: missed optimization, factor conversion out of COND_EXPR

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

            Bug ID: 66726
           Summary: missed optimization, factor conversion out of
                    COND_EXPR
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: law at redhat dot com
  Target Milestone: ---

Given something like this:

   n = (short unsigned int) mode_size[(unsigned int) mode] * 8 <= 64 ? (int)
((short unsigned int) mode_size[(unsigned int) mode] * 8) : 64; 

Note the (int) cast on the true arm of the COND_EXPR.  As a result of that
cast, the transformations to turn a COND_EXPR into a MIN/MAX expression in
fold-const.c will not trigger.

This could be fixed by a patch to match.pd, but the pattern would only really
be applicable to GENERIC and thus isn't considered a good design match for
match.pd.

We could extend fold-const.c to catch this case, but that would really only
help GENERIC as well.

The best option it seems would be to catch this in phi-opt which would also
take us a step closer to handling pr45397.

Testcase:

/* { dg-do compile } */
/* { dg-options "-O2 -fdump-tree-original" } */


extern unsigned short mode_size[];
int
oof (int mode)
{
  return (64 < mode_size[mode] ? 64 : mode_size[mode]);
}

/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "original" } } */

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[kugan at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #1 from kugan at gcc dot gnu.org ---
Created attachment 35888
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35888&action=edit
untested prototype patch

Hi Jeff,

Here is a patch (without debug dumps and not tesetd fully). Is this along what
you had in mind?

Thanks,
Kugan

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Comment on attachment 35888
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35888
untested prototype patch

I thought pre does this already and that seems like a better place than
phi-opt. Oh wait pre does the opposite and really you should handle more than
two arguments to phis.

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[kugan at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #3 from kugan at gcc dot gnu.org ---
> really you should handle more
> than two arguments to phis.
I am not sure how we can handle phi stmt with more than two arguments here. Any
hints please?

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to kugan from comment #3)
> > really you should handle more
> > than two arguments to phis.
> I am not sure how we can handle phi stmt with more than two arguments here.
> Any hints please?

Yes they are all interger constants but one of them. Also I think you do have
another bug where the conversion is an extension the original interger has to
fit in the new type. That is if you converting from char to int.

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[kugan at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #5 from kugan at gcc dot gnu.org ---
(In reply to Andrew Pinski from comment #4)
> (In reply to kugan from comment #3)
> > > really you should handle more
> > > than two arguments to phis.
> > I am not sure how we can handle phi stmt with more than two arguments here.
> > Any hints please?
> 
> Yes they are all interger constants but one of them. Also I think you do
> have another bug where the conversion is an extension the original interger
> has to fit in the new type. That is if you converting from char to int.

Thanks for that. Yes, I fixed this and another during testing. I will post it
after full regression testing.

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #6 from Jeffrey A. Law <law at redhat dot com> ---
WRT c#2.  PRE will try to optimize away a runtime redundant expression.  In the
cases I'm looking at, there is only a single runtime evaluation.   But that
evaluation can be sunk from a set of predecessors into a single successor.

It's more like tail merging than PRE.

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #7 from Jeffrey A. Law <law at redhat dot com> ---
The other thing to keep in mind, sinking of this nature ought to be applicable
to other unary ops and cases where we have multiple PHI args that are
SSA_NAMEs.    It shouldn't be structurally limited to just conversions where
one arg is an SSA_NAME and the other a constant.

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #9 from Andreas Schwab <schwab@linux-m68k.org> ---
On m68k:

FAIL: gcc.dg/pr46309.c scan-tree-dump-times reassoc1 "Optimizing range tests
a_[0-9]*.D. -.1, 1. and -.3, 3.[\n\r]* into" 1
FAIL: gcc.dg/pr46309.c scan-tree-dump-times reassoc1 "Optimizing range tests
a_[0-9]*.D. -.1, 1. and -.2, 2.[\n\r]* into" 1
FAIL: gcc.dg/pr46309.c scan-tree-dump-times reassoc1 "Optimizing range tests
a_[0-9]*.D. -.0, 31. and -.64, 95.[\n\r]* into" 2

$ gcc/xgcc -Bgcc/ ../gcc/testsuite/gcc.dg/pr46309.c -O2
-fdump-tree-reassoc-details -S -o pr46309.s
$ grep -e "Optimizing range tests" -e into pr46309.c.*.reassoc1
Optimizing range tests a_2(D) -[1, 1] and -[2, 2] and -[3, 3] and -[4, 4]
 into (unsigned int) a_2(D) + 4294967295 > 3
Optimizing range tests a_2(D) -[1, 1] and -[2, 2] and -[3, 3] and -[4, 4]
 into (unsigned int) a_2(D) + 4294967295 > 3
Optimizing range tests a_2(D) -[0, 31] and -[64, 95]
 into (a_2(D) & 4294967231) > 31
Optimizing range tests a_2(D) -[128, 159] and -[192, 223]
 into (a_2(D) & 4294967231) + 4294967168 > 31

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #10 from Jeffrey A. Law <law at redhat dot com> ---
Sinking the cast changes the form of the range tests into one that
tree-ssa-reassoc isn't prepared to handle.   Sadly the form presented with the
cast sunk is *simpler* than the original.

I'm testing a bit of a hack to avoid the sinking of the cast in cases where
doing so is less likely to be a win.

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[kugan at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #11 from kugan at gcc dot gnu.org ---
Thanks for reporting. This test case is valid for targets that has branch cost
greater than 1.

One way to handle this is by disabling this for convections involving bool that
are part of branch (?)

I think the real fix is to make tree-ssa-reassoc handle this? Looking into it.

[Bug tree-optimization/66726] missed optimization, factor conversion out of COND_EXPR

[kugan at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66726

--- Comment #12 from kugan at gcc dot gnu.org ---
Created attachment 35976
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35976&action=edit
patch for tree-ssa-reassoc

Here is a prototype patch (to fix comment 9) that makes tree-ssa-reassoc
understand after sinking the casts. Not fully tested yet.