Summary: | [12 Regression] Wrong code with -O3 since r12-1841-g9fe9c45ae33a2df7 | ||
---|---|---|---|
Product: | gcc | Reporter: | Martin Liška <marxin> |
Component: | tree-optimization | Assignee: | 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 |
Mine. 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. Created attachment 51087 [details]
Patch which I am testing
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 And I can confirm the patch candidate fixes that. 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; } 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. Fixed. |
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)