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

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

[H.J. Lu]

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?

Thanks.

H.J.
---
gcc/

	PR c++/69399
	* configure.ac: Check if C++ __builtin_constant_p works
	properly.
	(HAVE_WORKING_CXX_BUILTIN_CONSTANT_P): AC_DEFINE.
	* system.h (STATIC_CONSTANT_P): Use __builtin_constant_p only
	if HAVE_WORKING_CXX_BUILTIN_CONSTANT_P is defined.
	* config.in: Regenerated.
	* configure: Likewise.

gcc/testsuite/

	PR c++/69399
	* gcc.dg/torture/pr69399.c: New test.
---
 gcc/config.in                          | 10 ++++++++-
 gcc/configure                          | 41 ++++++++++++++++++++++++++++++++--
 gcc/configure.ac                       | 27 ++++++++++++++++++++++
 gcc/system.h                           |  2 +-
 gcc/testsuite/gcc.dg/torture/pr69399.c | 21 +++++++++++++++++
 5 files changed, 97 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr69399.c

diff --git a/gcc/config.in b/gcc/config.in
index 1796e1d..11ebf5c 100644
--- a/gcc/config.in
+++ b/gcc/config.in
@@ -1846,6 +1846,13 @@
 #endif
 
 
+/* Define this macro if C++ __builtin_constant_p with constexpr does not crash
+   with a variable. */
+#ifndef USED_FOR_TARGET
+#undef HAVE_WORKING_CXX_BUILTIN_CONSTANT_P
+#endif
+
+
 /* Define to 1 if `fork' works. */
 #ifndef USED_FOR_TARGET
 #undef HAVE_WORKING_FORK
@@ -1865,7 +1872,8 @@
 #endif
 
 
-/* Define if your assembler supports .dwsect 0xB0000 */
+/* Define if your assembler supports AIX debug frame section label reference.
+   */
 #ifndef USED_FOR_TARGET
 #undef HAVE_XCOFF_DWARF_EXTRAS
 #endif
diff --git a/gcc/configure b/gcc/configure
index ff646e8..2798231 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -6534,6 +6534,43 @@ fi
 rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
 fi
 
+# Check if C++ __builtin_constant_p works properly.  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.
+if test "$GCC" = yes; then
+  saved_CFLAGS="$CFLAGS"
+  saved_CXXFLAGS="$CXXFLAGS"
+  CFLAGS="$CFLAGS -O -x c++ -std=c++11"
+  CXXFLAGS="$CXXFLAGS -O -x c++ -std=c++11"
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CXX __builtin_constant_p works with constexpr" >&5
+$as_echo_n "checking whether $CXX __builtin_constant_p works with constexpr... " >&6; }
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+foo (int argc)
+{
+  constexpr bool x = __builtin_constant_p(argc);
+  return x ? 1 : 0;
+}
+
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
+$as_echo "yes" >&6; }
+
+$as_echo "#define HAVE_WORKING_CXX_BUILTIN_CONSTANT_P 1" >>confdefs.h
+
+else
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+  CFLAGS="$saved_CFLAGS"
+  CXXFLAGS="$saved_CXXFLAGS"
+fi
+
 # Check whether compiler is affected by placement new aliasing bug (PR 29286).
 # If the host compiler is affected by the bug, and we build with optimization
 # enabled (which happens e.g. when cross-compiling), the pool allocator may
@@ -18419,7 +18456,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18422 "configure"
+#line 18459 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -18525,7 +18562,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18528 "configure"
+#line 18565 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 4dc7c10..9791a96 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -416,6 +416,33 @@ struct X<long long> { typedef long long t; };
 ]], [[X<int64_t>::t x;]])],[],[AC_MSG_ERROR([error verifying int64_t uses long long])])
 fi
 
+# Check if C++ __builtin_constant_p works properly.  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.
+if test "$GCC" = yes; then
+  saved_CFLAGS="$CFLAGS"
+  saved_CXXFLAGS="$CXXFLAGS"
+  CFLAGS="$CFLAGS -O -x c++ -std=c++11"
+  CXXFLAGS="$CXXFLAGS -O -x c++ -std=c++11"
+  AC_MSG_CHECKING(whether $CXX __builtin_constant_p works with constexpr)
+  AC_COMPILE_IFELSE([
+int
+foo (int argc)
+{
+  constexpr bool x = __builtin_constant_p(argc);
+  return x ? 1 : 0;
+}
+],
+    [AC_MSG_RESULT([yes])
+     AC_DEFINE(HAVE_WORKING_CXX_BUILTIN_CONSTANT_P, 1,
+[Define this macro if C++ __builtin_constant_p with constexpr does not
+ crash with a variable.])],
+    [AC_MSG_RESULT([no])])
+  CFLAGS="$saved_CFLAGS"
+  CXXFLAGS="$saved_CXXFLAGS"
+fi
+
 # Check whether compiler is affected by placement new aliasing bug (PR 29286).
 # If the host compiler is affected by the bug, and we build with optimization
 # enabled (which happens e.g. when cross-compiling), the pool allocator may
diff --git a/gcc/system.h b/gcc/system.h
index ba2e963..e57787b 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -729,7 +729,7 @@ extern void fancy_abort (const char *, int, const char *) ATTRIBUTE_NORETURN;
 #define gcc_unreachable() (fancy_abort (__FILE__, __LINE__, __FUNCTION__))
 #endif
 
-#if GCC_VERSION >= 3001
+#if GCC_VERSION >= 3001 && defined HAVE_WORKING_CXX_BUILTIN_CONSTANT_P
 #define STATIC_CONSTANT_P(X) (__builtin_constant_p (X) && (X))
 #else
 #define STATIC_CONSTANT_P(X) (false && (X))
diff --git a/gcc/testsuite/gcc.dg/torture/pr69399.c b/gcc/testsuite/gcc.dg/torture/pr69399.c
new file mode 100644
index 0000000..3e17169
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr69399.c
@@ -0,0 +1,21 @@
+/* { dg-do run { target int128 } } */
+
+typedef __UINT64_TYPE__ u64;
+typedef unsigned __int128 u128;
+
+static unsigned __attribute__((noinline, noclone))
+foo(u64 u)
+{
+  u128 v = u | 0xffffff81;
+  v >>= 64;
+  return v;
+}
+
+int
+main()
+{
+  unsigned x = foo(27);
+  if (x != 0)
+    __builtin_abort();
+  return 0;
+}
-- 
2.5.0

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

[Richard Biener]

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?

Richard.

> Thanks.
>
> H.J.
> ---
> gcc/
>
>         PR c++/69399
>         * configure.ac: Check if C++ __builtin_constant_p works
>         properly.
>         (HAVE_WORKING_CXX_BUILTIN_CONSTANT_P): AC_DEFINE.
>         * system.h (STATIC_CONSTANT_P): Use __builtin_constant_p only
>         if HAVE_WORKING_CXX_BUILTIN_CONSTANT_P is defined.
>         * config.in: Regenerated.
>         * configure: Likewise.
>
> gcc/testsuite/
>
>         PR c++/69399
>         * gcc.dg/torture/pr69399.c: New test.
> ---
>  gcc/config.in                          | 10 ++++++++-
>  gcc/configure                          | 41 ++++++++++++++++++++++++++++++++--
>  gcc/configure.ac                       | 27 ++++++++++++++++++++++
>  gcc/system.h                           |  2 +-
>  gcc/testsuite/gcc.dg/torture/pr69399.c | 21 +++++++++++++++++
>  5 files changed, 97 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/torture/pr69399.c
>
> diff --git a/gcc/config.in b/gcc/config.in
> index 1796e1d..11ebf5c 100644
> --- a/gcc/config.in
> +++ b/gcc/config.in
> @@ -1846,6 +1846,13 @@
>  #endif
>
>
> +/* Define this macro if C++ __builtin_constant_p with constexpr does not crash
> +   with a variable. */
> +#ifndef USED_FOR_TARGET
> +#undef HAVE_WORKING_CXX_BUILTIN_CONSTANT_P
> +#endif
> +
> +
>  /* Define to 1 if `fork' works. */
>  #ifndef USED_FOR_TARGET
>  #undef HAVE_WORKING_FORK
> @@ -1865,7 +1872,8 @@
>  #endif
>
>
> -/* Define if your assembler supports .dwsect 0xB0000 */
> +/* Define if your assembler supports AIX debug frame section label reference.
> +   */
>  #ifndef USED_FOR_TARGET
>  #undef HAVE_XCOFF_DWARF_EXTRAS
>  #endif
> diff --git a/gcc/configure b/gcc/configure
> index ff646e8..2798231 100755
> --- a/gcc/configure
> +++ b/gcc/configure
> @@ -6534,6 +6534,43 @@ fi
>  rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>  fi
>
> +# Check if C++ __builtin_constant_p works properly.  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.
> +if test "$GCC" = yes; then
> +  saved_CFLAGS="$CFLAGS"
> +  saved_CXXFLAGS="$CXXFLAGS"
> +  CFLAGS="$CFLAGS -O -x c++ -std=c++11"
> +  CXXFLAGS="$CXXFLAGS -O -x c++ -std=c++11"
> +  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CXX __builtin_constant_p works with constexpr" >&5
> +$as_echo_n "checking whether $CXX __builtin_constant_p works with constexpr... " >&6; }
> +  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h.  */
> +
> +int
> +foo (int argc)
> +{
> +  constexpr bool x = __builtin_constant_p(argc);
> +  return x ? 1 : 0;
> +}
> +
> +_ACEOF
> +if ac_fn_cxx_try_compile "$LINENO"; then :
> +  { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
> +$as_echo "yes" >&6; }
> +
> +$as_echo "#define HAVE_WORKING_CXX_BUILTIN_CONSTANT_P 1" >>confdefs.h
> +
> +else
> +  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> +$as_echo "no" >&6; }
> +fi
> +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
> +  CFLAGS="$saved_CFLAGS"
> +  CXXFLAGS="$saved_CXXFLAGS"
> +fi
> +
>  # Check whether compiler is affected by placement new aliasing bug (PR 29286).
>  # If the host compiler is affected by the bug, and we build with optimization
>  # enabled (which happens e.g. when cross-compiling), the pool allocator may
> @@ -18419,7 +18456,7 @@ else
>    lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>    lt_status=$lt_dlunknown
>    cat > conftest.$ac_ext <<_LT_EOF
> -#line 18422 "configure"
> +#line 18459 "configure"
>  #include "confdefs.h"
>
>  #if HAVE_DLFCN_H
> @@ -18525,7 +18562,7 @@ else
>    lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>    lt_status=$lt_dlunknown
>    cat > conftest.$ac_ext <<_LT_EOF
> -#line 18528 "configure"
> +#line 18565 "configure"
>  #include "confdefs.h"
>
>  #if HAVE_DLFCN_H
> diff --git a/gcc/configure.ac b/gcc/configure.ac
> index 4dc7c10..9791a96 100644
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -416,6 +416,33 @@ struct X<long long> { typedef long long t; };
>  ]], [[X<int64_t>::t x;]])],[],[AC_MSG_ERROR([error verifying int64_t uses long long])])
>  fi
>
> +# Check if C++ __builtin_constant_p works properly.  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.
> +if test "$GCC" = yes; then
> +  saved_CFLAGS="$CFLAGS"
> +  saved_CXXFLAGS="$CXXFLAGS"
> +  CFLAGS="$CFLAGS -O -x c++ -std=c++11"
> +  CXXFLAGS="$CXXFLAGS -O -x c++ -std=c++11"
> +  AC_MSG_CHECKING(whether $CXX __builtin_constant_p works with constexpr)
> +  AC_COMPILE_IFELSE([
> +int
> +foo (int argc)
> +{
> +  constexpr bool x = __builtin_constant_p(argc);
> +  return x ? 1 : 0;
> +}
> +],
> +    [AC_MSG_RESULT([yes])
> +     AC_DEFINE(HAVE_WORKING_CXX_BUILTIN_CONSTANT_P, 1,
> +[Define this macro if C++ __builtin_constant_p with constexpr does not
> + crash with a variable.])],
> +    [AC_MSG_RESULT([no])])
> +  CFLAGS="$saved_CFLAGS"
> +  CXXFLAGS="$saved_CXXFLAGS"
> +fi
> +
>  # Check whether compiler is affected by placement new aliasing bug (PR 29286).
>  # If the host compiler is affected by the bug, and we build with optimization
>  # enabled (which happens e.g. when cross-compiling), the pool allocator may
> diff --git a/gcc/system.h b/gcc/system.h
> index ba2e963..e57787b 100644
> --- a/gcc/system.h
> +++ b/gcc/system.h
> @@ -729,7 +729,7 @@ extern void fancy_abort (const char *, int, const char *) ATTRIBUTE_NORETURN;
>  #define gcc_unreachable() (fancy_abort (__FILE__, __LINE__, __FUNCTION__))
>  #endif
>
> -#if GCC_VERSION >= 3001
> +#if GCC_VERSION >= 3001 && defined HAVE_WORKING_CXX_BUILTIN_CONSTANT_P
>  #define STATIC_CONSTANT_P(X) (__builtin_constant_p (X) && (X))
>  #else
>  #define STATIC_CONSTANT_P(X) (false && (X))
> diff --git a/gcc/testsuite/gcc.dg/torture/pr69399.c b/gcc/testsuite/gcc.dg/torture/pr69399.c
> new file mode 100644
> index 0000000..3e17169
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr69399.c
> @@ -0,0 +1,21 @@
> +/* { dg-do run { target int128 } } */
> +
> +typedef __UINT64_TYPE__ u64;
> +typedef unsigned __int128 u128;
> +
> +static unsigned __attribute__((noinline, noclone))
> +foo(u64 u)
> +{
> +  u128 v = u | 0xffffff81;
> +  v >>= 64;
> +  return v;
> +}
> +
> +int
> +main()
> +{
> +  unsigned x = foo(27);
> +  if (x != 0)
> +    __builtin_abort();
> +  return 0;
> +}
> --
> 2.5.0
>

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

[H.J. Lu]

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);


-- 
H.J.

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

[Richard Biener]

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.

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

[Jakub Jelinek]

On Tue, Jan 26, 2016 at 04:54:43PM +0100, Richard Biener wrote:
> > 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.

Yeah, it would be nice to understand the somehow and what exactly is gong
on.
So, what is xi.precision, is it variable or constant, what value does it
have, has __builtin_constant_p returned 1 when it was supposed to return 0?
But then there would be still the precision check.
What is xi.len and xi.val[0]?

	Jakub

[PATCH] Fix up wi::lrshift (PR c++/69399)

[Jakub Jelinek]

Hi!

On Tue, Jan 26, 2016 at 04:54:43PM +0100, Richard Biener wrote:
> > 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.

So, I had a look at this bug and it seems clearly a bug on the wide-int.h
side.  wi::lrshift right now doesn't do what the comment says (which says
that for the larger precision fixed types it only cares about constant
shift).
Compared to wi::lshift, for the
STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
case lrshift doesn't bother to check the value of shift.
While for xi.precision <= HOST_BITS_PER_WIDE_INT, at least conforming code
should not perform out of bounds shifts and thus there is no need to check
the value of shift, for xi.precision > HOST_BITS_PER_WIDE_INT it is
completely valid to have large shift (in between HOST_BITS_PER_WIDE_INT
inclusive and xi.precision exclusive), and then we just trigger undefined
behavior for that case.

The question is, shall we do what wi::lshift does and have the fast path
only for the constant shifts, as the untested patch below does, or shall we
check shift dynamically (thus use
shift < HOST_BITS_PER_WIDE_INT
instead of
STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
in the patch), or shall we have another case for such shifts and set
val[0] = 0; then?

The __builtin_constant_p change affects whether we trigger this bug
only for fixed large precision instantiations (with trunk
__builtin_constant_p) or also by mistake for variable large precision
instantiations (with gcc 5 __builtin_constant_p), but the fixed large
precision instantiations are broken in any case.

2016-01-26  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/69399
	* wide-int.h (wi::lrshift): For larger precisions, only
	use fast path if shift is known to be < HOST_BITS_PER_WIDE_INT.

--- gcc/wide-int.h.jj	2016-01-04 14:55:50.000000000 +0100
+++ gcc/wide-int.h	2016-01-26 19:05:20.715269366 +0100
@@ -2909,7 +2909,9 @@ wi::lrshift (const T1 &x, const T2 &y)
 	 For variable-precision integers like wide_int, handle HWI
 	 and sub-HWI integers inline.  */
       if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
-	  ? xi.len == 1 && xi.val[0] >= 0
+	  ? (STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
+	     && xi.len == 1
+	     && xi.val[0] >= 0)
 	  : xi.precision <= HOST_BITS_PER_WIDE_INT)
 	{
 	  val[0] = xi.to_uhwi () >> shift;


	Jakub

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Mike Stump]

On Jan 26, 2016, at 10:21 AM, Jakub Jelinek <jakub@redhat.com> wrote
> The question is, shall we do what wi::lshift does and have the fast path
> only for the constant shifts, as the untested patch below does, or shall we
> check shift dynamically (thus use
> shift < HOST_BITS_PER_WIDE_INT
> instead of
> STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
> in the patch),

Hum…  I think I prefer the dynamic check.  The reasoning is that when we fast path, we can tolerate the conditional branch to retain the fast path, as most of the time, that condition will usually be true.  If the compiler had troubles with knowing the usual truth value of the expression, seems like we can hint that it will be true and influence the static prediction of the branch.  This permits us to fast path almost all the time in the non-constant, but small enough case.  For known shifts, there is no code gen difference, so it doesn’t matter.

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Jakub Jelinek]

On Tue, Jan 26, 2016 at 11:00:52AM -0800, Mike Stump wrote:
> On Jan 26, 2016, at 10:21 AM, Jakub Jelinek <jakub@redhat.com> wrote
> > The question is, shall we do what wi::lshift does and have the fast path
> > only for the constant shifts, as the untested patch below does, or shall we
> > check shift dynamically (thus use
> > shift < HOST_BITS_PER_WIDE_INT
> > instead of
> > STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
> > in the patch),
> 
> Hum…  I think I prefer the dynamic check.  The reasoning is that when we
> fast path, we can tolerate the conditional branch to retain the fast path,
> as most of the time, that condition will usually be true.  If the compiler
> had troubles with knowing the usual truth value of the expression, seems
> like we can hint that it will be true and influence the static prediction
> of the branch.  This permits us to fast path almost all the time in the
> non-constant, but small enough case.  For known shifts, there is no code
> gen difference, so it doesn’t matter.

Ok, I've cancelled my pending bootstrap/regtest and am testing this instead:

2016-01-26  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/69399
	* wide-int.h (wi::lrshift): For larger precisions, only
	use fast path if shift is known to be < HOST_BITS_PER_WIDE_INT.

--- gcc/wide-int.h.jj	2016-01-04 18:50:34.656471663 +0100
+++ gcc/wide-int.h	2016-01-26 20:07:03.147054988 +0100
@@ -2909,7 +2909,9 @@ wi::lrshift (const T1 &x, const T2 &y)
 	 For variable-precision integers like wide_int, handle HWI
 	 and sub-HWI integers inline.  */
       if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
-	  ? xi.len == 1 && xi.val[0] >= 0
+	  ? (shift < HOST_BITS_PER_WIDE_INT
+	     && xi.len == 1
+	     && xi.val[0] >= 0)
 	  : xi.precision <= HOST_BITS_PER_WIDE_INT)
 	{
 	  val[0] = xi.to_uhwi () >> shift;


	Jakub

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[H.J. Lu]

On Tue, Jan 26, 2016 at 11:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Jan 26, 2016 at 11:00:52AM -0800, Mike Stump wrote:
>> On Jan 26, 2016, at 10:21 AM, Jakub Jelinek <jakub@redhat.com> wrote
>> > The question is, shall we do what wi::lshift does and have the fast path
>> > only for the constant shifts, as the untested patch below does, or shall we
>> > check shift dynamically (thus use
>> > shift < HOST_BITS_PER_WIDE_INT
>> > instead of
>> > STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
>> > in the patch),
>>
>> Hum…  I think I prefer the dynamic check.  The reasoning is that when we
>> fast path, we can tolerate the conditional branch to retain the fast path,
>> as most of the time, that condition will usually be true.  If the compiler
>> had troubles with knowing the usual truth value of the expression, seems
>> like we can hint that it will be true and influence the static prediction
>> of the branch.  This permits us to fast path almost all the time in the
>> non-constant, but small enough case.  For known shifts, there is no code
>> gen difference, so it doesn’t matter.
>
> Ok, I've cancelled my pending bootstrap/regtest and am testing this instead:
>
> 2016-01-26  Jakub Jelinek  <jakub@redhat.com>
>
>         PR tree-optimization/69399
>         * wide-int.h (wi::lrshift): For larger precisions, only
>         use fast path if shift is known to be < HOST_BITS_PER_WIDE_INT.
>
> --- gcc/wide-int.h.jj   2016-01-04 18:50:34.656471663 +0100
> +++ gcc/wide-int.h      2016-01-26 20:07:03.147054988 +0100
> @@ -2909,7 +2909,9 @@ wi::lrshift (const T1 &x, const T2 &y)
>          For variable-precision integers like wide_int, handle HWI
>          and sub-HWI integers inline.  */
>        if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
> -         ? xi.len == 1 && xi.val[0] >= 0
> +         ? (shift < HOST_BITS_PER_WIDE_INT
> +            && xi.len == 1
> +            && xi.val[0] >= 0)
>           : xi.precision <= HOST_BITS_PER_WIDE_INT)
>         {
>           val[0] = xi.to_uhwi () >> shift;
>
>
>         Jakub

Can you add the testcase in PR 69399 to gcc.dg/torture?

Thanks.


-- 
H.J.

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Jan 26, 2016 at 11:00:52AM -0800, Mike Stump wrote:
>> On Jan 26, 2016, at 10:21 AM, Jakub Jelinek <jakub@redhat.com> wrote
>> > The question is, shall we do what wi::lshift does and have the fast path
>> > only for the constant shifts, as the untested patch below does, or shall we
>> > check shift dynamically (thus use
>> > shift < HOST_BITS_PER_WIDE_INT
>> > instead of
>> > STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
>> > in the patch),
>> 
>> Hum…  I think I prefer the dynamic check.  The reasoning is that when we
>> fast path, we can tolerate the conditional branch to retain the fast path,
>> as most of the time, that condition will usually be true.  If the compiler
>> had troubles with knowing the usual truth value of the expression, seems
>> like we can hint that it will be true and influence the static prediction
>> of the branch.  This permits us to fast path almost all the time in the
>> non-constant, but small enough case.  For known shifts, there is no code
>> gen difference, so it doesn’t matter.
>
> Ok, I've cancelled my pending bootstrap/regtest and am testing this instead:
>
> 2016-01-26  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR tree-optimization/69399
> 	* wide-int.h (wi::lrshift): For larger precisions, only
> 	use fast path if shift is known to be < HOST_BITS_PER_WIDE_INT.
>
> --- gcc/wide-int.h.jj	2016-01-04 18:50:34.656471663 +0100
> +++ gcc/wide-int.h	2016-01-26 20:07:03.147054988 +0100
> @@ -2909,7 +2909,9 @@ wi::lrshift (const T1 &x, const T2 &y)
>  	 For variable-precision integers like wide_int, handle HWI
>  	 and sub-HWI integers inline.  */
>        if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
> -	  ? xi.len == 1 && xi.val[0] >= 0
> +	  ? (shift < HOST_BITS_PER_WIDE_INT
> +	     && xi.len == 1
> +	     && xi.val[0] >= 0)
>  	  : xi.precision <= HOST_BITS_PER_WIDE_INT)
>  	{
>  	  val[0] = xi.to_uhwi () >> shift;

LGTM, thanks.

Richard

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Richard Biener]

On January 26, 2016 8:00:52 PM GMT+01:00, Mike Stump <mikestump@comcast.net> wrote:
>On Jan 26, 2016, at 10:21 AM, Jakub Jelinek <jakub@redhat.com> wrote
>> The question is, shall we do what wi::lshift does and have the fast
>path
>> only for the constant shifts, as the untested patch below does, or
>shall we
>> check shift dynamically (thus use
>> shift < HOST_BITS_PER_WIDE_INT
>> instead of
>> STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
>> in the patch),
>
>Hum…  I think I prefer the dynamic check.  The reasoning is that when
>we fast path, we can tolerate the conditional branch to retain the fast
>path, as most of the time, that condition will usually be true.  If the
>compiler had troubles with knowing the usual truth value of the
>expression, seems like we can hint that it will be true and influence
>the static prediction of the branch.  This permits us to fast path
>almost all the time in the non-constant, but small enough case.  For
>known shifts, there is no code gen difference, so it doesn’t matter.

The original reasoning was to inline only the fast path if it is known at compile time and otherwise have a call. Exactly to avoid bloating callers with inlined conditionals.

Richard.

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Jakub Jelinek]

On Tue, Jan 26, 2016 at 09:45:19PM +0100, Richard Biener wrote:
> On January 26, 2016 8:00:52 PM GMT+01:00, Mike Stump <mikestump@comcast.net> wrote:
> >On Jan 26, 2016, at 10:21 AM, Jakub Jelinek <jakub@redhat.com> wrote
> >> The question is, shall we do what wi::lshift does and have the fast
> >path
> >> only for the constant shifts, as the untested patch below does, or
> >shall we
> >> check shift dynamically (thus use
> >> shift < HOST_BITS_PER_WIDE_INT
> >> instead of
> >> STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
> >> in the patch),
> >
> >Hum…  I think I prefer the dynamic check.  The reasoning is that when
> >we fast path, we can tolerate the conditional branch to retain the fast
> >path, as most of the time, that condition will usually be true.  If the
> >compiler had troubles with knowing the usual truth value of the
> >expression, seems like we can hint that it will be true and influence
> >the static prediction of the branch.  This permits us to fast path
> >almost all the time in the non-constant, but small enough case.  For
> >known shifts, there is no code gen difference, so it doesn’t matter.
> 
> The original reasoning was to inline only the fast path if it is known at compile time and otherwise have a call. Exactly to avoid bloating callers with inlined conditionals.

I'm now also bootstrapping/regtesting following patch, the previous one
passed bootstrap/regtest, and will do cc1plus size comparison afterwards.
That said, I have done a quick check, where I believe that unless xi and
shift are both compile time constants, for the
STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
case there should be some comparison and the lrshift_large call with
the non-STATIC_CONSTANT_P variant, but in the bootstrapped (non-LTO)
cc1plus I only see 14 calls to lrshift_large, thus I bet it will likely affect
only <= 14 places right now:

0000000000776990 <_ZL11build_new_1PP3vecIP9tree_node5va_gc8vl_embedES1_S1_S6_bi>:
  777ca4:	e8 17 28 91 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
00000000008b8bc0 <_ZL31adjust_offset_for_component_refP9tree_nodePbPl.part.91>:
  8b8cc2:	e8 f9 17 7d 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000a7b550 <_ZN2wi7rrotateIPK9tree_node16generic_wide_intI16wide_int_storageEEENS_12unary_traitsIT_E11result_typeERKS8_RKT0_j>:
  a7baea:	e8 d1 e9 60 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
0000000000a7bd90 <_ZN2wi7lrotateIPK9tree_node16generic_wide_intI16wide_int_storageEEENS_12unary_traitsIT_E11result_typeERKS8_RKT0_j>:
  a7c457:	e8 64 e0 60 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000a7c710 <_ZN2wi7lrshiftIP9tree_nodelEENS_12unary_traitsIT_E11result_typeERKS4_RKT0_>:
  a7c851:	e8 6a dc 60 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000a7d280 <_ZN2wi7lrshiftIPK9tree_node16generic_wide_intI16wide_int_storageEEENS_12unary_traitsIT_E11result_typeERKS8_RKT0_>:
  a7d3b9:	e8 02 d1 60 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000cc1370 <_Z15real_to_integerPK10real_valuePbi>:
  cc1752:	e8 69 8d 3c 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000cc27f0 <_Z17real_from_integerP10real_value13format_helperRK16generic_wide_intI20wide_int_ref_storageILb0EEE6signop>:
  cc2f05:	e8 b6 75 3c 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000d6c420 <_Z31simplify_const_binary_operation8rtx_code12machine_modeP7rtx_defS2_>:
  d6f5a5:	e8 16 af 31 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000d7ca40 <_ZN2wi7lrotateISt4pairIP7rtx_def12machine_modeES5_EENS_12unary_traitsIT_E11result_typeERKS7_RKT0_j>:
  d7d17a:	e8 41 d3 30 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
0000000000d7d310 <_ZN2wi7rrotateISt4pairIP7rtx_def12machine_modeES5_EENS_12unary_traitsIT_E11result_typeERKS7_RKT0_j>:
  d7d99d:	e8 1e cb 30 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000ea3c40 <_ZN2wi7lrshiftI16generic_wide_intI22fixed_wide_int_storageILi192EEES4_EENS_12unary_traitsIT_E11result_typeERKS6_RKT0_>:
  ea3ca7:	e8 14 68 1e 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
0000000000f435a0 <_ZL27copy_reference_ops_from_refP9tree_nodeP3vecI22vn_reference_op_struct7va_heap6vl_ptrE>:
  f444b2:	e8 09 60 14 00       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>
--
000000000161c840 <_ZL21restructure_referencePP9tree_nodeS1_P16generic_wide_intI22fixed_wide_int_storageILi192EEES1_.constprop.133>:
 161cb51:	e8 6a d9 a6 ff       	callq  108a4c0 <_ZN2wi13lrshift_largeEPlPKljjjj>

2016-01-26  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/69399
	* wide-int.h (wi::lrshift): For larger precisions, only
	use fast path if shift is known to be < HOST_BITS_PER_WIDE_INT.

	* gcc.dg/torture/pr69399.c: New test.

--- gcc/wide-int.h.jj	2016-01-04 14:55:50.000000000 +0100
+++ gcc/wide-int.h	2016-01-26 19:05:20.715269366 +0100
@@ -2909,7 +2909,9 @@ wi::lrshift (const T1 &x, const T2 &y)
 	 For variable-precision integers like wide_int, handle HWI
 	 and sub-HWI integers inline.  */
       if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
-	  ? xi.len == 1 && xi.val[0] >= 0
+	  ? (STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT)
+	     && xi.len == 1
+	     && xi.val[0] >= 0)
 	  : xi.precision <= HOST_BITS_PER_WIDE_INT)
 	{
 	  val[0] = xi.to_uhwi () >> shift;
--- gcc/testsuite/gcc.dg/torture/pr69399.c.jj	2016-01-26 21:39:45.732927748 +0100
+++ gcc/testsuite/gcc.dg/torture/pr69399.c	2016-01-26 21:41:09.081753051 +0100
@@ -0,0 +1,18 @@
+/* { dg-do run { target int128 } } */
+
+static unsigned __attribute__((noinline, noclone))
+foo (unsigned long long u)
+{
+  unsigned __int128 v = u | 0xffffff81U;
+  v >>= 64;
+  return v;
+}
+
+int
+main ()
+{
+  unsigned x = foo (27);
+  if (x != 0)
+    __builtin_abort ();
+  return 0;
+}


	Jakub

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Mike Stump]

On Jan 26, 2016, at 12:45 PM, Richard Biener <richard.guenther@gmail.com> wrote:
> The original reasoning was to inline only the fast path if it is known at compile time and otherwise have a call. Exactly to avoid bloating callers with inlined conditionals.

That’s part of it.  And generally, yes, we do that.  One other part is, from the comment:

         For variable-precision integers like wide_int, handle HWI                                                                                           
         and sub-HWI integers inline.  */

Before, we just did the shift directly.  This _was_ the fast path.  We bulk the fast path case by a shift check, because we want to ensure it is small enough to be well defined.  The code previously was the fast path, because almost all shifts ever run will be HWI or smaller and will be 63 bits or less shifted.  If we make the check static only, then we boost the existing fast path out into the subroutine call.  Since the condition is most always true and since the predicate is in a register for the subroutine call anyway, the extra check should be fairly light (one instruction slot, zero execution time, zero dependencies, one BHT slot pollution).  With 34 call sites, that should be around an additional 136 bytes to the executable.  I think that space is paid for by the speed different of the fast path and how often it hits.  But, would be nice to have a person that loves benchmarking compile times and just tell us which one is faster.  :-)  Anyone skilled at knowing which one is faster is free to change to the other form.  My confidence that I know which one is faster is small; I’m happy to defer to someone that is more confident than I.

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Mike Stump]

On Jan 26, 2016, at 1:26 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> will do cc1plus size comparison afterwards.

We know the dynamic check is larger.  You can’t tell the advantage of speed from size.  Better would be to time compiling any random large translation unit.

Nice to see that only 14 calls remain, that’s way better than the 34 I thought.

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Jakub Jelinek]

On Tue, Jan 26, 2016 at 01:55:41PM -0800, Mike Stump wrote:
> On Jan 26, 2016, at 1:26 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > will do cc1plus size comparison afterwards.
> 
> We know the dynamic check is larger.  You can’t tell the advantage of
> speed from size.  Better would be to time compiling any random large
> translation unit.
> 
> Nice to see that only 14 calls remain, that’s way better than the 34 I
> thought.

So, it seems probably the PR65656 changes made things actually significantly
worse, while I see a (small) difference in the generated code between the two
patches if I compile say tree-ssa-ccp.c with g++ 5.x, in the bootstrapped
compiler there is no difference at all, the compilers with either patch
have identical objdump -dr cc1plus.  Already at *.gimple time all the
relevant __builtin_constant_p are resolved and it seems all to 0.

So please agree on one of the two patches (don't care which), and I'll try
to distill a testcase to look at for PR65656.

	Jakub

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Richard Biener]

On Wed, Jan 27, 2016 at 12:41 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Jan 26, 2016 at 01:55:41PM -0800, Mike Stump wrote:
>> On Jan 26, 2016, at 1:26 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > will do cc1plus size comparison afterwards.
>>
>> We know the dynamic check is larger.  You can’t tell the advantage of
>> speed from size.  Better would be to time compiling any random large
>> translation unit.
>>
>> Nice to see that only 14 calls remain, that’s way better than the 34 I
>> thought.
>
> So, it seems probably the PR65656 changes made things actually significantly
> worse, while I see a (small) difference in the generated code between the two
> patches if I compile say tree-ssa-ccp.c with g++ 5.x, in the bootstrapped
> compiler there is no difference at all, the compilers with either patch
> have identical objdump -dr cc1plus.  Already at *.gimple time all the
> relevant __builtin_constant_p are resolved and it seems all to 0.
>
> So please agree on one of the two patches (don't care which), and I'll try
> to distill a testcase to look at for PR65656.

I don't care if the wi::lshift by LOG2_BITS_PER_UNIT done by
get_ref_base_and_extent
are compiled to as good quality as before (I suspect it doesn't matter
in this case
as the shift amount is constant, but maybe due to PR65656 the
non-STATIC_CONSTANT_P
variant is better).

Richard.

>         Jakub

Re: [PATCH] Fix up wi::lrshift (PR c++/69399)

[Jakub Jelinek]

On Wed, Jan 27, 2016 at 11:38:33AM +0100, Richard Biener wrote:
> On Wed, Jan 27, 2016 at 12:41 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Tue, Jan 26, 2016 at 01:55:41PM -0800, Mike Stump wrote:
> >> On Jan 26, 2016, at 1:26 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> >> > will do cc1plus size comparison afterwards.
> >>
> >> We know the dynamic check is larger.  You can’t tell the advantage of
> >> speed from size.  Better would be to time compiling any random large
> >> translation unit.
> >>
> >> Nice to see that only 14 calls remain, that’s way better than the 34 I
> >> thought.
> >
> > So, it seems probably the PR65656 changes made things actually significantly
> > worse, while I see a (small) difference in the generated code between the two
> > patches if I compile say tree-ssa-ccp.c with g++ 5.x, in the bootstrapped
> > compiler there is no difference at all, the compilers with either patch
> > have identical objdump -dr cc1plus.  Already at *.gimple time all the
> > relevant __builtin_constant_p are resolved and it seems all to 0.
> >
> > So please agree on one of the two patches (don't care which), and I'll try
> > to distill a testcase to look at for PR65656.
> 
> I don't care if the wi::lshift by LOG2_BITS_PER_UNIT done by
> get_ref_base_and_extent
> are compiled to as good quality as before (I suspect it doesn't matter
> in this case
> as the shift amount is constant, but maybe due to PR65656 the
> non-STATIC_CONSTANT_P
> variant is better).

Ok, I'll commit the non-STATIC_CONSTANT_P variant for now, we can revisit it
later.  I have distilled a testcase for Jason in PR65656.  It seems the
problematic STATIC_CONSTANT_P is only if it is inside of the condition of
?: expression.  So we could work around it by using something like:
-      if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
-          ? (STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT - 1)
-             && xi.len == 1
-             && xi.val[0] <= (HOST_WIDE_INT) ((unsigned HOST_WIDE_INT)
-                                              HOST_WIDE_INT_MAX >> shift))
-          : precision <= HOST_BITS_PER_WIDE_INT)
+      bool fast_path = false;
+      if (STATIC_CONSTANT_P (xi.precision > HOST_BITS_PER_WIDE_INT)
+        {
+          if (STATIC_CONSTANT_P (shift < HOST_BITS_PER_WIDE_INT - 1)
+              && xi.len == 1
+              && xi.val[0] <= (HOST_WIDE_INT) ((unsigned HOST_WIDE_INT)
+                                               HOST_WIDE_INT_MAX >> shift))
+            fast_path = true;
+        }
+      else if (precision <= HOST_BITS_PER_WIDE_INT)
+        fast_path = true;
+      if (fast_path)
        {
          val[0] = xi.ulow () << shift;
          result.set_len (1);
        }
      else
        result.set_len (lshift_large (val, xi.val, xi.len,
                                      precision, shift));
and similarly in lrshift.

	Jakub