Bug 101256

Summary: [12 Regression] Wrong code with -O3 since r12-1841-g9fe9c45ae33a2df7
Product: gcc Reporter: Martin Liška <marxin>
Component: tree-optimizationAssignee: Andrew Pinski <pinskia>
Status: RESOLVED FIXED    
Severity: normal CC: pinskia
Priority: P3 Keywords: patch, wrong-code
Version: 12.0   
Target Milestone: 12.0   
URL: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574490.html
Host: Target:
Build: Known to work: 11.1.0
Known to fail: 12.0 Last reconfirmed: 2021-06-29 00:00:00
Bug Depends on:    
Bug Blocks: 103035    
Attachments: test-case
Patch which I am testing

Description Martin Liška 2021-06-29 11:18:05 UTC
Created attachment 51081 [details]
test-case

It's a yarpgen test case and it's not easily reducible.

However, with the following patch:


diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
index 93e7b4fd30e..6186b5533ca 100644
--- a/gcc/dbgcnt.def
+++ b/gcc/dbgcnt.def
@@ -158,6 +158,7 @@ DEBUG_COUNTER (dom_unreachable_edges)
 DEBUG_COUNTER (dse)
 DEBUG_COUNTER (dse1)
 DEBUG_COUNTER (dse2)
+DEBUG_COUNTER (edge_range)
 DEBUG_COUNTER (gcse2_delete)
 DEBUG_COUNTER (gimple_unroll)
 DEBUG_COUNTER (global_alloc_at_func)
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index ab12e85569d..21741504038 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -50,6 +50,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-fold.h"
 #include "internal-fn.h"
 #include "gimple-range.h"
+#include "dbgcnt.h"
 
 static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
 static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
@@ -413,7 +414,8 @@ replace_phi_edge_with_variable (basic_block cond_block,
       && EDGE_COUNT (gimple_bb (phi)->preds) == 2
       && INTEGRAL_TYPE_P (TREE_TYPE (phi_result))
       && !SSA_NAME_RANGE_INFO (new_tree)
-      && SSA_NAME_RANGE_INFO (phi_result))
+      && SSA_NAME_RANGE_INFO (phi_result)
+      && dbg_cnt(edge_range))
     duplicate_ssa_name_range_info (new_tree,
 				   SSA_NAME_RANGE_TYPE (phi_result),
 				   SSA_NAME_RANGE_INFO (phi_result));

One can reduce it to:

$ g++ driver.cpp -c && g++ func.cpp -O3 -c -fdbg-cnt=edge_range:52-52 && gcc func.o driver.o && ./a.out
***dbgcnt: lower limit 52 reached for edge_range.***
***dbgcnt: upper limit 52 reached for edge_range.***
a.out: driver.cpp:3609: void checksum(): Assertion `var_303 == (unsigned char)240' failed.
Aborted (core dumped)
Comment 1 Andrew Pinski 2021-06-29 17:28:36 UTC
Mine.
Comment 2 Andrew Pinski 2021-06-29 20:39:51 UTC
I have a fix but I don't have a testcase really.
What is happening is value_replacement is happening but since the ssa_name that is being used for replace_phi_edge_with_variable is not newly defined, we are adding the range info from something which was conditionally defined.

To be able to make sure we are doing the range for a newly defined name, we need to pass the sequence of statements that get moved/added before conditional.
Comment 3 Andrew Pinski 2021-06-30 00:37:16 UTC
Created attachment 51087 [details]
Patch which I am testing
Comment 4 Martin Liška 2021-07-02 06:56:34 UTC
I managed reducing a test-case:

$ cat driver.cpp 
#include <stdio.h>
#include <cassert>

template<class T> 
const T& max(const T& a, const T& b)
{
    return (a < b) ? b : a;
}

signed char var_5 = -128;
unsigned int var_11 = 2144479212U;
unsigned long long int arr [22];

void
__attribute__((noipa))
test(signed char var_5, unsigned var_11) {
  for (short i_61 = 0; i_61 < var_5 + 149; i_61 += 10000)
    arr[i_61] = max((signed char)0, var_5) ? max((signed char)1, var_5) : var_11;
}

int main() {
  for (size_t i_0 = 0; i_0 < 22; ++i_0) 
      arr [i_0] = 11834725929543695741ULL;

    test(var_5, var_11);
    assert(arr [0] == 2144479212ULL || arr [0]  == 11834725929543695741ULL);
    __builtin_printf ("OK\n");
}

$ g++ driver.cpp -O2 && ./a.out 
a.out: driver.cpp:26: int main(): Assertion `arr [0] == 2144479212ULL || arr [0] == 11834725929543695741ULL' failed.
Aborted (core dumped)
$ g++ driver.cpp && ./a.out 
OK
Comment 5 Martin Liška 2021-07-02 07:03:23 UTC
And I can confirm the patch candidate fixes that.
Comment 6 Andrew Pinski 2021-07-02 20:10:16 UTC
And here is a self-contained testcase:

template<class T> 
const T& max(const T& a, const T& b)
{
    return (a < b) ? b : a;
}

signed char var_5 = -128;
unsigned int var_11 = 2144479212U;
unsigned long long int arr [22];

void
__attribute__((noipa))
test(signed char var_5, unsigned var_11) {
  for (short i_61 = 0; i_61 < var_5 + 149; i_61 += 10000)
    arr[i_61] = max((signed char)0, var_5) ? max((signed char)1, var_5) : var_11;
}

int main() {
  for (size_t i_0 = 0; i_0 < 22; ++i_0) 
      arr [i_0] = 11834725929543695741ULL;

  test(var_5, var_11);
  if (arr [0] != 2144479212ULL && arr [0] != 11834725929543695741ULL)
    __builtin_abort ();
  return 0;
}
Comment 7 Andrew Pinski 2021-07-06 01:34:54 UTC
Updated patch:
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574490.html
Comment 8 GCC Commits 2021-07-06 07:34:20 UTC
The master branch has been updated by Andrew Pinski <pinskia@gcc.gnu.org>:

https://gcc.gnu.org/g:51abfb6a893c87dbf84a33009b6cd6dbd25d66f1

commit r12-2048-g51abfb6a893c87dbf84a33009b6cd6dbd25d66f1
Author: Andrew Pinski <apinski@marvell.com>
Date:   Tue Jun 29 14:30:34 2021 -0700

    Fix 101256: Wrong code due to range incorrect from PHI-OPT
    
    So the problem here is that replace_phi_edge_with_variable
    will copy range information to a already (not newly) defined
    ssa name.  This causes wrong code later on.
    This fixes the problem by require the new ssa name to
    be defined in the same bb as the conditional that is
    about to be deleted.
    
    OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
    
    Changes from v1:
    * this is a simplification of what was trying to be done before.
    
    gcc/ChangeLog:
    
            PR tree-optimization/101256
            * dbgcnt.def (phiopt_edge_range): New counter.
            * tree-ssa-phiopt.c (replace_phi_edge_with_variable):
            Check to make sure the new name is defined in the same
            bb as the conditional before duplicating range info.
            Also add debug counter.
    
    gcc/testsuite/ChangeLog:
    
            PR tree-optimization/101256
            * g++.dg/torture/pr101256.C: New test.
Comment 9 Andrew Pinski 2021-07-06 07:35:01 UTC
Fixed.