Bug 108639 - [13 Regression] ICE on valid code at -O1 and above: in decompose, at wide-int.h:984 since r13-5578
Summary: [13 Regression] ICE on valid code at -O1 and above: in decompose, at wide-int...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 13.0
: P1 normal
Target Milestone: 13.0
Assignee: Jakub Jelinek
URL:
Keywords:
: 108638 108668 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-02-02 14:29 UTC by Zhendong Su
Modified: 2023-02-03 22:56 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2023-02-02 00:00:00


Attachments
gcc13-pr108639.patch (795 bytes, patch)
2023-02-02 15:31 UTC, Jakub Jelinek
Details | Diff
untested patch for irange::operator== (1.25 KB, patch)
2023-02-02 17:14 UTC, Aldy Hernandez
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhendong Su 2023-02-02 14:29:06 UTC
It appears to be a recent regression.

Compiler Explorer: https://godbolt.org/z/aE6Tssrq6

[538] % gcctk -v
Using built-in specs.
COLLECT_GCC=gcctk
COLLECT_LTO_WRAPPER=/local/suz-local/software/local/gcc-trunk/libexec/gcc/x86_64-pc-linux-gnu/13.0.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-trunk/configure --disable-bootstrap --enable-checking=yes --prefix=/local/suz-local/software/local/gcc-trunk --enable-sanitizers --enable-languages=c,c++ --disable-werror --enable-multilib --with-system-zlib
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 13.0.1 20230202 (experimental) [master r13-5642-g66d700af5bb] (GCC)
[539] %
[539] % gcctk -O1 small.c
small.c: In function ‘main’:
small.c:3:18: warning: division by zero [-Wdiv-by-zero]
    3 |   a = a ? 0 || 0 % 0 : 0;
      |                  ^
during GIMPLE pass: dom
small.c:2:5: internal compiler error: in decompose, at wide-int.h:984
    2 | int main() {
      |     ^~~~
0x7b6a92 wi::int_traits<generic_wide_int<wide_int_ref_storage<false, false> > >::decompose(long*, unsigned int, generic_wide_int<wide_int_ref_storage<false, false> > const&)
        ../../gcc-trunk/gcc/wide-int.h:984
0x7b7838 wi::int_traits<generic_wide_int<wide_int_storage> >::decompose(long*, unsigned int, generic_wide_int<wide_int_storage> const&)
        ../../gcc-trunk/gcc/value-range.h:940
0x7b7838 wide_int_ref_storage<true, false>::wide_int_ref_storage<generic_wide_int<wide_int_storage> >(generic_wide_int<wide_int_storage> const&, unsigned int)
        ../../gcc-trunk/gcc/wide-int.h:1034
0x7b7838 generic_wide_int<wide_int_ref_storage<true, false> >::generic_wide_int<generic_wide_int<wide_int_storage> >(generic_wide_int<wide_int_storage> const&, unsigned int)
        ../../gcc-trunk/gcc/wide-int.h:790
0x7b7838 bool wi::eq_p<generic_wide_int<wide_int_storage>, generic_wide_int<wide_int_storage> >(generic_wide_int<wide_int_storage> const&, generic_wide_int<wide_int_storage> const&)
        ../../gcc-trunk/gcc/wide-int.h:1873
0x7b7838 wi::binary_traits<generic_wide_int<wide_int_storage>, generic_wide_int<wide_int_storage>, wi::int_traits<generic_wide_int<wide_int_storage> >::precision_type, wi::int_traits<generic_wide_int<wide_int_storage> >::precision_type>::predicate_result operator==<generic_wide_int<wide_int_storage>, generic_wide_int<wide_int_storage> >(generic_wide_int<wide_int_storage> const&, generic_wide_int<wide_int_storage> const&)
        ../../gcc-trunk/gcc/wide-int.h:3307
0x7b7838 irange::operator==(irange const&) const
        ../../gcc-trunk/gcc/value-range.cc:1297
0x7b7838 irange::operator==(irange const&) const
        ../../gcc-trunk/gcc/value-range.cc:1266
0x1e4fc26 range_operator::fold_range(irange&, tree_node*, irange const&, irange const&, relation_trio) const
        ../../gcc-trunk/gcc/range-op.cc:271
0x1e5030c operator_lshift::fold_range(irange&, tree_node*, irange const&, irange const&, relation_trio) const
        ../../gcc-trunk/gcc/range-op.cc:2246
0x1d4db80 fold_using_range::range_of_range_op(vrange&, gimple_range_op_handler&, fur_source&)
        ../../gcc-trunk/gcc/gimple-range-fold.cc:589
0x1d4f56a fold_using_range::fold_stmt(vrange&, gimple*, fur_source&, tree_node*)
        ../../gcc-trunk/gcc/gimple-range-fold.cc:489
0x1d40d7c gimple_ranger::fold_range_internal(vrange&, gimple*, tree_node*)
        ../../gcc-trunk/gcc/gimple-range.cc:257
0x1d40d7c gimple_ranger::range_of_stmt(vrange&, gimple*, tree_node*)
        ../../gcc-trunk/gcc/gimple-range.cc:318
0x1d42ac0 gimple_ranger::range_of_expr(vrange&, tree_node*, gimple*)
        ../../gcc-trunk/gcc/gimple-range.cc:126
0x1028acf cprop_operand
        ../../gcc-trunk/gcc/tree-ssa-dom.cc:1972
0x102a969 cprop_into_stmt
        ../../gcc-trunk/gcc/tree-ssa-dom.cc:2049
0x102a969 dom_opt_dom_walker::optimize_stmt(basic_block_def*, gimple_stmt_iterator*, bool*)
        ../../gcc-trunk/gcc/tree-ssa-dom.cc:2277
0x102bd13 dom_opt_dom_walker::before_dom_children(basic_block_def*)
        ../../gcc-trunk/gcc/tree-ssa-dom.cc:1682
0x1d0a0f7 dom_walker::walk(basic_block_def*)
        ../../gcc-trunk/gcc/domwalk.cc:311
Please submit a full bug report, with preprocessed source (by using -freport-bug).
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
[540] %
[540] % cat small.c
long a;
int main() {
  a = a ? 0 || 0 % 0 : 0;
  a = a << a;
  return 0;
}
Comment 1 Jakub Jelinek 2023-02-02 14:43:00 UTC
Started with r13-5578-g809d661aff99ae0287baf4a52269425de62381e6
Comment 2 Jakub Jelinek 2023-02-02 15:11:04 UTC
The problem is in
271	  if (relation_equiv_p (rel) && lh == rh)
We see
  # RANGE [irange] int [0, 1] NONZERO 0x1
  # iftmp.0_6 = PHI <_10(3), 0(2)>
  # RANGE [irange] long int [0, 1] NONZERO 0x1
  _3 = (long int) iftmp.0_6;
  # RANGE [irange] unsigned int [0, 1] NONZERO 0x1
  _4 = (unsigned int) iftmp.0_6;
  # RANGE [irange] long int [-INF, +INF] NONZERO 0x3
  _5 = _3 << _4;
so lhs and rhs indeed have the same range, but unlike most binary operations,
shifts/rotates have the same type between lhs and rhs1, but rhs2 can have different type.
So, the lh == rh comparison ICEs because the wide_ints have different precision (but same values).
Comment 3 Jakub Jelinek 2023-02-02 15:31:55 UTC
Created attachment 54391 [details]
gcc13-pr108639.patch

Untested fix.
Comment 4 Jakub Jelinek 2023-02-02 15:47:55 UTC
*** Bug 108638 has been marked as a duplicate of this bug. ***
Comment 5 Aldy Hernandez 2023-02-02 17:13:39 UTC
(In reply to Jakub Jelinek from comment #3)
> Created attachment 54391 [details]
> gcc13-pr108639.patch
> 
> Untested fix.

I think the problem is more fundamental than that.  The equality operator for irange is not ICEing for the sub-range comparison (which also have different precision), but is dying in the nonzero mask comparison.
Comment 6 Aldy Hernandez 2023-02-02 17:14:20 UTC
Created attachment 54393 [details]
untested patch for irange::operator==
Comment 7 Aldy Hernandez 2023-02-02 17:15:41 UTC
Jakub, take a look and see if you agree.  I've fired off some tests.
Comment 8 Jakub Jelinek 2023-02-02 18:07:40 UTC
(In reply to Aldy Hernandez from comment #5)
> (In reply to Jakub Jelinek from comment #3)
> > Created attachment 54391 [details]
> > gcc13-pr108639.patch
> > 
> > Untested fix.
> 
> I think the problem is more fundamental than that.  The equality operator
> for irange is not ICEing for the sub-range comparison (which also have
> different precision), but is dying in the nonzero mask comparison.

Well, that is obvious, because for the actual range boundaries you compare trees, not wide_ints.  And operand_equal_p does allow comparing of trees with different types.
It in that case calls tree_int_cst_equal.  But when you switch the boundaries from trees to wide_ints, they will ICE again as well.

I think for the operator==, the important question is, shall ranges with same values but non-compatible types compare
1) equal
2) non-equal
3) be an ICE (e.g. gcc_checking_assert)
The current state is 3) without the assert and my patch was trying to fix the caller.
Your patch is changing it to 1), in that case no change would be needed in the particular lh == rh user, but are we sure that everywhere where non-compatible types could appear we don't imply from operator== returning true that the types are actually the same?
If we did 2) e.g. by adding a types_compatible_p (type (), b.type ()) check, then we'd
need to change this lh == rh user too, because for the shifts we really just care about values and not the exact types which can differ.
Comment 9 Andrew Macleod 2023-02-02 19:09:18 UTC
(In reply to Jakub Jelinek from comment #8)
> (In reply to Aldy Hernandez from comment #5)
> > (In reply to Jakub Jelinek from comment #3)
> > > Created attachment 54391 [details]
> > > gcc13-pr108639.patch
> > > 
> > > Untested fix.
> > 
> > I think the problem is more fundamental than that.  The equality operator
> > for irange is not ICEing for the sub-range comparison (which also have
> > different precision), but is dying in the nonzero mask comparison.
> 
> Well, that is obvious, because for the actual range boundaries you compare
> trees, not wide_ints.  And operand_equal_p does allow comparing of trees
> with different types.
> It in that case calls tree_int_cst_equal.  But when you switch the
> boundaries from trees to wide_ints, they will ICE again as well.
> 
> I think for the operator==, the important question is, shall ranges with
> same values but non-compatible types compare
> 1) equal
> 2) non-equal
> 3) be an ICE (e.g. gcc_checking_assert)
> The current state is 3) without the assert and my patch was trying to fix
> the caller.

3) is not a desired result for sure.

I think 1) is our desired outcome... I initially considered 2) as desirable as we could simply check the types before doing any other work.   As I thought about it tho, I can't think of any good reason why we would want them to be unequal.

If they compare equal this way (ignoring type), then performing a cast on either of them to the same type as the other would also compare equal... its symmetrical so that actually makes them equal in my book.
Comment 10 Jakub Jelinek 2023-02-02 19:56:07 UTC
Ok then.
I won't test my patch then, the testcases from it were:
--- gcc/testsuite/gcc.c-torture/compile/pr108638.c.jj	2022-11-21 10:04:00.210677046 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr108638.c	2023-02-02 16:51:06.082371450 +0100
@@ -0,0 +1,12 @@
+/* PR tree-optimization/108638 */
+
+long long a;
+int b;
+
+void
+foo (void)
+{
+  for (a = 0; a < __SIZEOF_LONG_LONG__ * __CHAR_BIT__; a++)
+    if (b)
+      b |= a << a;
+}
--- gcc/testsuite/gcc.c-torture/compile/pr108639.c.jj	2023-02-02 16:26:44.113600462 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr108639.c	2023-02-02 16:26:23.309902211 +0100
@@ -0,0 +1,11 @@
+/* PR tree-optimization/108639 */
+
+long long a;
+
+int
+main ()
+{
+  a = a ? 0 || 0 % 0 : 0;
+  a = a << a;
+  return 0;
+}
Comment 11 Richard Biener 2023-02-03 07:56:41 UTC
Yes, they should compare equal.  Integer constant ranges do not need a type.
Not even FP constant ranges.  Symbolic ranges need a type, but then the endpoints in their symbolic form have one (and those might even differ).
Comment 12 Aldy Hernandez 2023-02-03 08:49:51 UTC
Yeah, I've been mulling around the idea of removing the type from storage of both irange and frange.  It seems we need it for setting a type (setting the endpoints for varying, querying HONOR_NANS, HONOR_INFINITIES,e tc), but not in the storage itself.  Something to keep in mind when moving to wide_ints.

It does seem we need to keep the sign and precision somewhere.  The precision lives in the wide-int, but the sign bit still needs storage??
Comment 13 GCC Commits 2023-02-03 20:33:37 UTC
The master branch has been updated by Aldy Hernandez <aldyh@gcc.gnu.org>:

https://gcc.gnu.org/g:e261fcefb71e1270673f0457fcc73711f13d3079

commit r13-5696-ge261fcefb71e1270673f0457fcc73711f13d3079
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Thu Feb 2 18:08:44 2023 +0100

    irange: Compare nonzero bits in irange with widest_int [PR108639]
    
    The problem here is we are trying to compare two ranges with different
    precisions and the == operator in wide_int is complaining.
    
    Interestingly, the problem is not the nonzero bits, but the fact that
    the entire ranges have different precisions.  The reason we don't ICE
    when comparing the sub-ranges, is because the code in
    irange::operator== works on trees, and tree_int_cst_equal is
    promoting the comparison to a widest int:
    
      if (TREE_CODE (t1) == INTEGER_CST
          && TREE_CODE (t2) == INTEGER_CST
          && wi::to_widest (t1) == wi::to_widest (t2))
        return 1;
    
    This is why we don't see the ICE until the nonzero bits comparison is
    done on wide ints.  I think we should maintain the current equality
    behavior, and follow suit in the nonzero bit comparison.
    
    I have also fixed the legacy equality code, even though technically
    nonzero bits shouldn't appear in legacy.  But better safe than sorry.
    
            PR tree-optimization/108639
    
    gcc/ChangeLog:
    
            * value-range.cc (irange::legacy_equal_p): Compare nonzero bits as
            widest_int.
            (irange::operator==): Same.
Comment 14 Jakub Jelinek 2023-02-03 20:43:39 UTC
Fixed.
Comment 15 Andrew Pinski 2023-02-03 22:56:40 UTC
*** Bug 108668 has been marked as a duplicate of this bug. ***