Bug 56365 - [5 Regression] Missed opportunities for smin/smax standard name patterns when compiling as C++
Summary: [5 Regression] Missed opportunities for smin/smax standard name patterns when...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.8.0
: P2 normal
Target Milestone: 6.0
Assignee: Richard Biener
URL:
Keywords: missed-optimization
Depends on:
Blocks:
 
Reported: 2013-02-17 15:27 UTC by Oleg Endo
Modified: 2017-10-10 14:56 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.3.6, 6.0
Known to fail: 4.7.3, 5.3.0, 5.5.0
Last reconfirmed: 2013-02-18 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Endo 2013-02-17 15:27:22 UTC
While working on a patch for PR 55303 to add signed / unsigned clipping insns for the SH2A target, I've noticed the following (tested with -O2 on 196091 for SH and ARM cross configs):

int test_03 (int a, int b)
{
  int r = a + b;
  if (r > 127)
    r = 127;
  else if (r < -128)
    r = -128;
  return r;
}

This will utilize smin / smax standard name patterns.

The following equivalent (if I'm not mistaken), however:

static inline int min (int a, int b) { return a < b ? a : b; }
static inline int max (int a, int b) { return a < b ? b : a; }

int test_04 (int a, int b)
{
  return max (-128, min (127, a));
}

will not expand to smin / smax patterns.


Another case is:

int test_05 (int a)
{
  if (127 <= a)
    a = 127;
  else if (a <= -128)
    a = -128;
  return a;
}

For integers this could also be done with smin / smax, but it isn't.
Comment 1 Richard Biener 2013-02-18 10:58:13 UTC
I see, at -O2, on x86_64 in 070t.phiopt:

test_04 (int a, int b)
{
  int D.1744;
  int D.1741;
  int _3;
  int _4;

  <bb 2>:
  _3 = MIN_EXPR <a_1(D), 127>;
  _4 = MAX_EXPR <_3, -128>;
  return _4;

}

for the other cases you run into the issue that the tree-level phiopt
can be confused by phi-merging:

test_05 (int a)
{
  <bb 2>:
  if (a_2(D) > 126)
    goto <bb 5>;
  else
    goto <bb 3>;

  <bb 3>:
  if (a_2(D) < -127)
    goto <bb 5>;
  else
    goto <bb 4>;

  <bb 4>:

  <bb 5>:
  # a_1 = PHI <127(2), a_2(D)(4), -128(3)>
  return a_1;
Comment 2 Oleg Endo 2013-02-18 20:04:40 UTC
(In reply to comment #1)
> I see, at -O2, on x86_64 in 070t.phiopt:
> 
> test_04 (int a, int b)
> {
>   int D.1744;
>   int D.1741;
>   int _3;
>   int _4;
> 
>   <bb 2>:
>   _3 = MIN_EXPR <a_1(D), 127>;
>   _4 = MAX_EXPR <_3, -128>;
>   return _4;
> 
> }

Ah yes, now I see that here, too.  I don't know where or how I was looking, sorry.
Comment 3 Oleg Endo 2014-05-10 12:52:04 UTC
(In reply to Oleg Endo from comment #2)
> (In reply to comment #1)
> > I see, at -O2, on x86_64 in 070t.phiopt:
> > 
> > test_04 (int a, int b)
> > {
> >   int D.1744;
> >   int D.1741;
> >   int _3;
> >   int _4;
> > 
> >   <bb 2>:
> >   _3 = MIN_EXPR <a_1(D), 127>;
> >   _4 = MAX_EXPR <_3, -128>;
> >   return _4;
> > 
> > }
> 
> Ah yes, now I see that here, too.  I don't know where or how I was looking,
> sorry.

If compiled as C (-x c -std=gnu99) the min/max patterns work.  Compiling as C++ (-x c++ -std=c++11) does not work.  Probably that was reason for my original confusion -- usually I compile such small test snippets as C++ ...
Comment 4 Oleg Endo 2014-07-31 17:04:00 UTC
As of r213381 this problem still exists.

compiled as C 003t.original:

;; Function min (null)
;; enabled by -tree-original


{
  return MIN_EXPR <b, a>;
}


;; Function max (null)
;; enabled by -tree-original


{
  return MAX_EXPR <a, b>;
}


;; Function test_04 (null)
;; enabled by -tree-original


{
  return max (-128, min (127, a));
}


compiled as C++ 003t.original:


;; Function int min(int, int) (null)
;; enabled by -tree-original


return <retval> = a < b ? a : b;


;; Function int max(int, int) (null)
;; enabled by -tree-original


return <retval> = a < b ? b : a;


;; Function int test_04(int, int) (null)
;; enabled by -tree-original


<<cleanup_point return <retval> = max (-128, min (127, a))>>;
Comment 5 Oleg Endo 2014-10-17 10:14:45 UTC
Richard, any chance this might get fixed with the match-and-simplify branch?
Comment 6 Richard Biener 2014-10-17 11:41:22 UTC
No, that doesn't handle PHI nodes.  phiopt needs to be improved to handle
merged PHIs for this case.
Comment 7 Richard Biener 2016-03-14 09:30:22 UTC
Ok, so let me fix this for GCC 7.  It isn't realy "phi merging" but
phiopt being confused by our a <= -128 to a < -127 canonicalization.
Comment 8 Oleg Endo 2016-03-14 10:58:57 UTC
(In reply to Richard Biener from comment #7)
> Ok, so let me fix this for GCC 7.

Thanks :)
Comment 9 Richard Biener 2016-03-14 11:48:59 UTC
In the following testcase we used to do better with GCC 4.3 at least, handling
test_02 and test_04 completely and test_01 and test_03 half-way.  Compared
to GCC 5 which only handles test_04 completely and test_03 half-way and
test_01 and test_02 not at all.

So this is a regression probably caused by adding maybe_canonicalize_comparison
to comparison folding, PR26899.

int test_01 (int a)
{
  if (127 <= a)
    a = 127;
  else if (a <= -128)
    a = -128;
  return a;
}
int test_02 (int a)
{
  if (127 < a)
    a = 127;
  else if (a <= -128)
    a = -128;
  return a;
}
int test_03 (int a)
{
  if (127 <= a)
    a = 127;
  else if (a < -128)
    a = -128;
  return a;
}
int test_04 (int a)
{
  if (127 < a)
    a = 127;
  else if (a < -128)
    a = -128;
  return a;
}
Comment 10 Richard Biener 2016-03-14 14:51:08 UTC
Fixed on trunk sofar.  Not exactly planning to backport.
Comment 11 Richard Biener 2016-03-14 14:51:11 UTC
Author: rguenth
Date: Mon Mar 14 14:50:40 2016
New Revision: 234183

URL: https://gcc.gnu.org/viewcvs?rev=234183&root=gcc&view=rev
Log:
2016-03-14  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/56365
	* tree-ssa-phiopt.c (minmax_replacement): Handle alternate
	constants to compare against.

	* gcc.dg/tree-ssa/phi-opt-14.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-14.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-phiopt.c
Comment 12 Oleg Endo 2016-03-15 13:40:50 UTC
(In reply to Richard Biener from comment #10)
> Fixed on trunk sofar.  Not exactly planning to backport.

Hm yeah.  Although the patch applies and builds on the 5 branch without issues, test_04 from c#0 doesn't seem to work.
Comment 13 Richard Biener 2016-08-03 10:58:32 UTC
GCC 4.9 branch is being closed
Comment 14 Oleg Endo 2016-09-25 10:36:55 UTC
Richi, if you're not going to backport any patches, maybe close this one as fixed?
Comment 15 Richard Biener 2016-09-26 07:29:38 UTC
Keeping it open as it is a regression and to mark the last GCC 5 release as known-to-fail.
Comment 16 Jakub Jelinek 2017-10-10 13:36:13 UTC
GCC 5 branch has been closed, should be fixed in GCC 6 and later.