[Bug tree-optimization/104987] New: [12 Regression] Recent change causing vrp13.c regressions on several targets

[Bug tree-optimization/104987] New: [12 Regression] Recent change causing vrp13.c regressions on several targets

[law at gcc dot gnu.org]

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

            Bug ID: 104987
           Summary: [12 Regression] Recent change causing vrp13.c
                    regressions on several targets
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: law at gcc dot gnu.org
  Target Milestone: ---

This change:
commit 8db155ddf8cec9e31f0a4b8d80cc67db2c7a26f9 (refs/bisect/bad)
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Thu Mar 17 10:52:10 2022 -0400

    Always use dominators in the cache when available.

    This patch adjusts range_from_dom to follow the dominator tree through the
    cache until value is found, then apply any outgoing ranges encountered
    along the way.  This reduces the amount of cache storage required.

            PR tree-optimization/102943
            * gimple-range-cache.cc (ranger_cache::range_from_dom): Find range
via
            dominators and apply intermediary outgoing edge ranges.

Is causing gcc.dg/tree-ssa/vrp13.c to fail on a couple targets (iq2000-elf,
v850e-elf).

It looks like we're mis-compiling foo_mult.

Here's the reduced testcase:

/* { dg-do run }  */
/* { dg-options -O2 }  */

extern void abort (void);
extern void link_error (void);
int
foo_mult (int i, int j)
{
  int k;

  /* [-20, -10] * [2, 10] should give [-200, -20].  */
  if (i >= -20)
    if (i <= -10)
      if (j >= 2)
        if (j <= 10)
          {
            k = i * j;
            if (k < -200)
              link_error ();
            if (k > -20)
              link_error ();

            return k;
          }

  /* [-20, -10] * [-10, -2] should give [20, 200].  */
  if (i >= -20)
    if (i <= -10)
      if (j >= -10)
        if (j <= -2)
          {
            k = i * j;
            if (k < 20)
              link_error ();
            if (k > 200)
              link_error ();

            return k;
          }

  /* [-20, 10] * [2, 10] should give [-200, 100].  */
  if (i >= -20)
    if (i <= 10)
      if (j >= 2)
        if (j <= 10)
          {
            k = i * j;
            if (k < -200)
              link_error ();
            if (k > 100)
              link_error ();

            return k;
          }

  /* [-20, 10] * [-10, -2] should give [-100, 200].  */
  if (i >= -20)
    if (i <= 10)
      if (j >= -10)
        if (j <= -2)
          {
            k = i * j;
            if (k < -100)
              link_error ();
            if (k > 200)
              link_error ();

            return k;
          }

  /* [10, 20] * [2, 10] should give [20, 200].  */
  if (i >= 10)
    if (i <= 20)
      if (j >= 2)
        if (j <= 10)
          {
            k = i * j;
            if (k < 20)
              link_error ();
            if (k > 200)
              link_error ();

            return k;
          }

  /* [10, 20] * [-10, -2] should give [-200, -20].  */
  if (i >= 10)
    if (i <= 20)
      if (j >= -10)
        if (j <= -2)
          {
            k = i * j;
            if (k < -200)
              link_error ();
            if (k > -20)
              link_error ();

            return k;
          }

  abort ();
}

int
main()
{
  if (foo_mult (10, -2) != -20)
    abort ();

  return 0;
}

The symptom on the v850 is we get the sign wrong on the multiplication.  I
haven't looked into what goes wrong on iq2000-elf.

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |iq2000-elf, v850e-elf
           Keywords|                            |wrong-code
            Version|unknown                     |12.0
   Target Milestone|---                         |12.0

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[amacleod at redhat dot com]

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

--- Comment #1 from Andrew Macleod <amacleod at redhat dot com> ---
This is all very strange.  So its a runtime error on those targets?

The code we produce is slightly different, it happens to expose certain
limitations with picking up ranges via dominators when there are multiple paths
from the dominator, all of which happen to carry range information.  But it
shouldn't be a correctness thing

And we end up not inlining and doing different things in ifcombine for some
reason... but I cannot find any place where a transformation is incorrect.

If I follow the IL thru from the .optimized listing:    

_1 = foo_mult (10, -2);

and tracing thru with   i = 10,  j = -2

int foo_mult (int i, int j)
{
  int k;
  int _1;
  unsigned int _31;
  unsigned int _35;

  <bb 2> [local count: 1073741821]:
  if (i_5(D) >= -20)
    goto <bb 3>; [100.00%]          2->3          

  <bb 3> [local count: 1073741821]:
  if (i_5(D) < -9)
    goto <bb 4>; [50.00%]
  else
    goto <bb 9>; [50.00%]             3->9

<bb 9> [local count: 330972458]:
  if (i_5(D) <= 10)
    goto <bb 10>; [50.00%]            9->10
  else
    goto <bb 12>; [50.00%]

  <bb 10> [local count: 76561739]:
  if (j_6(D) > 1)
    goto <bb 11>; [84.14%]
  else
    goto <bb 14>; [15.86%]           10->14

  <bb 14> [local count: 160296875]:
  _31 = (unsigned int) j_6(D);          _31 = unsigned (-2)
  _35 = _31 + 10;                        _35 = -2 + 10 == 8
  if (_35 <= 8)
    goto <bb 8>; [79.40%]           14->8

  <bb 8> [local count: 414017807]:
  k_7 = i_5(D) * j_6(D);             k_7 = 10 * -2
  goto <bb 19>; [100.00%]           8->19

 <bb 19> [local count: 1073741824]:
  # _1 = PHI <k_7(8), k_14(17)>
  return _1;


_1 should be returning k_7 which should be -20

It seems fine at the .optimized listing?

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[law at gcc dot gnu.org]

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

--- Comment #2 from Jeffrey A. Law <law at gcc dot gnu.org> ---
It does trigger execution failures on those targets.

I guess it's possible it's merely exposing existing bugs on those targets.  If
we were inlining before, we may have collapsed the test away completely.  Let
me do some poking over here.

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[amacleod at redhat dot com]

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

--- Comment #3 from Andrew Macleod <amacleod at redhat dot com> ---
FWIW, if I remove the ifcombine pass, then I get the identical IL on x86_64 in
the .optimized pass, and it executes as expected...

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
If I use --param=logical-op-non-short-circuit=0 -O2 on x86_64-linux, I get
exactly the same -fdump-tree-optimized-alias dump as on v850-elf and the
testcase passes and the optimized dump looks sane.
I admit I don't have v850-elf binutils nor sim around, but the assembly I get
is:
_foo_mult:
.LFB0:
        prepare {r31}, 0
        movea -20,r0,r10
        cmp r10,r6
        blt .L2
        cmp -9,r6
        blt .L14
        cmp 10,r6
        bgt .L7
        cmp 1,r7
        bgt .L4
        addi 10,r7,r10
        cmp 8,r10
        bh .L2
.L12:
        mov r6,r10
        mul r7,r10,r0
        dispose 0 {r31}, r31
...
.L2:
        jarl _abort, r31

When this is called, r6 is 10 and r7 is -2, so I think none of the conditional
jumps should branch away,
because r6 >= -20, r6 >= -9, r6 <= 10, r7 <= 1 and so it will do r10 = r7 + 10
(thus set r10 to 8) and r10 <= 8U,
so I think it should fall through into .L12 and multiply + return.

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And it is unclear to me how it could get the sign of the multiplication wrong,
it is a normal SImode mul which multiplies the values of SImode r6 and r7
registers.
As it isn't hipart multiply or something similar, it doesn't care about sign,
it should be the same.
Or are you getting a different assembly?

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[law at gcc dot gnu.org]

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

--- Comment #6 from Jeffrey A. Law <law at gcc dot gnu.org> ---
For the v850 at least, I'm starting to think this is a simulator bug.  In
particular the simulator code doesn't look safe on a 64bit host for a signed
input to the MUL instruction.

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[law at gcc dot gnu.org]

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

--- Comment #7 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Highly confident this is a simulator bug for the v850.  I hiaven't looked at
iq2000-elf yet, but I wouldn't be surprised if that turns out to be something
similar.

[Bug tree-optimization/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|iq2000-elf, v850e-elf       |iq2000-elf
           Priority|P3                          |P4

--- Comment #8 from Jeffrey A. Law <law at gcc dot gnu.org> ---
V850 simulator fix has been posted to the binutils list.

I've never really hacked the iq2000, but from the looks of things I think it's
mis-compiling mulsi3 in libgcc.  In particular, I don't think it's handling
delay slots properly for the bbi instruction.  reorg has tagged it as a
nullified-if-false branch, but it appears that we're using the wrong form at
assembly time and the instruction in the delay slot always executes.

So P4 as this appears to be an iq2000 specific issue.

[Bug target/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:0364465e3708249ece810ca5d65164552595538c

commit r12-7974-g0364465e3708249ece810ca5d65164552595538c
Author: Jeff Law <jeffreyalaw@gmail.com>
Date:   Sun Apr 3 18:22:13 2022 -0400

    [committed][PR target/104987] Avoid "likely" forms of bbi[n] on iq2000.

    The iq2000 port is mis-compiling its mulsi3 libgcc2 function.

    AFAICT, the iq2000 has delay slots and can use "branch-likely" forms of
conditional branches to annul-false the slot.   There's a support routine that
handles creation of the  likely form.  However, that routine is not used by the
bbi[n] instructions.

    If I manually add the likely extension to the bbi[b] instructions, the
assembler complains  After a fair amount of digging it appears that the likely
forms of bbi[n] are only supported on the IQ10 variant.

    Given this is a dead processor and has been so for a while it seems
reasonable to just disallow annul-false slots for the bbi[n] instructions
rather than try to handle them just for the IQ10 (which we don't have real
support for anyway).

    This (of course) fixes the vrp13 regression.  But it also fixes nearly a
thousand execution test failures in the testsuite (Yow!).

    gcc/
            PR target/104987
            * config/iq2000/iq2000.md (bbi): New attribute,  default to no.
            (delay slot descripts): Use different delay slot description when
            the insn as the "bbi" attribute.
            (bbi, bbin patterns): Set the bbi attribute to yes.

[Bug target/104987] [12 Regression] Recent change causing vrp13.c regressions on several targets

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #10 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Fixed on the trunk.