Re: [PATCH] PR c++/69399: Add HAVE_WORKING_CXX_BUILTIN_CONSTANT_P

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Jan 25, 2016 at 5:25 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jan 25, 2016 at 4:40 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Fri, Jan 22, 2016 at 7:55 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>> Without the fix for PR 65656, g++ miscompiles __builtin_constant_p in
>>> wi::lrshift in wide-int.h.  Add a check with PR 65656 testcase to verify
>>> that C++ __builtin_constant_p works properly.
>>>
>>> Tested on x86-64 with working GCC:
>>>
>>> gcc/auto-host.h:/* #undef HAVE_WORKING_CXX_BUILTIN_CONSTANT_P */
>>> prev-gcc/auto-host.h:/* #undef HAVE_WORKING_CXX_BUILTIN_CONSTANT_P */
>>> stage1-gcc/auto-host.h:#define HAVE_WORKING_CXX_BUILTIN_CONSTANT_P 1
>>>
>>> and broken GCC:
>>>
>>> gcc/auto-host.h:/* #undef HAVE_WORKING_CXX_BUILTIN_CONSTANT_P */
>>> prev-gcc/auto-host.h:/* #undef HAVE_WORKING_CXX_BUILTIN_CONSTANT_P */
>>> stage1-gcc/auto-host.h:/* #undef HAVE_WORKING_CXX_BUILTIN_CONSTANT_P */
>>>
>>> Ok for trunk?
>>
>> I have a hard time seeing how we are "miscompiling"
>>
>>       if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
>>           ? xi.len == 1 && xi.val[0] >= 0
>>           : xi.precision <= HOST_BITS_PER_WIDE_INT)
>>
>> anything that relies on __builtin_constant_p () for sematics is fishy so why
>> is this not simply an lrshfit implementation bug?
>>
>
>
> We hit this via:
>
> Breakpoint 1, wi::lrshift<generic_wide_int<fixed_wide_int_storage<192>
>>, generic_wide_int<fixed_wide_int_storage<192> > > (x=..., y=...)
>     at /export/gnu/import/git/sources/gcc-release/gcc/wide-int.h:2898
> 2898  val[0] = xi.to_uhwi () >> shift;
> (gdb) bt
> #0  wi::lrshift<generic_wide_int<fixed_wide_int_storage<192> >,
> generic_wide_int<fixed_wide_int_storage<192> > > (x=..., y=...)
>     at /export/gnu/import/git/sources/gcc-release/gcc/wide-int.h:2898
> #1  0x00000000009e7bbe in
> wi::rshift<generic_wide_int<fixed_wide_int_storage<192> >,
> generic_wide_int<fixed_wide_int_storage<192> > > (sgn=<optimized out>,
>     y=..., x=...)
>     at /export/gnu/import/git/sources/gcc-release/gcc/wide-int.h:2947
> #2  bit_value_binop_1 (code=code@entry=RSHIFT_EXPR,
>     type=type@entry=0x7fffefe82dc8, val=val@entry=0x7fffffffd7c0,
>     mask=mask@entry=0x7fffffffd790, r1type=0x7fffefe82dc8, r1val=...,
>     r1mask=..., r2type=0x7fffefd6b690, r2val=..., r2mask=...)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-ccp.c:1348
> #3  0x00000000009e9e7b in bit_value_binop (code=code@entry=RSHIFT_EXPR,
>     type=0x7fffefe82dc8, rhs1=rhs1@entry=0x7fffefd71708, rhs2=<optimized out>)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-ccp.c:1549
> #4  0x00000000009eb520 in evaluate_stmt (stmt=stmt@entry=0x7fffefe9a160)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-ccp.c:1785
> #5  0x00000000009ec8d2 in visit_assignment (stmt=stmt@entry=0x7fffefe9a160,
>     output_p=output_p@entry=0x7fffffffdba0)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-ccp.c:2258
> #6  0x00000000009ec9c2 in ccp_visit_stmt (stmt=0x7fffefe9a160,
>     taken_edge_p=0x7fffffffdba8, output_p=0x7fffffffdba0)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-ccp.c:2336
> ---Type <return> to continue, or q <return> to quit---
> #7  0x0000000000a4efcf in simulate_stmt (stmt=stmt@entry=0x7fffefe9a160)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-propagate.c:348
> #8  0x0000000000a50f79 in simulate_block (block=<optimized out>)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-propagate.c:471
> #9  ssa_propagate (
>     visit_stmt=visit_stmt@entry=0x9ec937 <ccp_visit_stmt(gimple,
> edge*, tree*)>, visit_phi=visit_phi@entry=0x9e6aa5
> <ccp_visit_phi_node(gphi*)>)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-propagate.c:888
> #10 0x00000000009e6295 in do_ssa_ccp ()
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-ccp.c:2382
> #11 (anonymous namespace)::pass_ccp::execute (this=<optimized out>)
>     at /export/gnu/import/git/sources/gcc-release/gcc/tree-ssa-ccp.c:2415
> #12 0x000000000089ca0c in execute_one_pass (pass=pass@entry=0x19b4bf0)
>     at /export/gnu/import/git/sources/gcc-release/gcc/passes.c:2330
> #13 0x000000000089cd62 in execute_pass_list_1 (pass=0x19b4bf0)
>     at /export/gnu/import/git/sources/gcc-release/gcc/passes.c:2382
> #14 0x000000000089cd7f in execute_pass_list_1 (pass=0x19b4a70,
>     pass@entry=0x19b48f0)
>     at /export/gnu/import/git/sources/gcc-release/gcc/passes.c:2383
> #15 0x000000000089cd9c in execute_pass_list (fn=0x7fffefe98000, pass=0x19b48f0)
>     at /export/gnu/import/git/sources/gcc-release/gcc/passes.c:2393
> #16 0x000000000089ba57 in do_per_function_toporder (
>     callback=callback@entry=0x89cd83 <execute_pass_list(function*,
> opt_pass*)>, ---Type <return> to continue, or q <return> to quit---
> data=0x19b48f0) at /export/gnu/import/git/sources/gcc-release/gcc/passes.c:1728
> #17 0x000000000089d3e3 in execute_ipa_pass_list (pass=0x19b4890)
>     at /export/gnu/import/git/sources/gcc-release/gcc/passes.c:2736
> #18 0x000000000066f3ac in ipa_passes ()
>     at /export/gnu/import/git/sources/gcc-release/gcc/cgraphunit.c:2172
> #19 symbol_table::compile (this=this@entry=0x7fffefd6b000)
>     at /export/gnu/import/git/sources/gcc-release/gcc/cgraphunit.c:2313
> #20 0x0000000000670be5 in symbol_table::finalize_compilation_unit (
>     this=0x7fffefd6b000)
>     at /export/gnu/import/git/sources/gcc-release/gcc/cgraphunit.c:2462
> #21 0x000000000058ea41 in c_write_global_declarations ()
>     at /export/gnu/import/git/sources/gcc-release/gcc/c/c-decl.c:10822
> #22 0x0000000000939509 in compile_file ()
>     at /export/gnu/import/git/sources/gcc-release/gcc/toplev.c:613
> #23 0x000000000093b3c4 in do_compile ()
>     at /export/gnu/import/git/sources/gcc-release/gcc/toplev.c:2067
> #24 toplev::main (this=this@entry=0x7fffffffdf60, argc=argc@entry=15,
>     argv=argv@entry=0x7fffffffe068)
>     at /export/gnu/import/git/sources/gcc-release/gcc/toplev.c:2165
> #25 0x0000000000f50ab7 in main (argc=15, argv=0x7fffffffe068)
>     at /export/gnu/import/git/sources/gcc-release/gcc/main.c:39
> (gdb) p x
> $2 = (const generic_wide_int<fixed_wide_int_storage<192> > &)
> @0x7fffffffd670: {<fixed_wide_int_storage<192>> = {val = {4294967169,
> 0, 140737217214936, 92},
>     len = 1}, static is_sign_extended = <optimized out>}
> (gdb) p y
> $3 = (const generic_wide_int<fixed_wide_int_storage<192> > &)
> @0x7fffffffd640: {<fixed_wide_int_storage<192>> = {val = {64,
> 824633720833, -4294967168,
>       140737218308160}, len = 1}, static is_sign_extended = <optimized out>}
> (gdb) p xi
> $3 = {<wide_int_ref_storage<true>> = {<wi::storage_ref> = {
>       val = 0x7fffffffcfe0, len = 2, precision = 192}, scratch = {64,
>       8589921072}}, static is_sign_extended = <optimized out>}
> (gdb) p yi
> $4 = {<wide_int_ref_storage<true>> = {<wi::storage_ref> = {
>       val = 0x7fffffffcbc0, len = 1, precision = 192}, scratch = {
>       140737488344768, 140737488343008}},
>   static is_sign_extended = <optimized out>}
> (gdb)
>
> Somehow PR 65656 miscompiled:
>
>       if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
>           ? xi.len == 1 && xi.val[0] >= 0
>           : xi.precision <= HOST_BITS_PER_WIDE_INT)
>
> which turned the expression into true and hit
>
>           val[0] = xi.to_uhwi () >> shift;
>           result.set_len (1);

I think we need a better analysis as we use __builtin_constant_p
elsewhere as well.

Richard.

>
> --
> H.J.