Bug 93231 - [10 Regression] ICEs since r280132
Summary: [10 Regression] ICEs since r280132
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 10.0
: P1 normal
Target Milestone: 10.0
Assignee: Wilco
URL:
Keywords: ice-on-valid-code
Depends on:
Blocks:
 
Reported: 2020-01-10 22:14 UTC by Jakub Jelinek
Modified: 2020-01-16 13:36 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-01-10 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2020-01-10 22:14:18 UTC
int ctz2 (int x)
{
  static const char table[32] =
    {
      0, 1, 28, 2, 29, 14, 24, 3, 30, 22, 20, 15, 25, 17, 4, 8,
      31, 27, 13, 23, 21, 19, 16, 7, 26, 12, 18, 6, 11, 5, 10, 9
    };

  return table[((int)((x & -x) * -0x077CB531)) >> 27];
}

ICEs because
   unsigned HOST_WIDE_INT val = tree_to_uhwi (mulc);
is used without checking that the INTEGER_CST fits into uhwi.
If we don't want to support signed x, we should start the function by verification that inner_type is INTEGRAL_TYPE_P which is TYPE_UNSIGNED, if we do want to support even signed values, it needs to be tweaked differently.

Similarly,
int ctz3 (unsigned x)
{
  static const char table[32] =
    {
      0, 1, 28, 2, 29, 14, 24, 3, 30, 22, 20, 15, 25, 17, 4, 8,
      31, 27, 13, 23, 21, 19, 16, 7, 26, 12, 18, 6, 11, 5, 10, 9
    };

  return table[((unsigned)((x & -x) * 0x077CB531U)) >> -27];
}
ICEs because -27 doesn't fit into uhwi.  We should just punt if it doesn't.
I'm also surprised by
  /* Check the array is not wider than integer type and the input is a 32-bit
     or 64-bit type.  */
  if (TYPE_PRECISION (type) > 32)
    return false;
because the comment doesn't match what the check is doing, either you want an array with 32-bit or smaller elts, then the comment should match that, or
you care about integers and then you should compare against TYPE_PRECISION (integer_type_node).
Comment 1 Andrew Pinski 2020-01-10 22:25:35 UTC
Confirmed.  Using wide_int might make this simplier and all.
Comment 2 Jakub Jelinek 2020-01-10 22:28:32 UTC
Also, there is no testcase for the string case, nor any non-target specific testcase that it at least compiles and perhaps with tree dump scan on selected targets that it recognizes the ctz.
And I don't see a check that if it is a STRING_CST, the array elements must be bytes and not wider, which the function assumes (e.g. if it is u"..."[(x & -x) * ...) >> 27]).
Comment 3 Jakub Jelinek 2020-01-10 22:40:10 UTC
Perhaps with the just added native_encode_initializer, rather than having separate function for CONSTRUCTOR and STRING_CST it might be better to always native_encode_initializer at certain offset with byte length of the array element type, verify it returned the passed in length and then native_interpret_expr it?
Comment 4 Wilco 2020-01-13 12:05:34 UTC
(In reply to Jakub Jelinek from comment #0)
> int ctz2 (int x)
> {
>   static const char table[32] =
>     {
>       0, 1, 28, 2, 29, 14, 24, 3, 30, 22, 20, 15, 25, 17, 4, 8,
>       31, 27, 13, 23, 21, 19, 16, 7, 26, 12, 18, 6, 11, 5, 10, 9
>     };
> 
>   return table[((int)((x & -x) * -0x077CB531)) >> 27];
> }
> 
> ICEs because
>    unsigned HOST_WIDE_INT val = tree_to_uhwi (mulc);
> is used without checking that the INTEGER_CST fits into uhwi.
> If we don't want to support signed x, we should start the function by
> verification that inner_type is INTEGRAL_TYPE_P which is TYPE_UNSIGNED, if
> we do want to support even signed values, it needs to be tweaked differently.

I guess TREE_INT_CST_LOW should fix that. The goal is to support signed and unsigned types.

> Similarly,
> int ctz3 (unsigned x)
> {
>   static const char table[32] =
>     {
>       0, 1, 28, 2, 29, 14, 24, 3, 30, 22, 20, 15, 25, 17, 4, 8,
>       31, 27, 13, 23, 21, 19, 16, 7, 26, 12, 18, 6, 11, 5, 10, 9
>     };
> 
>   return table[((unsigned)((x & -x) * 0x077CB531U)) >> -27];
> }
> ICEs because -27 doesn't fit into uhwi.  We should just punt if it doesn't.

I'm adding an extra tree_fits_uhwi for that.

> I'm also surprised by
>   /* Check the array is not wider than integer type and the input is a 32-bit
>      or 64-bit type.  */
>   if (TYPE_PRECISION (type) > 32)
>     return false;
> because the comment doesn't match what the check is doing, either you want
> an array with 32-bit or smaller elts, then the comment should match that, or
> you care about integers and then you should compare against TYPE_PRECISION
> (integer_type_node).

I'll check but I think this check is no longer required.

> Also, there is no testcase for the string case, nor any non-target specific
> testcase that it at least compiles and perhaps with tree dump scan on selected > targets that it recognizes the ctz.

I can add a generic testcase as well.

> And I don't see a check that if it is a STRING_CST, the array elements must be 
> bytes and not wider, which the function assumes (e.g. if it is u"..."[(x & -x) > * ...) >> 27]).

Right now "abc"[] can't match - the constructor always returns an error for this case. And this doesn't seem a common idiom so adding support isn't useful.
Comment 5 Jakub Jelinek 2020-01-13 12:26:01 UTC
(In reply to Wilco from comment #4)
> I guess TREE_INT_CST_LOW should fix that. The goal is to support signed and
> unsigned types.

Maybe.  Though, perhaps you want to zero extend the constant from the type's precision to HOST_BITS_PER_WIDE_INT too.

> I'm adding an extra tree_fits_uhwi for that.

Ok.

> I'll check but I think this check is no longer required.

I guess it wouldn't work if the constants are 64-bit, because then tree_to_shwi might not work for them.

> I can add a generic testcase as well.

Great.

> > And I don't see a check that if it is a STRING_CST, the array elements must be 
> > bytes and not wider, which the function assumes (e.g. if it is u"..."[(x & -x) > * ...) >> 27]).
> 
> Right now "abc"[] can't match - the constructor always returns an error for
> this case. And this doesn't seem a common idiom so adding support isn't
> useful.

But you do have check_ctz_string, so let's talk about const type var[] = u"abcd"; or const type var[] = "defg";
I was just lazy to construct an u"\x...\x...\x......" string that would trigger with check_ctz_string reading it as by array, when at runtime it would actually be treated as short array etc.

For the non-zero stuff, perhaps you could if (tree_expr_nonzero_p (res_ops[0])) { zero_ok = true; zeroval = 0; ctzval = 0; } or so, because if x is not zero, then you don't need to bother with it, don't need to & mask at the end or punt etc.
Comment 7 CVS Commits 2020-01-15 18:39:39 UTC
The master branch has been updated by Wilco Dijkstra <wilco@gcc.gnu.org>:

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

commit r10-5986-gbc071d3a951a98284a3f46043afd44c03c123376
Author: Wilco Dijkstra <wdijkstr@arm.com>
Date:   Wed Jan 15 15:23:54 2020 +0000

    Fix ctz issues (PR93231)
    
    Further improve the ctz recognition: Avoid ICEing on negative shift
    counts or multiply constants.  Check the type is a char type for the
    string constant case to avoid accidentally matching a wide STRING_CST.
    Add a tree_expr_nonzero_p check to allow the optimization even if
    CTZ_DEFINED_VALUE_AT_ZERO returns 0 or 1.  Add extra test cases.
    
    Bootstrap OK on AArch64 and x64.
    
        gcc/
    	PR tree-optimization/93231
    	* tree-ssa-forwprop.c (optimize_count_trailing_zeroes): Check
    	input_type is unsigned.  Use tree_to_shwi for shift constant.
    	Check CST_STRING element size is CHAR_TYPE_SIZE bits.
    	(simplify_count_trailing_zeroes): Add test to handle known non-zero
    	inputs more efficiently.
    
        testsuite/
    	PR tree-optimization/93231
    	* gcc.dg/pr90838.c: New test.
    	* gcc.dg/pr93231.c: New test.
    	* gcc.target/aarch64/pr90838.c: Use #define u 0.
Comment 8 Wilco 2020-01-16 13:36:42 UTC
Fixed.