Re: [PATCH] Fix PR tree-optimization/77550

[Richard Biener]

On Mon, 19 Sep 2016, Bernd Edlinger wrote:

> On 09/19/16 11:25, Richard Biener wrote:
> > On Sun, 18 Sep 2016, Bernd Edlinger wrote:
> >
> >> Hi,
> >>
> >> this PR shows that in vectorizable_store and vectorizable_load
> >> as well, the vector access always uses the first dr as the alias
> >> type for the whole access.  But that is not right, if they are
> >> different types, like in this example.
> >>
> >> So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
> >> by an alias type that is correct for all references in the whole
> >> access group.  With this patch we fall back to ptr_type_node, which
> >> can alias anything, if the group consists of different alias sets.
> >>
> >>
> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> >> Is it OK for trunk and gcc-6-branch?
> >
> > +/* Function get_group_alias_ptr_type.
> > +
> > +   Return the alias type for the group starting at FIRST_STMT
> > +   containing GROUP_SIZE elements.  */
> > +
> > +static tree
> > +get_group_alias_ptr_type (gimple *first_stmt, int group_size)
> > +{
> > +  struct data_reference *first_dr, *next_dr;
> > +  gimple *next_stmt;
> > +  int i;
> > +
> > +  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
> > +  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
> > +  for (i = 1; i < group_size && next_stmt; i++)
> > +{
> >
> >
> > there is no need to pass in group_size, it's enough to walk
> > GROUP_NEXT_ELEMENT until it becomes NULL.
> >
> > Ok with removing the redundant arg.
> >
> > Thanks,
> > Richard.
> >
> 
> Hmmm, I'm afraid this needs one more iteration.
> 
> 
> I tried first to check if there are no stmts after the group_size
> and noticed there are cases when group_size is 0,
> for instance in gcc.dg/torture/pr36244.c.
> 
> I think there is a bug in vectorizable_load, here:
> 
>   if (grouped_load)
> {
>   first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
>/* For SLP vectorization we directly vectorize a subchain
>   without permutation.  */
>if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
>  first_stmt = ;
> 
>group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt));
> 
>= 0, and even worse:
> 
>group_gap_adj = vf * group_size - nunits * vec_num;
> 
>= -4 !
> 
> apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT,

Yes.  I'm not sure group_size or group_gap_adj are used in the
slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists () case but moving
the computation up before we re-set first_stmt is probably a good idea.

> while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0]
> 
> moving the GROUP_SIZE up before first_stmt is overwritten
> results in no different code

See above - it's eventually unused.  The load/store vectorization code
is quite twisted ;)

Re: [PATCH] Fix ICE with __atomic_{always,is}_lock_free (PR middle-end/77624)

[Richard Biener]

On Tue, 20 Sep 2016, Jakub Jelinek wrote:

> Hi!
> 
> fold_builtin_atomic_always_lock_free has an assertion that if the 2nd
> argument isn't INTEGER_CST, it has POINTER_TYPE_P, but additionally for
> casts to void * it looks through the cast.  That means though for invalid
> code if an integral expression etc. is converted to pointer, we violate the
> assertion.  This patch instead doesn't look through such casts, so
> type_align is effectively alignment of void and thus it considers objects
> insufficiently aligned, but doesn't ICE.
> 
> Another approach (incomplete) is what I've attached to the PR - instead of
> prototyping __atomic_{always,is}_lock_free it uses ... and only checks that
> it has 2 arguments and that the last one is a pointer - that way we don't
> have to try to undo implicit cast to void * and know the conversion in there
> is the user conversion.  Unfortunately the C++ FE isn't prepared for the
> check_builtin_function_arguments to change the arguments (unlike C).
> 
> I've bootstrapped/regtested on x86_64-linux and i686-linux the following
> fix, ok for trunk, or should I invest more time into the other approach?

Ok for trunk.  I think the other solution would be to somehow mark the
builtin to not get the (void *) cast applied in the first place ...
(make it varargs?)

Thanks,
Richard.

> 2016-09-19  Jakub Jelinek  
> 
>   PR middle-end/77624
>   * builtins.c (fold_builtin_atomic_always_lock_free): Only look through
>   cast to void * if the cast is from some other pointer type.
> 
>   * c-c++-common/pr77624-1.c: New test.
>   * c-c++-common/pr77624-2.c: New test.
> 
> --- gcc/builtins.c.jj 2016-09-16 22:19:38.0 +0200
> +++ gcc/builtins.c2016-09-19 15:28:41.100412521 +0200
> @@ -5575,8 +5575,10 @@ fold_builtin_atomic_always_lock_free (tr
>end before anything else has a chance to look at it.  The pointer
>parameter at this point is usually cast to a void *, so check for that
>and look past the cast.  */
> -  if (CONVERT_EXPR_P (arg1) && POINTER_TYPE_P (ttype)
> -   && VOID_TYPE_P (TREE_TYPE (ttype)))
> +  if (CONVERT_EXPR_P (arg1)
> +   && POINTER_TYPE_P (ttype)
> +   && VOID_TYPE_P (TREE_TYPE (ttype))
> +   && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (arg1, 0
>   arg1 = TREE_OPERAND (arg1, 0);
>  
>ttype = TREE_TYPE (arg1);
> --- gcc/testsuite/c-c++-common/pr77624-1.c.jj 2016-09-19 15:18:09.049394093 
> +0200
> +++ gcc/testsuite/c-c++-common/pr77624-1.c2016-09-19 15:41:09.972949880 
> +0200
> @@ -0,0 +1,14 @@
> +/* PR middle-end/77624 */
> +/* { dg-do compile } */
> +
> +int
> +foo (int a)
> +{
> +  return __atomic_is_lock_free (2, a);   /* { dg-warning 
> "pointer from integer" "" { target c } } */
> +}/* { dg-error "invalid 
> conversion" "" { target c++ } 7 } */
> +
> +int
> +bar (int a)
> +{
> +  return __atomic_always_lock_free (2, a);   /* { dg-warning "pointer from 
> integer" "" { target c } } */
> +}/* { dg-error "invalid 
> conversion" "" { target c++ } 13 } */
> --- gcc/testsuite/c-c++-common/pr77624-2.c.jj 2016-09-19 15:18:12.280353292 
> +0200
> +++ gcc/testsuite/c-c++-common/pr77624-2.c2016-09-19 15:42:37.441844581 
> +0200
> @@ -0,0 +1,26 @@
> +/* PR middle-end/77624 */
> +/* { dg-do compile } */
> +
> +void
> +foo (int *a)
> +{
> +  double b = 0;
> +  __atomic_is_lock_free (2, a, 2);   /* { dg-error "too many arguments" } */
> +  __atomic_is_lock_free (2); /* { dg-error "too few arguments" } */
> +  __atomic_is_lock_free (2, b);  /* { dg-error "incompatible 
> type" "" { target c } } */
> + /* { dg-message "expected" "" { target 
> c } 10 } */
> + /* { dg-error "convert" "" { target c++ 
> } 10 } */
> +  __atomic_is_lock_free (2, 0);
> +}
> +
> +void
> +bar (int *a)
> +{
> +  double b = 0;
> +  __atomic_always_lock_free (2, a, 2);   /* { dg-error "too many 
> arguments" } */
> +  __atomic_always_lock_free (2); /* { dg-error "too few arguments" } */
> +  __atomic_always_lock_free (2, b);  /* { dg-error "incompatible type" "" { 
> target c } } */
> + /* { dg-message "expected" "" { target 
> c } 22 } */
> + /* { dg-error "convert" "" { target c++ 
> } 22 } */
> +  __atomic_always_lock_free (2, 0);
> +}
> 
>   Jakub
> 
> 

-- 
Richard Biener 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Andreas Schwab]

In file included from ./tm.h:25:0,
 from ../../gcc/target.h:52,
 from ../../gcc/cp/typeck.c:30:
../../gcc/cp/typeck.c: In function 'tree_node* 
get_member_function_from_ptrfunc(tree_node**, tree, tsubst_flags_t)':
../../gcc/config/ia64/ia64.h:224:54: error: ?: using integer constants in 
boolean context [-Werror=int-in-bool-context]
 #define TARGET_VTABLE_USES_DESCRIPTORS (TARGET_ILP32 ? 4 : 2)
~~^~~~
../../gcc/cp/typeck.c:3441:11: note: in expansion of macro 
'TARGET_VTABLE_USES_DESCRIPTORS'
   if (TARGET_VTABLE_USES_DESCRIPTORS)
   ^~

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Early jump threading

[Thomas Preudhomme]

On 19/09/16 08:02, Christophe Lyon wrote:

Index: testsuite/gcc.dg/tree-ssa/pr68198.c
===
--- testsuite/gcc.dg/tree-ssa/pr68198.c (revision 240109)
+++ testsuite/gcc.dg/tree-ssa/pr68198.c (working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -fdump-tree-thread1-details" } */
+/* { dg-options "-O2 -fdump-tree-thread1-details -fdisable-tree-ethread" } */

 extern void abort (void);

@@ -40,4 +40,4 @@ c_finish_omp_clauses (tree clauses)
 /* There are 3 FSM jump threading opportunities, two of which will
   get filtered out.  */
 /* { dg-final { scan-tree-dump-times "Registering FSM" 1 "thread1"} } */
-/* { dg-final { scan-tree-dump-times "FSM Thread through multiway branch without threading a 
multiway branch" 2 "thread1"} } */
+/* { dg-final { scan-tree-dump-times "FSM Thread through multiway branch without threading a 
multiway branch" 2 "ethread"} } */


This test does not work, at least on arm* and aarch64*. I'm seeing:
cc1: note: disable pass tree-ethread for functions in the range of [0,
4294967295]

PASS: gcc.dg/tree-ssa/pr68198.c (test for excess errors)
PASS: gcc.dg/tree-ssa/pr68198.c scan-tree-dump-times thread1 "Registering FSM" 1
gcc.dg/tree-ssa/pr68198.c: dump file does not exist
UNRESOLVED: gcc.dg/tree-ssa/pr68198.c scan-tree-dump-times ethread
"FSM Thread through multiway branch without threading a multiway
branch" 2


Shouldn't the test enable tree-ethread to have a ethread dump?

Best regards,

Thomas

Re: Re: [PATCH, 5.x/6.x/7.x] Be more conservative in early inliner if FDO is enabled

[Yuan, Pengfei]

> > > > I also like a new param better as it avoids a new magic constant and
> > > > makes playing with
> > > > it easier (your patch removes the ability to do statistics like you did 
> > > > via the
> > > > early-inlining-insns parameter by forcing it to two).
> > > 
> > > Hmm, you are right that you do not know if this particular function will 
> > > get
> > > profile (forgot about that).  Still, please use two params - it is more
> > > consistent with what we have now and we may make it profile specific in
> > > future..
> > > 
> > > Honza
> > > > 
> > > > Thanks,
> > > > Richard.
> > 
> > A new patch for trunk is attached.
> > 
> > Regards,
> > Yuan, Pengfei
> > 
> > 
> > 2016-09-16  Yuan Pengfei  
> > 
> > * doc/invoke.texi (--param early-inlining-insns-feedback): New.
> > * ipa-inline.c (want_early_inline_function_p): Use
> > PARAM_EARLY_INLINING_INSNS_FEEDBACK when FDO is enabled.
> > * params.def (PARAM_EARLY_INLINING_INSNS_FEEDBACK): Define.
> > (PARAM_EARLY_INLINING_INSNS): Change help string accordingly.
> 
> OK,
> thanks
> 
> Honza

Do I have to sign an FSF copyright assignment to get this patch applied?

Regards,
Yuan, Pengfei

Re: [PATCH] libstdc++/77645 fix deque and vector xmethods for Python 3

[Jonathan Wakely]

On 19/09/16 21:39 +, Joseph Myers wrote:

On Mon, 19 Sep 2016, Jonathan Wakely wrote:


On 19/09/16 17:24 +, Joe Buck wrote:
> Python has a distinct integer division operator, "//".  7 // 3 returns the
> integer 2.

Python 3 does, but Python 2 doesn't have it unless you import it from
__future, and I don't know how far back that works. I don't want to
introduce a fix for Python 3 that breaks it for old systems.


No, // is available in Python, without needing __future__, from 2.2
onwards.


I stand corrected, thanks both of you.

I'll test and commnit this then.


commit 07345154582af68b50866cbe9b90432f2d45
Author: Jonathan Wakely 
Date:   Tue Sep 20 09:38:49 2016 +0100

Use python floored quotient operator in xmethods

	* python/libstdcxx/v6/xmethods.py (DequeWorkerBase.__init__)
	(DequeWorkerBase.index, VectorWorkerBase.get): Use // for division.

diff --git a/libstdc++-v3/python/libstdcxx/v6/xmethods.py b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
index 71a5b75..ed6a111 100644
--- a/libstdc++-v3/python/libstdcxx/v6/xmethods.py
+++ b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
@@ -165,7 +165,7 @@ class ArrayMethodsMatcher(gdb.xmethod.XMethodMatcher):
 class DequeWorkerBase(gdb.xmethod.XMethodWorker):
 def __init__(self, val_type):
 self._val_type = val_type
-self._bufsize = int(512 / val_type.sizeof) or 1
+self._bufsize = 512 // val_type.sizeof or 1
 
 def size(self, obj):
 first_node = obj['_M_impl']['_M_start']['_M_node']
@@ -176,7 +176,7 @@ class DequeWorkerBase(gdb.xmethod.XMethodWorker):
 
 def index(self, obj, idx):
 first_node = obj['_M_impl']['_M_start']['_M_node']
-index_node = first_node + int(idx / self._bufsize)
+index_node = first_node + idx // self._bufsize
 return index_node[0][idx % self._bufsize]
 
 class DequeEmptyWorker(DequeWorkerBase):
@@ -419,7 +419,7 @@ class VectorWorkerBase(gdb.xmethod.XMethodWorker):
 if self._val_type.code == gdb.TYPE_CODE_BOOL:
 start = obj['_M_impl']['_M_start']['_M_p']
 bit_size = start.dereference().type.sizeof * 8
-valp = start + int(index / bit_size)
+valp = start + index // bit_size
 offset = index % bit_size
 return (valp.dereference() & (1 << offset)) > 0
 else:

Re: debug container mutex association

[Jonathan Wakely]

On 19/09/16 21:56 +0200, François Dumont wrote:

Hi

   Following our conversation here is a much simpler patch. I just 
consider that all debug containers will have the same alignment.


   Even if I submit this patch as a whole I will commit into pieces, 
at least one for the pure cleanup parts and one for the debug.cc 
change.


   Among those changes there is:
-   __gnu_cxx::__scoped_lock(this->_M_get_mutex());
+   __gnu_cxx::__scoped_lock __l(this->_M_get_mutex());

   I would appreciate if you could tell me what was happening with 
the initial expression. I don't understand why it is compiling. I even 
tried the same in debug.cc and started having compilation errors.


It creates a temporary __scoped_lock, which immediately goes out of
scope and unlocks the mutex again. This should be fixed on the gcc-5
and gcc-6 branches too.

I'll review the rest of the patch today.

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00429.html

Thanks,
Kyrill

On 08/09/16 12:00, Kyrill Tkachov wrote:

Hi all,

This is a rebase and slight respin of [1] that I sent out last November to 
change all uses of
sprintf to snprintf in the arm backend.

Bootstrapped and tested on arm-none-linux-gnueabihf.
Ok for trunk?

Thanks,
Kyrill

[1] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html

2016-09-08  Kyrylo Tkachov  

* config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
rather than sprintf.
(arm_set_fixed_conv_libfunc): Likewise.
(arm_option_override): Likewise.
(neon_output_logic_immediate): Likewise.
(neon_output_shift_immediate): Likewise.
(arm_output_multireg_pop): Likewise.
(vfp_output_vstmd): Likewise.
(output_move_vfp): Likewise.
(output_move_neon): Likewise.
(output_return_instruction): Likewise.
(arm_elf_asm_cdtor): Likewise.
(arm_output_shift): Likewise.
(arm_output_iwmmxt_shift_immediate): Likewise.
(arm_output_iwmmxt_tinsr): Likewise.
* config/arm/neon.md (*neon_mov, VDX): Likewise.
(*neon_mov, VQXMOV): Likewise.
(neon_vc_insn): Likewise.
(neon_vc_insn_unspec): Likewise.

Re: debug container mutex association

[Jonathan Wakely]

On 19/09/16 21:56 +0200, François Dumont wrote:

Hi

   Following our conversation here is a much simpler patch. I just 
consider that all debug containers will have the same alignment.


   Even if I submit this patch as a whole I will commit into pieces, 
at least one for the pure cleanup parts and one for the debug.cc 
change.


   Among those changes there is:
-   __gnu_cxx::__scoped_lock(this->_M_get_mutex());
+   __gnu_cxx::__scoped_lock __l(this->_M_get_mutex());

   I would appreciate if you could tell me what was happening with 
the initial expression. I don't understand why it is compiling. I even 
tried the same in debug.cc and started having compilation errors.


   * include/debug/bitset (bitset::reference::reference(const _Base_ref&,
   bitset*)): Remove __unused__ attribute.
   * include/debug/safe_base.h (_Safe_iterator_base): Make
   _Safe_sequence_base a friend.
   (_Safe_iterator_base::_M_attach): Make protected.
   (_Safe_iterator_base::_M_attach_single): Likewise.
   (_Safe_iterator_base::_M_detach): Likewise.
   (_Safe_iterator_base::_M_detach_single): Likewise.
   (_Safe_sequence_base): Make _Safe_iterator_base a friend.
(_Safe_sequence_base::_Safe_sequence_base(_Safe_sequence_base&&)): New.
   (_Safe_sequence_base::_M_swap): Make protected.
   (_Safe_sequence_base::_M_attach): Make private.
   (_Safe_sequence_base::_M_attach_single): Likewise.
   (_Safe_sequence_base::_M_detach): Likewise.
   (_Safe_sequence_base::_M_detach_single): Likewise.
   * include/debug/safe_container.h
   (_Safe_container::_Safe_container(_Safe_container&&)): Make default.
   * include/debug/safe_iterator.h
   (_Safe_iterator::operator++()): Name __scoped_lock instance.
   * include/debug/safe_iterator.tcc: Remove trailing line.
   * include/debug/safe_unordered_base.h
   (_Safe_local_iterator_base::_M_attach): Make protected.
   (_Safe_local_iterator_base::_M_attach_single): Likewise.
   (_Safe_local_iterator_base::_M_detach): Likewise.
   (_Safe_local_iterator_base::_M_detach_single): Likewise.
   (_Safe_unordered_container_base): Make _Safe_local_iterator_base 
friend.

   (_Safe_unordered_container_base::_M_attach_local): Make private.
   (_Safe_unordered_container_base::_M_attach_local_single): Likewise.
   (_Safe_unordered_container_base::_M_detach_local): Likewise.
   (_Safe_unordered_container_base::_M_detach_local_single): Likewise.
   * src/c++11/debug.cc: Include debug/vector. Include cctype. Remove
   functional.
   (get_safe_base_mutex): Get mutex based on address lowest non nil bits.
   * testsuite/23_containers/vector/debug/mutex_association.cc: New.

Tested under Linux x86_64.

Ok to commit ?


Yes, OK for trunk.  Thanks for revising it, I think this is a much
bettter fix.



On 15/09/2016 10:51, Jonathan Wakely wrote:

N.B. https://gcc.gnu.org/PR71312
Ok, debug mode is also impacted. Shouldn't the alignment be set on 
the __mutex type directly ?


No, definitely not. Apart from the fact that it would be an ABI
change, it would mean that struct X { __mutex mx; int i; } would not
place the int right next to the mutex, it would force it onto a
different cacheline. We want our arrays of mutexes to be on separate
cachelines, but most uses of __mutex are not in an array.

We probably can't fix this properly yet, because we don't have the
hardware_destructive_interference_size value. We could just make a
conservative estimate of 64 bytes though.

Thanks for explaining that it is to avoid false sharing.

Maybe we could share this mutex pool with debug mode. This way the day 
we fix this false sharing issue it will benefit to both shared_ptr and 
debug mode.


Yes, that would reduce the size of the library, by not having two
arrays of mutexes. Currently the arrays are not visible outside their
own file, but that's not too hard to solve.

Re: [GCC][PATCH] Add __artificial__ attribute to Aarch64 NEON intrinsics

[James Greenhalgh]

On Tue, Sep 13, 2016 at 01:15:39PM +0100, Tamar Christina wrote:
> 
> 
> On 05/09/16 22:37, Andrew Pinski wrote:
> >On Mon, Sep 5, 2016 at 4:53 AM, Tamar Christina  
> >wrote:
> >>Hi all,
> >>
> >>This patch adds __artificial__ attribute to the intrinsics
> >>in arm_neon.h so that costs are associated to the user
> >>function during profiling and during the intrinsics are
> >>are hidden in trace.
> >>
> >>The attribute does not affect code generation.
> >>
> >>No new tests for this since it would require a gdb test
> >>but regression tests on aarch64-none-elf was performed.
> >I think this is obvious.  Note I would change it one more step to be:
> >__extension__ extern__inline TYPE
> >__attribute__  ((__always_inline__, __gnu_inline__, __artificial__))
> >
> >The main reason is because in C++98 (and I can't remember if C++11),
> >template are not supposed to consider a static function being a
> >candidate (though GCC does not get it right is no reason why not to
> >change it).
> 
> Hi Andrew,
> 
> I've updated the script and patch. New script is:
> 
> #!/bin/bash
> 
> # first apply to the ones in #define blocks and add extra \ at the end
> sed -i -r
> 's/(__inline.+)(__attribute__\s*)\(\((.+)\)\)\s*\\/\1\\\n\2 \(\(\3,
> __gnu_inline__,__artificial__\)\) \\/m' \
>  gcc/config/aarch64/arm_neon.h
> 
> # Then write all normal functions
> sed -i -r 's/(__inline.+)(__attribute__\s*)\(\((.+)\)\)/\1\n\2
> \(\(\3, __gnu_inline__, __artificial__\)\)/m' \
>  gcc/config/aarch64/arm_neon.h
> 
> # Then correct any trailing whitespaces we might have introduced
> sed -i 's/[ \t]*$//' \
>  gcc/config/aarch64/arm_neon.h
> 
> # And then finish up by correcting some attribute values which don't
> fit the patterns above.
> sed -i -r
> 's/(__attribute__\s+)\(\((__always_inline__)\)\)\s+\\/\1\(\(\2,
> __gnu_inline__, __artificial__\)\) \\/m' \
>  gcc/config/aarch64/arm_neon.h
> 
> # Replace static definitions with extern
> sed -i -r 's/(__extension__\s+)static(.+)/\1extern\2/m' \
>  gcc/config/aarch64/arm_neon.h
> 
> I've also update the patch.

Thanks, I applied this following your script above, and committed it as
revision 240256, after tweaking one part of the script to replace a double
space after __attribute__ introduced by the second sed command.

Thanks,
James

Re: [PATCH, GCC/LRA] Teach LRA to not use same register value for multiple output operands of an insn

[Thomas Preudhomme]

Hi Vladimir,

I'm sorry I failed to give a proper amount of context in that email. You already 
approved that patch for trunk one or two months ago and I was just replying to 
your approval to ask if you are now ok to commit it to gcc-6-branch.


I've committed it now then since it has been on trunk for long enough ;-)

Best regards.

Thomas

On 19/09/16 16:05, Vladimir N Makarov wrote:



On 07/08/2016 11:07 AM, Thomas Preudhomme wrote:

Hi,

While investigating the root cause a testsuite regression for the
ARM/embedded-5-branch GCC in gcc.dg/vect/slp-perm-5.c, we found that the bug
seems to also affect trunk. The bug manifests itself as an ICE in cselib due to
a parallel insn with two SET to the same register. When processing the second
SET in cselib_record_set (), the assert gcc_assert (REG_VALUES (dreg)->elt ==
0) fails because the field was already set when processing the first SET. The
root cause seems to be a register allocation issue in lra-constraints.

When considering an output operand with matching input operand(s),
match_reload does a number of checks to see if it can reuse the first matching
input operand register value or if a new unique value should be given to the
output operand. The current check ignores the case of multiple output operands
(as in neon_vtrn_insn insn pattern in config/arm/arm.md). This can lead
to cases where multiple output operands share a same register value when the
first matching input operand has the same value as another output operand,
leading to the ICE in cselib. This patch changes match_reload to get
information about other output operands and check whether this case is met or
not.

ChangeLog entry is as follows:

*** gcc/ChangeLog ***

2016-07-01  Thomas Preud'homme  

 * lra-constraints.c (match_reload): Pass information about other
 output operands.  Create new unique register value if matching input
 operand shares same register value as output operand being considered.
 (curr_insn_transform): Record output operands already processed.


Patch passed bootstrap under arm-none-linux-gnueabihf (Cortex-A57 in ARM mode
as well as Thumb mode), aarch64-linux-gnu (Cortex-A57) and x86_64-linux-gnu
and testsuite results does not regress for these and for arm-none-eabi
targeting Cortex-A8.

Is this ok for trunk?



Yes, Thomas.  I tried to find an alternative solution but your approach is
probably better.  Your patch is also ok for gcc-6 branch.  I'd delay its commit
to gcc-6 branch for a few days to see how it behaves on the trunk.  Although I
don't expect any troubles as the patch is quite safe with my point of view.

Thank you for the patch and sorry for the delay with the answer.

Report DejaGnu ERROR messages in compare_tests

[Christophe Lyon]

Hello,

We recently faced a problem where a DejaGnu error went un-noticed
(https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01879.html).

To help identify these problems earlier, here is a patch for
compare_tests that will report such cases as:

New tests that FAIL:

arm-sim: (DejaGnu) proc "scan-dump-tree-not fail_test optimized" does not exist.


OK?

Thanks,

Christophe
contrib/ChangeLog:

2016-09-20  Christophe Lyon  

* compare_tests: Take ERROR messages into account when
  comparing.
diff --git a/contrib/compare_tests b/contrib/compare_tests
index 8ecf824..d16e7e9 100755
--- a/contrib/compare_tests
+++ b/contrib/compare_tests
@@ -107,8 +107,8 @@ elif [ -d "$1" -o -d "$2" ] ; then
usage "Must specify either two directories or two files"
 fi
 
-sed 's/^XFAIL/FAIL/; s/^XPASS/PASS/' < "$1" | awk '/^Running target / {target 
= $3} { if (target != "unix") { sub(/: /, "&"target": " ); }; print $0; }' | 
cut -c1-2000 >$tmp1
-sed 's/^XFAIL/FAIL/; s/^XPASS/PASS/' < "$2" | awk '/^Running target / {target 
= $3} { if (target != "unix") { sub(/: /, "&"target": " ); }; print $0; }' | 
cut -c1-2000 >$tmp2
+sed 's/^XFAIL/FAIL/; s/^ERROR/FAIL/; s/^XPASS/PASS/' < "$1" | awk '/^Running 
target / {target = $3} { if (target != "unix") { sub(/: /, "&"target": " ); }; 
print $0; }' | cut -c1-2000 >$tmp1
+sed 's/^XFAIL/FAIL/; s/^ERROR/FAIL/; s/^XPASS/PASS/' < "$2" | awk '/^Running 
target / {target = $3} { if (target != "unix") { sub(/: /, "&"target": " ); }; 
print $0; }' | cut -c1-2000 >$tmp2
 
 before=$tmp1
 now=$tmp2

Report DejaGnu ERROR messages in dg-extract-results

[Christophe Lyon]

Hello,

We recently faced a problem where a DejaGnu error went un-noticed
(https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01879.html).

To help identify these problems earlier, here is a patch for
dg-extract-results that makes such error obvious in the final summary:

=== gcc Summary ===

# of DejaGnu errors 1
# of expected passes104885
# of unexpected failures43
# of unexpected successes   2
# of expected failures  276
# of unsupported tests  2284


OK?

Thanks,

Christophe
contrib/ChangeLog:

2016-09-20  Christophe Lyon  

* dg-extract-results.py: Report DejaGnu error in the final
summary.
* dg-extract-results.sh: Likewise.

diff --git a/contrib/dg-extract-results.py b/contrib/dg-extract-results.py
index 7db5e64..4b02a5b 100644
--- a/contrib/dg-extract-results.py
+++ b/contrib/dg-extract-results.py
@@ -134,6 +134,7 @@ class Prog:
 self.end_line = None
 # Known summary types.
 self.count_names = [
+'# of DejaGnu errors\t\t',
 '# of expected passes\t\t',
 '# of unexpected failures\t',
 '# of unexpected successes\t',
@@ -245,6 +246,10 @@ class Prog:
 segment = Segment (filename, file.tell())
 variation.header = segment
 
+# Parse the rest of the summary (the '# of ' lines).
+if len (variation.counts) == 0:
+variation.counts = self.zero_counts()
+
 # Parse up until the first line of the summary.
 if num_variations == 1:
 end = '\t\t=== ' + tool.name + ' Summary ===\n'
@@ -291,6 +296,11 @@ class Prog:
 harness.results.append ((key, line))
 if not first_key and sort_logs:
 first_key = key
+if line.startswith ('ERROR: (DejaGnu)'):
+for i in range (len (self.count_names)):
+if 'DejaGnu errors' in self.count_names[i]:
+variation.counts[i] += 1
+break
 
 # 'Using ...' lines are only interesting in a header.  Splitting
 # the test up into parallel runs leads to more 'Using ...' lines
@@ -309,9 +319,6 @@ class Prog:
 segment.lines -= final_using
 harness.add_segment (first_key, segment)
 
-# Parse the rest of the summary (the '# of ' lines).
-if len (variation.counts) == 0:
-variation.counts = self.zero_counts()
 while True:
 before = file.tell()
 line = file.readline()
diff --git a/contrib/dg-extract-results.sh b/contrib/dg-extract-results.sh
index 5a8e67e..519d49c 100755
--- a/contrib/dg-extract-results.sh
+++ b/contrib/dg-extract-results.sh
@@ -369,10 +369,11 @@ EOF
 BEGIN {
   variant="$VAR"
   tool="$TOOL"
-  passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kpasscnt=0; 
kfailcnt=0; unsupcnt=0; unrescnt=0;
+  passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kpasscnt=0; 
kfailcnt=0; unsupcnt=0; unrescnt=0; dgerrorcnt=0;
   curvar=""; insummary=0
 }
 /^Running target / { curvar = \$3; next }
+/^ERROR: \(DejaGnu\)/  { if (variant == curvar) dgerrorcnt += 1 }
 /^# of /   { if (variant == curvar) insummary = 1 }
 /^# of expected passes/{ if (insummary == 1) passcnt += \$5; 
next; }
 /^# of unexpected successes/   { if (insummary == 1) xpasscnt += \$5; next; }
@@ -390,6 +391,7 @@ BEGIN {
 { next }
 END {
   printf ("\t\t=== %s Summary for %s ===\n\n", tool, variant)
+  if (dgerrorcnt != 0) printf ("# of DejaGnu errors\t\t%d\n", dgerrorcnt)
   if (passcnt != 0) printf ("# of expected passes\t\t%d\n", passcnt)
   if (failcnt != 0) printf ("# of unexpected failures\t%d\n", failcnt)
   if (xpasscnt != 0) printf ("# of unexpected successes\t%d\n", xpasscnt)
@@ -419,8 +421,9 @@ TOTAL_AWK=${TMP}/total.awk
 cat << EOF > $TOTAL_AWK
 BEGIN {
   tool="$TOOL"
-  passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kfailcnt=0; 
unsupcnt=0; unrescnt=0
+  passcnt=0; failcnt=0; untstcnt=0; xpasscnt=0; xfailcnt=0; kfailcnt=0; 
unsupcnt=0; unrescnt=0; dgerrorcnt=0
 }
+/^# of DejaGnu errors/ { dgerrorcnt += \$5 }
 /^# of expected passes/{ passcnt += \$5 }
 /^# of unexpected failures/{ failcnt += \$5 }
 /^# of unexpected successes/   { xpasscnt += \$5 }
@@ -431,7 +434,8 @@ BEGIN {
 /^# of unresolved testcases/   { unrescnt += \$5 }
 /^# of unsupported tests/  { unsupcnt += \$5 }
 END {
-  printf ("\n\t\t=== %s Summary ===\n\n", tool)
+  printf ("\n\t\t=== %s MySummary ===\n\n", tool)
+  if (dgerrorcnt != 0) printf ("# of DejaGnu errors\t\t%d\n", dgerrorcnt)
   if (passcnt != 0) printf ("# of expected passes\t\t%d\n", passcnt)
   if (failcnt != 0) printf ("# of unexpected failures\t%d\n", failcnt)
   if (xpasscnt != 0) printf ("# of unexpected successes\t%d\n", xpasscnt)

Re: PR35503 - warn for restrict pointer

[Pedro Alves]

On 09/18/2016 06:16 PM, Prathamesh Kulkarni wrote:
> +  warning_at_rich_loc (&richloc, OPT_Wrestrict, "passing argument %i to"
> +" restrict-qualified parameter aliases with argument%s 
> %I",
> +param_pos + 1, (arg_positions.length() > 1) ? "s" : "",
> +&arg_positions); 
> +}

This way of building a plural string is not i18n friendly:

 https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html

I see that there are _n suffixed versions of a few diagnostic
functions, like inform_n, warning_n, etc. that handle plurals
by calling ngettext in diagnostic_n_impl, but it looks like a
warning_at_rich_loc_n variant of warning_at_rich_loc is missing.

Thanks,
Pedro Alves

Re: [PATCH] libstdc++/77645 fix deque and vector xmethods for Python 3

[Jonathan Wakely]

On 20/09/16 09:41 +0100, Jonathan Wakely wrote:

On 19/09/16 21:39 +, Joseph Myers wrote:

On Mon, 19 Sep 2016, Jonathan Wakely wrote:


On 19/09/16 17:24 +, Joe Buck wrote:

Python has a distinct integer division operator, "//".  7 // 3 returns the
integer 2.


Python 3 does, but Python 2 doesn't have it unless you import it from
__future, and I don't know how far back that works. I don't want to
introduce a fix for Python 3 that breaks it for old systems.


No, // is available in Python, without needing __future__, from 2.2
onwards.


I stand corrected, thanks both of you.

I'll test and commnit this then.


I still need a cast in one place, because gdb.Value doesn't support
the // operator (https://sourceware.org/PR20624).

Tested x86_64-linux, committed to all branches.

commit c5abd6b08844bb626ea3f3df9e33bb2b97829624
Author: Jonathan Wakely 
Date:   Tue Sep 20 10:50:55 2016 +0100

Replace casts with floordiv operator in Python xmethods

	* python/libstdcxx/v6/xmethods.py (DequeWorkerBase.__init__)
	(DequeWorkerBase.index, VectorWorkerBase.get): Use // for division.

diff --git a/libstdc++-v3/python/libstdcxx/v6/xmethods.py b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
index 71a5b75..eb0dd79 100644
--- a/libstdc++-v3/python/libstdcxx/v6/xmethods.py
+++ b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
@@ -165,7 +165,7 @@ class ArrayMethodsMatcher(gdb.xmethod.XMethodMatcher):
 class DequeWorkerBase(gdb.xmethod.XMethodWorker):
 def __init__(self, val_type):
 self._val_type = val_type
-self._bufsize = int(512 / val_type.sizeof) or 1
+self._bufsize = 512 // val_type.sizeof or 1
 
 def size(self, obj):
 first_node = obj['_M_impl']['_M_start']['_M_node']
@@ -176,7 +176,7 @@ class DequeWorkerBase(gdb.xmethod.XMethodWorker):
 
 def index(self, obj, idx):
 first_node = obj['_M_impl']['_M_start']['_M_node']
-index_node = first_node + int(idx / self._bufsize)
+index_node = first_node + int(idx) // self._bufsize
 return index_node[0][idx % self._bufsize]
 
 class DequeEmptyWorker(DequeWorkerBase):
@@ -419,7 +419,7 @@ class VectorWorkerBase(gdb.xmethod.XMethodWorker):
 if self._val_type.code == gdb.TYPE_CODE_BOOL:
 start = obj['_M_impl']['_M_start']['_M_p']
 bit_size = start.dereference().type.sizeof * 8
-valp = start + int(index / bit_size)
+valp = start + index // bit_size
 offset = index % bit_size
 return (valp.dereference() & (1 << offset)) > 0
 else:

[patch,avr] Fix PR77326: CC_NONE might clobber condition code

[Georg-Johann Lay]

This fixes the case of CC_NONE (insn attribute for cc is "none"):

Even in cases where the instructions of an insn do not change the condition 
code of the machine, they still might change some registers by clobber, set, 
etc.  If one such register overlaps an expression stored in cc_status.value1/2 
we must reset cc_status as the value stored there no more represents the state 
of cc0.


Ok to apply and backport?


Johann

gcc/
PR target/77326
* config/avr/avr.c (avr_notice_update_cc) [CC_NONE]: If insn
touches some regs mentioned in cc_status, do CC_STATUS_INIT.

gcc/testsuite/
PR target/77326
* gcc.target/avr/torture/pr77326.c: New test.
Index: config/avr/avr.c
===
--- config/avr/avr.c	(revision 240135)
+++ config/avr/avr.c	(working copy)
@@ -2511,8 +2511,44 @@ avr_notice_update_cc (rtx body ATTRIBUTE
   break;
 
 case CC_NONE:
-  /* Insn does not affect CC at all.  */
-  break;
+  /* Insn does not affect CC at all, but it might set some registers
+ that are stored in cc_status.  If such a register is affected by
+ the current insn, for example by means of a SET or a CLOBBER,
+ then we must reset cc_status; cf. PR77326.
+
+ Unfortunately, set_of cannot be used as reg_overlap_mentioned_p
+ will abort on COMPARE (which might be found in cc_status.value1/2).
+ Thus work out the registers set by the insn and regs mentioned
+ in cc_status.value1/2.  */
+
+  if (cc_status.value1
+  || cc_status.value2)
+{
+  HARD_REG_SET regs_used;
+  HARD_REG_SET regs_set;
+  CLEAR_HARD_REG_SET (regs_used);
+
+  if (cc_status.value1
+  && !CONSTANT_P (cc_status.value1))
+{
+  find_all_hard_regs (cc_status.value1, ®s_used);
+}
+
+  if (cc_status.value2
+  && !CONSTANT_P (cc_status.value2))
+{
+  find_all_hard_regs (cc_status.value1, ®s_used);
+}
+
+  find_all_hard_reg_sets (insn, ®s_set, false);
+
+  if (hard_reg_set_intersect_p (regs_used, regs_set))
+{
+  CC_STATUS_INIT;
+}
+}
+
+  break; // CC_NONE
 
 case CC_SET_N:
   CC_STATUS_INIT;
Index: testsuite/gcc.target/avr/torture/pr77326.c
===
--- testsuite/gcc.target/avr/torture/pr77326.c	(nonexistent)
+++ testsuite/gcc.target/avr/torture/pr77326.c	(working copy)
@@ -0,0 +1,26 @@
+/* { dg-do run } */
+/* { dg-options "-Wl,--defsym,test1=0" } */
+
+extern void test1 (void) __attribute__((weak));
+
+__attribute__((noinline,noclone))
+static void va_pseudo (int flag, ...)
+{
+  __asm ("nop":);
+}
+
+__attribute__((noinline,noclone))
+static void func (void)
+{
+  va_pseudo (0, 0, 0, 0);
+
+  if (test1)
+__builtin_abort ();
+}
+
+int main (void)
+{
+  func();
+  __builtin_exit (0);
+  return 0;
+}

Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments

[Bernd Schmidt]

On 09/20/2016 01:16 AM, Segher Boessenkool wrote:

On Mon, Sep 19, 2016 at 06:07:46PM -0400, Trevor Saunders wrote:

- JUMP_LABEL can be a return which is not an insn.  That also seems
  like something that should be improved at some point.

The JUMP_LABEL field within a JUMP_INSN?  That sounds like a bug.


yes, see the comment before the declaration of rtx_jump_insn::jump_label ()
it can be either a possibly deleted label, or a RETURN or SIMPLE_RETURN
expression.  Memory usage concerns aside its a tempting design to
change, but at the moment itts definitely intended to work this way,
there's plenty of code that checks ANY_RETURN_P (JUMP_LABEL (x)).


Yes.  But rtl.texi still says:

  Return insns count as jumps, but since they do not refer to any
  labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.


And the comment at the top of jump.c:

   Each CODE_LABEL has a count of the times it is used
   stored in the LABEL_NUSES internal field, and each JUMP_INSN
   has one label that it refers to stored in the
   JUMP_LABEL internal field.  With this we can detect labels that
   become unused because of the deletion of all the jumps that
   formerly used them.  The JUMP_LABEL info is sometimes looked
   at by later passes.  For return insns, it contains either a
   RETURN or a SIMPLE_RETURN rtx.

It's intentional, and really there's nothing wrong with it, or with 
operands[] containing CODE_LABELs. The problem is trying to force a 
static type system onto a dynamically typed data structure.



Bernd

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Kyrill Tkachov]

Hi Bernd,

On 19/09/16 22:32, Bernd Edlinger wrote:

On 09/19/16 21:51, Jeff Law wrote:

On 09/15/2016 03:19 AM, Bernd Edlinger wrote:

On 09/14/16 20:11, Jason Merrill wrote:

Yes.  The reasoning I initially had was that it is completely
pointless to have something of the form "if (x ? 1 : 2)" or
"if (x ? 0 : 0)" because the result does not even depend on x
in this case.  But something like "if (x ? 4999 : 0)" looks
bogus but does at least not ignore x.

If the false-positives are becoming too much of a problem here,
then I should of course revert to the previous heuristic again.

I think we could have both, where the weaker form is part of -Wall and
people can explicitly select the stronger form.


Yes, agreed.  So here is what I would think will be the first version.

It can later be extended to cover the more pedantic cases which
will not be enabled by -Wall.

I would like to send a follow-up patch for the warning on
signed-integer shift left in boolean context, which I think
should also be good for Wall.
(I already had that feature in patch version 2 but that's meanwhile
outdated).


Bootstrap and reg-testing on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.


changelog-pr77434v3.txt


gcc:
2016-09-14  Bernd Edlinger  

 PR c++/77434
 * doc/invoke.texi: Document -Wcond-in-bool-context.

 PR middle-end/77421
 * dwarf2out.c (output_loc_operands): Fix an assertion.

c-family:
2016-09-14  Bernd Edlinger  

 PR c++/77434
 * c.opt (Wcond-in-bool-context): New warning.
 * c-common.c (c_common_truthvalue_conversion): Warn on integer
 constants in boolean context.

cp:
2016-09-14  Bernd Edlinger  

 PR c++/77434
 * cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.

testsuite:
2016-09-14  Bernd Edlinger  

 PR c++/77434
 * c-c++-common/Wcond-in-bool-context.c: New test.


patch-pr77434v3.diff


Index: gcc/builtins.c
===
--- gcc/builtins.c(revision 240135)
+++ gcc/builtins.c(working copy)
@@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree
fndecl
  tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg);

  signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
-signbit_call, integer_zero_node);
+signbit_call, integer_zero_node);
  isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
-  isinf_call, integer_zero_node);
+  isinf_call, integer_zero_node);

-tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
signbit_call,
-   integer_minus_one_node, integer_one_node);
  tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
-   isinf_call, tmp,
-   integer_zero_node);
+   signbit_call, integer_minus_one_node,
+   integer_one_node);
+/* Avoid a possible -Wint-in-bool-context warning in C.  */
+tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp,
+   integer_zero_node);
+tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
+   isinf_call, tmp, integer_zero_node);
}

  return tmp;

This hunk is not mentioned in the ChangeLog and there's a good chance
this has or is going to change significantly.  I don't like the tmp+0
workaround either.  If there isn't an immediate need, can we let this
hunk slide and come back to it after the other changes from Tamar &
Wilco are wrapped up?


Yes that would be good.


I think this is OK with the builtins.c hunk dropped as long as exclusion
of that hunk doesn't trigger spurious warnings.


It does trigger a warning but it is usually masked by -Wsystem-headers,
so I initially missed it completely, but it comes quite often when
I build a recent glibc.  That does only happen with C, not C++.

If I drop that chunk then I must also drop that line

if (__builtin_isinf_sign ((float)a/b)) /* { dg-bogus "boolean
context" } */

But I have to come back and silence the warning on that construct
in a follow-up patch.


If it is OK, knowing it is work in progress,
I could hold back the builtins part for now,
and commit what I have ?


arm bootstrap is now failing:
$SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in boolean 
context [-Werror=int-in-bool-context]
: (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
   ~^~
$SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro 'TARGET_ARM_FP'
   if (TARGET_ARM_FP)


The full definition of TARGET_ARM_FP is:
#define TARGET_ARM_FP\
  (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
: (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
  : 0)

We want it set to 0 when there's no FP but when FP is available we set it to a 
bitmask
to suggest the level that i

Re: [PATCH, 5.x/6.x/7.x] Be more conservative in early inliner if FDO is enabled

[Richard Biener]

On Fri, Sep 16, 2016 at 2:00 PM, Jan Hubicka  wrote:
>> > > I also like a new param better as it avoids a new magic constant and
>> > > makes playing with
>> > > it easier (your patch removes the ability to do statistics like you did 
>> > > via the
>> > > early-inlining-insns parameter by forcing it to two).
>> >
>> > Hmm, you are right that you do not know if this particular function will 
>> > get
>> > profile (forgot about that).  Still, please use two params - it is more
>> > consistent with what we have now and we may make it profile specific in
>> > future..
>> >
>> > Honza
>> > >
>> > > Thanks,
>> > > Richard.
>>
>> A new patch for trunk is attached.
>>
>> Regards,
>> Yuan, Pengfei
>>
>>
>> 2016-09-16  Yuan Pengfei  
>>
>>   * doc/invoke.texi (--param early-inlining-insns-feedback): New.
>>   * ipa-inline.c (want_early_inline_function_p): Use
>>   PARAM_EARLY_INLINING_INSNS_FEEDBACK when FDO is enabled.
>>   * params.def (PARAM_EARLY_INLINING_INSNS_FEEDBACK): Define.
>>   (PARAM_EARLY_INLINING_INSNS): Change help string accordingly.

Btw, It occurs to me that then win in code-size might be purely due to the
smaller base value for the TU size we use to compute the maximum unit
growth with ... any idea how to improve it on this side?  Say, computing
the TU size before early optimization (uh, probably not ...)

That said, the inliner always completely fills its budged, that is, increase
the unit by max-unit-growth?

Richard.

> OK,
> thanks
>
> Honza

Re: [PATCH, 5.x/6.x/7.x] Be more conservative in early inliner if FDO is enabled

[Richard Biener]

On Tue, Sep 20, 2016 at 1:31 PM, Richard Biener
 wrote:
> On Fri, Sep 16, 2016 at 2:00 PM, Jan Hubicka  wrote:
>>> > > I also like a new param better as it avoids a new magic constant and
>>> > > makes playing with
>>> > > it easier (your patch removes the ability to do statistics like you did 
>>> > > via the
>>> > > early-inlining-insns parameter by forcing it to two).
>>> >
>>> > Hmm, you are right that you do not know if this particular function will 
>>> > get
>>> > profile (forgot about that).  Still, please use two params - it is more
>>> > consistent with what we have now and we may make it profile specific in
>>> > future..
>>> >
>>> > Honza
>>> > >
>>> > > Thanks,
>>> > > Richard.
>>>
>>> A new patch for trunk is attached.
>>>
>>> Regards,
>>> Yuan, Pengfei
>>>
>>>
>>> 2016-09-16  Yuan Pengfei  
>>>
>>>   * doc/invoke.texi (--param early-inlining-insns-feedback): New.
>>>   * ipa-inline.c (want_early_inline_function_p): Use
>>>   PARAM_EARLY_INLINING_INSNS_FEEDBACK when FDO is enabled.
>>>   * params.def (PARAM_EARLY_INLINING_INSNS_FEEDBACK): Define.
>>>   (PARAM_EARLY_INLINING_INSNS): Change help string accordingly.
>
> Btw, It occurs to me that then win in code-size might be purely due to the
> smaller base value for the TU size we use to compute the maximum unit
> growth with ... any idea how to improve it on this side?  Say, computing
> the TU size before early optimization (uh, probably not ...)
>
> That said, the inliner always completely fills its budged, that is, increase
> the unit by max-unit-growth?

What I'm trying to say is that rather than limiting early inlining we should
maybe decrease inline-unit-growth when FDO is in effect?  Because we
can better control where the inlining goes.  If there is over 8% reduction
in size benchmarking (unpatched) compiler on Firefox with FDO and
--param inline-unit-growth=12 might show if the results are the same.

Richard.

> Richard.
>
>> OK,
>> thanks
>>
>> Honza

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

On Tue, Sep 20, 2016 at 1:32 AM, kugan
 wrote:
> Hi Richard,
> Thanks for the review.
>
>
> On 19/09/16 23:40, Richard Biener wrote:
>>
>> On Sun, Sep 18, 2016 at 10:21 PM, kugan
>>  wrote:
>>>
>>> Hi Richard,
>>>
>>>
>>> On 14/09/16 21:31, Richard Biener wrote:


 On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah
  wrote:
>
>
> Hi Richard,
>
> On 25 August 2016 at 22:24, Richard Biener 
> wrote:
>>
>>
>> On Thu, Aug 11, 2016 at 1:09 AM, kugan
>>  wrote:
>>>
>>>
>>> Hi,
>>>
>>>
>>> On 10/08/16 20:28, Richard Biener wrote:



 On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek 
 wrote:
>
>
>
> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
>>
>>
>>
>> I see it now. The problem is we are just looking at (-1) being in
>> the
>> ops
>> list for passing changed to rewrite_expr_tree in the case of
>> multiplication
>> by negate.  If we have combined (-1), as in the testcase, we will
>> not
>> have
>> the (-1) and will pass changed=false to rewrite_expr_tree.
>>
>> We should set changed based on what happens in
>> try_special_add_to_ops.
>> Attached patch does this. Bootstrap and regression testing are
>> ongoing.
>> Is
>> this OK for trunk if there is no regression.
>
>
>
>
> I think the bug is elsewhere.  In particular in
> undistribute_ops_list/zero_one_operation/decrement_power.
> All those look problematic in this regard, they change RHS of
> statements
> to something that holds a different value, while keeping the LHS.
> So, generally you should instead just add a new stmt next to the
> old
> one,
> and adjust data structures (replace the old SSA_NAME in some ->op
> with
> the new one).  decrement_power might be a problem here, dunno if
> all
> the
> builtins are const in all cases that DSE would kill the old one,
> Richard, any preferences for that?  reset flow sensitive info +
> reset
> debug
> stmt uses, or something different?  Though, replacing the LHS with
> a
> new
> anonymous SSA_NAME might be needed too, in case it is before
> SSA_NAME
> of
> a
> user var that doesn't yet have any debug stmts.




 I'd say replacing the LHS is the way to go, with calling the
 appropriate
 helper
 on the old stmt to generate a debug stmt for it / its uses (would
 need
 to look it
 up here).

>>>
>>> Here is an attempt to fix it. The problem arises when in
>>> undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR
>>> is
>>> added
>>> (-1) MULT_EXPR (OP). Real problem starts when we handle this in
>>> zero_one_operation. Unlike what was done earlier, we now change the
>>> stmt
>>> (with propagate_op_to_signle use or by directly) such that the value
>>> computed by stmt is no longer what it used to be. Because of this,
>>> what
>>> is
>>> computed in undistribute_ops_list and rewrite_expr_tree are also
>>> changed.
>>>
>>> undistribute_ops_list already expects this but rewrite_expr_tree will
>>> not if
>>> we dont pass the changed as an argument.
>>>
>>> The way I am fixing this now is, in linearize_expr_tree, I set
>>> ops_changed
>>> to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we
>>> call
>>> zero_one_operation with ops_changed = true, I replace all the LHS in
>>> zero_one_operation with the new SSA and replace all the uses. I also
>>> call
>>> the rewrite_expr_tree with changed = false in this case.
>>>
>>> Does this make sense? Bootstrapped and regression tested for
>>> x86_64-linux-gnu without any new regressions.
>>
>>
>>
>> I don't think this solves the issue.  zero_one_operation associates
>> the
>> chain starting at the first *def and it will change the intermediate
>> values
>> of _all_ of the stmts visited until the operation to be removed is
>> found.
>> Note that this is independent of whether try_special_add_to_ops did
>> anything.
>>
>> Even for the regular undistribution cases we get this wrong.
>>
>> So we need to back-track in zero_one_operation, replacing each LHS
>> and in the end the op in the opvector of the main chain.  That's
>> basically
>> the same as if we'd do a regular re-assoc operation on the sub-chains.
>> Take their subops, simulate zero_one_operation by
>> appending the cancelling operation and optimizing the oplist, and t

Re: [PATCH, 5.x/6.x/7.x] Be more conservative in early inliner if FDO is enabled

[Yuan, Pengfei]

> > Btw, It occurs to me that then win in code-size might be purely due to the
> > smaller base value for the TU size we use to compute the maximum unit
> > growth with ... any idea how to improve it on this side?  Say, computing
> > the TU size before early optimization (uh, probably not ...)
> >
> > That said, the inliner always completely fills its budged, that is, increase
> > the unit by max-unit-growth?
> 
> What I'm trying to say is that rather than limiting early inlining we should
> maybe decrease inline-unit-growth when FDO is in effect?  Because we
> can better control where the inlining goes.  If there is over 8% reduction
> in size benchmarking (unpatched) compiler on Firefox with FDO and
> --param inline-unit-growth=12 might show if the results are the same.
> 
> Richard.
> 
> > Richard.

AFAIK, param inline-unit-growth only affects pass_ipa_inline. However, the code
size reduction achieved with my patch is from pass_early_inline.

Yuan, Pengfei

Re: [PATCH] Optimise the fpclassify builtin to perform integer operations when possible

[Tamar Christina]

On 16/09/16 20:49, Jeff Law wrote:

On 09/12/2016 10:19 AM, Tamar Christina wrote:

Hi All,
+
+  /* Re-interpret the float as an unsigned integer type
+ with equal precision.  */
+  int_arg_type = build_nonstandard_integer_type (TYPE_PRECISION 
(type), 0);

+  int_arg = fold_build1_loc (loc, INDIRECT_REF, int_arg_type,
+  fold_build1_loc (loc, NOP_EXPR,
+   build_pointer_type (int_arg_type),
+fold_build1_loc (loc, ADDR_EXPR,
+ build_pointer_type (type), arg)));
Doesn't this make ARG addressable?  Which in turn means ARG won't be 
exposed to the gimple/ssa optimizers.Or is it the case that when 
fpclassify is used its argument is already in memory (and thus 
addressable?)


I believe that it is the case that when fpclassify is use the argument 
is already addressable, but I am not 100% certain. I may be able to do 
this differently so I'll

come back to you on this one.

+ exp, const1));
+
+  /* Combine the values together.  */
+  specials = fold_build3_loc (loc, COND_EXPR, int_type, 
zero_check, fp_zero,

+   fold_build3_loc (loc, COND_EXPR, int_type, exp_lsb_set,
+fold_build3_loc (loc, COND_EXPR, int_type, 
mantissa_any_set,

+  HONOR_NANS (mode) ? fp_nan : fp_normal,
+  HONOR_INFINITIES (mode) ? fp_infinite : fp_normal),
+fp_subnormal));
So this implies you're running on generic, not gimple, right? 
Otherwise you can't generate these kinds of expressions.




Yes this is generic.


diff --git a/gcc/real.h b/gcc/real.h
index 
59af580e78f2637be84f71b98b45ec6611053222..36ded57cf4db7c30c935bdb24219a167480f39c8 
100644

--- a/gcc/real.h
+++ b/gcc/real.h
@@ -161,6 +161,15 @@ struct real_format
   bool has_signed_zero;
   bool qnan_msb_set;
   bool canonical_nan_lsbs_set;
+
+  /* This flag indicates whether the format can be used in the 
optimized

+ code paths for the __builtin_fpclassify function and friends.
+ The format has to have the same NaN and INF representation as 
normal
+ IEEE floats (e.g. exp must have all bits set), most significant 
bit must be
+ sign bit, followed by exp bits of at most 32 bits.  Lastly the 
floating
+ point number must be representable as an integer.  The base of 
the number

+ also must be base 2.  */
+  bool is_binary_ieee_compatible;
   const char *name;
 };
I think Joseph has already commented on the contents of the 
initializer and a few more cases were we can use the optimized paths.


However, I do have a general question.  There are some targets which 
have FPUs that are basically IEEE, but don't support certain IEEE 
features like NaNs, denorms, etc.


Presumably all that's needed is for those targets to define a hook to 
describe which checks will always be false and you can check the 
hook's return value.  Right?


Yes, that should be enough. Not supporting NAN and Infinities is already 
supported though, but it's tied to the real format rather than a 
particular target.


Can you please include some tests to verify you're getting the initial 
code generation you want?  Ideally there'd be execution tests too 
where you generate one of the special nodes, then call the __builtin 
and verify that you get the expected results back. The latter in 
particular are key since it'll allow us to catch problems much earlier 
across the wide variety of targets GCC supports.


I can add some code generation tests. There are I believe already some 
execution tests, which test both correct and incorrect output.


I think you already had plans to post an updated patch.  Please 
include the fixes noted above in that update.


Yes I will include your feedback in it. I'm currently waiting for some 
extra performance numbers.


Thanks,
Tamar

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 20 September 2016 at 08:57, Martin Sebor  wrote:
>> and used
>> pp_format for formatting arg_positions by adding specifier %I (name
>> chosen arbitrarily).
>> Does that look OK ?
>
>
> diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
> index a39815e..e8bd1ef 100644
> --- a/gcc/pretty-print.c
> +++ b/gcc/pretty-print.c
> @@ -610,6 +610,23 @@ pp_format (pretty_printer *pp, text_info *text)
>   (pp, *text->args_ptr, precision, unsigned, "u");
>   break;
>
> +   case 'I':
>
> The comment just above pp_format that lists the format specifiers
> understood by the function should be updated.  There probably (I
> hope) is more formal documentation of the format specifiers
> somewhere else that should be updated as well.
>
> That said, since this specifier formats a vec, it seems that
> it might be useful to be able to format vectors of other elements,
> such as short and long.  With that in mind, would adding a new V
> length modifier instead be a more regular way to extend the pretty
> printer to these types?  The V length modifier would have to be
> accepted in conjunction with other length modifiers (h, l, etc
> and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept
> a vec*  as an argument and format it as a sequence
> of decimal numbers, and "%lVx" would do the same for vec*
> formatting them in hex.
Hi,
Thanks everyone for the suggestions.

This patch does the following:
a) adds warning_at_rich_loc_n(). Somehow I missed David's clear explanation of
warning_at_rich_loc_n earlier :(

b) clears TREE_VISITED flag in C/C++ FE after we are done with
warn_for_restrict().
I assumed that TREE_VISITED would be treated as a "scratch" flag and any pass
wishing to use it must set it first, so didn't bother clearing at first.

c) It appears '%I' is already used for some format specifier in
c-family/c-format.c ?
I changed name to %Z instead. Martin suggested a better idea above to
implement %Z as a length modifier so we can extend it to print vec of other
types, like %hZx would print vec formatted in hex.
I am not sure though if it would be a simple fix, and left it as a FIXME
in the patch adding %Z just to print vec, maybe we could revisit it later ?
Could someone please take a look at the change to c-format.c, I am not sure
if I have added that correctly.

Thanks,
Prathamesh
>
> Martin
>
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index fc25686..742f06d 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -47,6 +47,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimplify.h"
 #include "substring-locations.h"
 #include "spellcheck.h"
+#include "gcc-rich-location.h"
 
 cpp_reader *parse_in;		/* Declared in c-pragma.h.  */
 
@@ -13084,4 +13085,53 @@ diagnose_mismatched_attributes (tree olddecl, tree newdecl)
   return warned;
 }
 
+/* Warn if an argument at position param_pos is passed to a
+   restrict-qualified param, and it aliases with another argument.  */
+
+void
+warn_for_restrict (unsigned param_pos, vec *args)
+{
+  tree arg = (*args)[param_pos];
+  if (TREE_VISITED (arg) || operand_equal_p (arg, null_pointer_node, 0))
+return;
+
+  location_t loc = EXPR_LOC_OR_LOC (arg, input_location);
+  gcc_rich_location richloc (loc);
+
+  unsigned i;
+  tree current_arg;
+  auto_vec arg_positions;
+
+  FOR_EACH_VEC_ELT (*args, i, current_arg) 
+{
+  if (i == param_pos)
+	continue;
+
+  tree current_arg = (*args)[i];
+  if (operand_equal_p (arg, current_arg, 0))
+	{
+	  TREE_VISITED (current_arg) = 1; 
+	  arg_positions.safe_push (i + 1); 
+	}
+}
+
+  if (arg_positions.is_empty ())
+return;
+
+  int pos;
+  FOR_EACH_VEC_ELT (arg_positions, i, pos)
+{
+  tree arg = (*args)[pos - 1];
+  if (EXPR_HAS_LOCATION (arg))
+	richloc.add_range (EXPR_LOCATION (arg), false);
+}
+
+  warning_at_rich_loc_n (&richloc, OPT_Wrestrict, arg_positions.length (),
+			 "passing argument %i to restrict-qualified parameter"
+			 " aliases with argument %Z",
+			 "passing argument %i to restrict-qualified parameter"
+			 " aliases with arguments %Z",
+			 param_pos + 1, &arg_positions); 
+}
+
 #include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 5bbf951..f29ea1f 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -922,6 +922,7 @@ extern void c_parse_final_cleanups (void);
 
 extern void warn_for_omitted_condop (location_t, tree);
 extern void warn_for_memset (location_t, tree, tree, int);
+extern void warn_for_restrict (unsigned, vec *);
 
 /* These macros provide convenient access to the various _STMT nodes.  */
 
diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c
index bf39ee0..b27d34f 100644
--- a/gcc/c-family/c-format.c
+++ b/gcc/c-family/c-format.c
@@ -713,6 +713,7 @@ static const format_char_info gcc_tdiag_char_table[] =
   { "r",   1, STD_C89, { T89_C,   BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN, 

Re: [PATCH, 5.x/6.x/7.x] Be more conservative in early inliner if FDO is enabled

[Richard Biener]

On Tue, Sep 20, 2016 at 1:57 PM, Yuan, Pengfei  wrote:
>> > Btw, It occurs to me that then win in code-size might be purely due to the
>> > smaller base value for the TU size we use to compute the maximum unit
>> > growth with ... any idea how to improve it on this side?  Say, computing
>> > the TU size before early optimization (uh, probably not ...)
>> >
>> > That said, the inliner always completely fills its budged, that is, 
>> > increase
>> > the unit by max-unit-growth?
>>
>> What I'm trying to say is that rather than limiting early inlining we should
>> maybe decrease inline-unit-growth when FDO is in effect?  Because we
>> can better control where the inlining goes.  If there is over 8% reduction
>> in size benchmarking (unpatched) compiler on Firefox with FDO and
>> --param inline-unit-growth=12 might show if the results are the same.
>>
>> Richard.
>>
>> > Richard.
>
> AFAIK, param inline-unit-growth only affects pass_ipa_inline. However, the 
> code
> size reduction achieved with my patch is from pass_early_inline.

Any inlining you don't do at early inlining time will a) decrease the
size inline-unit-growth
is computed on b) just delays inlining to IPA inlining (with the now
more constrained size budget).

Richard.

> Yuan, Pengfei

[PATCH] Fix PR77646

[Richard Biener]

This fixes the PR.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

Richard.

2016-09-20  Richard Biener  

PR tree-optimization/77646
* tree-ssa-sccvn.c (visit_reference_op_call): Always value-number
a VDEF.

* gcc.dg/torture/pr77646.c: New testcase.

Index: gcc/tree-ssa-sccvn.c
===
*** gcc/tree-ssa-sccvn.c(revision 240255)
--- gcc/tree-ssa-sccvn.c(working copy)
*** visit_reference_op_call (tree lhs, gcall
*** 3470,3475 
--- 3577,3586 
  {
if (vnresult->result_vdef && vdef)
changed |= set_ssa_val_to (vdef, vnresult->result_vdef);
+   else if (vdef)
+   /* If the call was discovered to be pure or const reflect
+  that as far as possible.  */
+   changed |= set_ssa_val_to (vdef, vuse_ssa_val (gimple_vuse (stmt)));
  
if (!vnresult->result && lhs)
vnresult->result = lhs;
Index: gcc/testsuite/gcc.dg/torture/pr77646.c
===
*** gcc/testsuite/gcc.dg/torture/pr77646.c  (revision 0)
--- gcc/testsuite/gcc.dg/torture/pr77646.c  (working copy)
***
*** 0 
--- 1,21 
+ /* { dg-do compile } */
+ 
+ struct e {
+ int (*f)();
+ void (*g)();
+ } * c;
+ int a;
+ void *h();
+ typedef struct { struct e j; } k;
+ int l() { return a; }
+ const struct e b = {l};
+ void m()
+ {
+   k *d = h();
+   d->j = b;
+   c = (struct e *)d;
+   struct e *i = c;
+   if (i->f(c))
+ while (i->f(c))
+   i->g();
+ }

Re: [accaf, fortran, 4/4] Allocatable components support in derived typed coarrays

[Andreas Schwab]

On Sep 19 2016, Andre Vehreschild  wrote:

> Index: gcc/testsuite/gfortran.dg/coarray_allocate_7.f08
> ===
> --- gcc/testsuite/gfortran.dg/coarray_allocate_7.f08  (nicht existent)
> +++ gcc/testsuite/gfortran.dg/coarray_allocate_7.f08  (Arbeitskopie)
> @@ -0,0 +1,27 @@
> +! { dg-do run }
> +! { dg-options "-fcoarray=lib -lcaf_single -fdump-tree-original" }
> +
> +! Contributed by Damian Rouson
> +! Checking whether (de-)registering of coarrays works.
> +
> +program main
> +
> +  implicit none
> +
> +  type mytype
> +integer, allocatable :: indices(:)
> +  end type
> +
> +  type(mytype), save :: object[*]
> +  integer :: i,me
> +
> +  me=this_image() ! me is always 1 here
> +  object%indices=[(i,i=1,me)]
> +  if ( size(object%indices) /= 1 ) call abort()
> +  ! therefore no array is present here and no array test needed.
> +  if ( object%indices(1) /= 1 ) call abort()
> +end program
> +
> +! { dg-final { scan-tree-dump-times "_gfortran_caf_register \\(D.\[0-9\]{4}, 
> 1, &\\(\\(struct mytype\\) \\*object\\).indices.token, &\\(\\(struct 
> mytype\\) \\*object\\).indices, 0B, 0B, 0\\);" 2 "original" } }
> +! { dg-final { scan-tree-dump-times "_gfortran_caf_deregister 
> \\(&\\(\\(struct mytype\\) \\*object\\).indices.token, 0B, 0B, 0\\);" 1 
> "original" } }
> +

FAIL: gfortran.dg/coarray_allocate_7.f08   -O0   scan-tree-dump-times original 
"_gfortran_caf_register \\(D.[0-9]{4}, 1, &\\(\\(struct mytype\\) 
\\*object\\).indices.token, &\\(\\(struct mytype\\) \\*object\\).indices, 0B, 
0B, 0\\);" 2
PASS: gfortran.dg/coarray_allocate_7.f08   -O0   scan-tree-dump-times original 
"_gfortran_caf_deregister \\(&\\(\\(struct mytype\\) 
\\*object\\).indices.token, 0B, 0B, 0\\);" 1

$ grep _gfortran_caf_register coarray_allocate_7.f08.003t.original 
  _gfortran_caf_register (28, 0, (void * *) &caf_token.0, (void *) &desc.2, 0B, 
0B, 0);
_gfortran_caf_register (D.986, 1, &((struct mytype) 
*object).indices.token, &((struct mytype) *object).indices, 0B, 0B, 0);
_gfortran_caf_register (D.986, 1, &((struct mytype) 
*object).indices.token, &((struct mytype) *object).indices, 0B, 0B, 0);

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Handle VECTOR_CST in integer_nonzerop()

[Marc Glisse]

On Mon, 19 Sep 2016, Patrick Palka wrote:


On Wed, Sep 14, 2016 at 1:58 AM, Marc Glisse  wrote:

On Fri, 19 Aug 2016, Patrick Palka wrote:


On Fri, Aug 19, 2016 at 7:30 PM, Patrick Palka 
wrote:


integer_nonzerop() currently unconditionally returns false for a
VECTOR_CST argument.  This is confusing because one would expect that
integer_onep(x) => integer_nonzerop(x) for all x but that is currently
not the case.  For a VECTOR_CST of all ones i.e. {1,1,1,1},
integer_onep() returns true but integer_nonzerop() returns false.

This patch makes integer_nonzerop() handle VECTOR_CSTs in the obvious
way and also adds some self tests (the last of which fails without the
change).  Does this look OK to commit afetr bootstrap + regtesting on
x86_64-pc-linux-gnu?



Actually I guess there is some ambiguity as to whether
integer_nonzerop() should return true for a VECTOR_CST only if it has
at least one non-zero element or only if all of its elements are
non-zero...



In my opinion, the second one would make a lot more sense. I think most
(all?) current uses are protected by checks that mean it is never called on
vectors (where did you notice this issue?). But if we wanted to generalize,


Yeah I don't think integer_onep is called anywhere on a VECTOR_CST
currently. I ran into this issue because I tried using integer_onep on
a VECTOR_CST and found that it didn't work as expected.


I thought we were talking about integer_nonzerop? integer_onep is used on 
vectors just fine...



looking for instance at fold-const.c (A & C) == D where D & ~C != 0, we
would want that no element of D&~C is 0 to be able to simplify the whole
vector to 0. If we want an OR as in your patch, in most cases we could use
!integer_zerop, possibly with extra checks that it is indeed a constant.
Maybe we could solve this with more explicit names? integer_each_nonzerop vs
integer_not_all_zerop?


More precise naming would be nice.  Note that integer_all_onesp() and
integer_truep() already exist which is another reason I leaned towards
making integer_onep behave as an OR for VECTOR_CSTs since the AND
behavior is already covered by those aforementioned two predicates.
But since there is no consensus and no such user of integer_onep I
will just drop this patch.


--
Marc Glisse

Re: [PATCH] Tree-level fix for PR 69526

[Robin Dapp]

Hi,

> I meant to do sth like
> 
> Index: tree-ssa-propagate.c
> ===
> --- tree-ssa-propagate.c(revision 240133)
> +++ tree-ssa-propagate.c(working copy)
> @@ -1105,10 +1105,10 @@ substitute_and_fold_dom_walker::before_d
>/* Replace real uses in the statement.  */
>did_replace |= replace_uses_in (stmt, get_value_fn);
> 
> -  /* If we made a replacement, fold the statement.  */
> -  if (did_replace)
> +  /* Fold the statement.  */
> +  if (fold_stmt (&i, follow_single_use_edges))
> {
> - fold_stmt (&i, follow_single_use_edges);
> + did_replace = true;
>   stmt = gsi_stmt (i);
> }
> 
> this would need compile-time cost evaluation (and avoid the tree-vrp.c
> folding part
> of your patch).

Using this causes the simplifications to be performed in ccp1 instead of
fwprop1. I noticed two tests failing that rely on propagation being
performed in fwprop. Should these be adapted or rather the patch be changed?

> Heh, but I think duplicating code is even worse.

Ok, I changed extract_range_... after all, it's not so bad as I had
thought.  Imho it should be simplified nevertheless, perhaps I'll do it
in the future if I find some time.

> + tree cst_inner = wide_int_to_tree (inner_type, cst);
> 
> What prevents CST being truncated here?  It looks like
> (long)intvar + 0xL will be mishandled that way.
> 

Right, it may be truncated here, but the following TYPE_PRECISION ()
guard prevents the value from being used in that case.  I pulled the
line into the condition for clarity.

> OTOH given that you use this to decide whether you can use a combined constant
> that doesn't look necessary (if the constant is correct, that is) --
> what you need
> to make sure is that the final operation, (T)(A) +- CST, does not overflow
> if ! TYPE_OVERFLOW_WRAPS and there wasn't any overflow in the
> original operation.  I think that always holds, thus the combine_ovf check 
> looks
> superfluous to me.
> 

I included this to account for cases like
 return (long)(a + 2) + LONG_MAX
which should not simplify as opposed to
 return (unsigned long)(a + 2) + LONG_MAX
where the constant is usable despite the overflow. Do you think it
should be handled differently?

Revised version attached.

Regards
 Robin

--

gcc/ChangeLog:

2016-09-20  Robin Dapp  

PR middle-end/69526
This enables combining of wrapped binary operations and fixes
the tree level part of the PR.

* match.pd: Introduce patterns to simplify binary operations
wrapped inside a cast.
* tree-vrp.h: Export extract_range_from_binary_expr ().
* tree-ssa-propagate.c
(substitute_and_fold_dom_walker::before_dom_children):
Try to fold regardless of prior use propagations.
* tree-vrp.c (extract_range_from_binary_expr_1):
Add overflow check arguments and wrapper function without them.
(extract_range_from_binary_expr):
 Add wrapper function.


gcc/testsuite/ChangeLog:

2016-09-20  Robin Dapp  

* gcc.dg/wrapped-binop-simplify-signed-1.c: New test.
* gcc.dg/wrapped-binop-simplify-signed-2.c: New test.
* gcc.dg/wrapped-binop-simplify-unsigned-1.c: New test.
* gcc.dg/wrapped-binop-simplify-unsigned-2.c: New test.
diff --git a/gcc/match.pd b/gcc/match.pd
index 123e754..f624e7e 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -1138,6 +1138,130 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
(minus @0 (minus @0 @1))
@1)
 
+  /* ((T)(A +- CST)) +- CST -> (T)(A) +- CST)  */
+#if GIMPLE
+   (for outer_op (plus minus)
+ (for inner_op (plus minus)
+   (simplify
+	 (outer_op (convert (inner_op@3 @0 INTEGER_CST@1)) INTEGER_CST@2)
+	   (with
+	{
+	   /* If the inner operation is wrapped inside a conversion, we have
+	  to check it overflows/underflows and can only then perform the
+	  simplification, i.e. add the second constant to the first
+	  (wrapped) and convert afterwards.  */
+	bool inner_ovf = false;
+
+	bool combine_ovf = true;
+	tree cst = NULL_TREE;
+	bool combine_constants = false;
+	bool types_ok = false;
+
+	tree inner_type = TREE_TYPE (@3);
+	/* Check overflow in wrapped binop? */
+	bool check_inner_ovf = !TYPE_OVERFLOW_UNDEFINED (inner_type);
+	bool check_combine_ovf = !TYPE_OVERFLOW_WRAPS (type);
+
+	signop s1 = TYPE_SIGN (type);
+
+	if (TYPE_PRECISION (type) >= TYPE_PRECISION (inner_type))
+	  {
+		types_ok = true;
+
+		/* Check if the inner binop does not overflow i.e. we have VRP
+		   information and can prove prove it.  */
+		if (check_inner_ovf)
+		  {
+		value_range vr = VR_INITIALIZER;
+		if (@0 == NULL_TREE || @1 == NULL_TREE
+			|| inner_type == NULL_TREE
+			|| TREE_CODE (type) != INTEGER_TYPE)
+		  {
+			inner_ovf = true;
+		  }
+		else
+		  {
+			extract_range_from_binary_expr (&vr, inner

[PATCH 0/3] S/390: Improved risbg usage.

[Dominik Vogt]

The following series of patches improves usage of the risbg and
risbgn instructions on s390/s390x.  The patches have been
regression tested on s390 and s390x and pass the Spec2006
testsuite without any negative effects.

Patch 1 introduces a new mode attribute to simplify some
instruction patterns.

Patch 2 enables wraparound of bit ranges and rewrites portion of
the bitrange calculation code to do so.

Patch 3 adds new patterns and tests based on the first two patches
to make better use of the risbg and risbgn instructions.

Please check the individual Changelogs for details.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [PATCH 1/3] S/390: Mode attrs "bitoff[_plus]" simplify risbg instructions.

[Dominik Vogt]

On Tue, Sep 20, 2016 at 01:37:18PM +0100, Dominik Vogt wrote:
> The following series of patches improves usage of the risbg and
> risbgn instructions on s390/s390x.  The patches have been
> regression tested on s390 and s390x and pass the Spec2006
> testsuite without any negative effects.
 
> Patch 1 introduces a new mode attribute to simplify some
> instruction patterns.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany
gcc/ChangeLog

* config/s390/s390.md (bitoff, bitoff_plus): Neq mode attributes.
("*extzv_zEC12", "*insv_zEC12", "*insv_z10")
("*insv_zEC12_appendbitsleft")
("*insv_z10_appendbitsleft", "*rsbg__sll")
("*rsbg__srl"): Use new attributes.
gcc/testsuite/ChangeLog

* gcc.target/s390/md/rXsbg_mode_sXl.c: Adapt expected assembly output
to the simplified instructions.
>From 08ea0513fe57e0fdb7582ac7c4ff950495898e3a Mon Sep 17 00:00:00 2001
From: Dominik Vogt 
Date: Fri, 5 Aug 2016 08:52:44 +0100
Subject: [PATCH 1/3] S/390: Mode attrs "bitoff[_plus]" simplify risbg
 instructions.

---
 gcc/config/s390/s390.md   | 17 ++---
 gcc/testsuite/gcc.target/s390/md/rXsbg_mode_sXl.c | 16 
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index 30ddc14..21ff33e 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -761,6 +761,9 @@
 
 ;; In place of GET_MODE_BITSIZE (mode)
 (define_mode_attr bitsize [(DI "64") (SI "32") (HI "16") (QI "8")])
+;; 64 - bitsize
+(define_mode_attr bitoff [(DI "0") (SI "32") (HI "48") (QI "56")])
+(define_mode_attr bitoff_plus [(DI "") (SI "32+") (HI "48+") (QI "56+")])
 
 ;; In place of GET_MODE_SIZE (mode)
 (define_mode_attr modesize [(DI "8") (SI "4")])
@@ -3754,7 +3757,7 @@
 (match_operand 2 "const_int_operand" "")   ; size
 (match_operand 3 "const_int_operand" "")))] ; start]
   "TARGET_ZEC12"
-  "risbgn\t%0,%1,64-%2,128+63,+%3+%2" ; dst, src, start, end, shift
+  "risbgn\t%0,%1,64-%2,128+63,%3+%2" ; dst, src, start, end, shift
   [(set_attr "op_type" "RIE")])
 
 (define_insn "*extzv_z10"
@@ -3846,7 +3849,7 @@
(match_operand:GPR 3 "nonimmediate_operand" "d"))]
   "TARGET_ZEC12
&& (INTVAL (operands[1]) + INTVAL (operands[2])) <= "
-  "risbgn\t%0,%3,64-+%2,64-+%2+%1-1,-%2-%1"
+  "risbgn\t%0,%3,%2,%2+%1-1,-%2-%1"
   [(set_attr "op_type" "RIE")])
 
 (define_insn "*insv_z10"
@@ -3857,7 +3860,7 @@
(clobber (reg:CC CC_REGNUM))]
   "TARGET_Z10
&& (INTVAL (operands[1]) + INTVAL (operands[2])) <= "
-  "risbg\t%0,%3,64-+%2,64-+%2+%1-1,-%2-%1"
+  "risbg\t%0,%3,%2,%2+%1-1,-%2-%1"
   [(set_attr "op_type" "RIE")
(set_attr "z10prop" "z10_super_E1")])
 
@@ -3894,7 +3897,7 @@
 (ashift:GPR (match_operand:GPR 3 "nonimmediate_operand" "d")
 (match_operand:GPR 4 "nonzero_shift_count_operand" 
""]
   "TARGET_ZEC12 && UINTVAL (operands[2]) == (1UL << UINTVAL (operands[4])) - 1"
-  "risbgn\t%0,%3,64-,64-%4-1,%4"
+  "risbgn\t%0,%3,,64-%4-1,%4"
   [(set_attr "op_type" "RIE")
(set_attr "z10prop" "z10_super_E1")])
 
@@ -3906,7 +3909,7 @@
 (match_operand:GPR 4 "nonzero_shift_count_operand" 
""
(clobber (reg:CC CC_REGNUM))]
   "TARGET_Z10 && !TARGET_ZEC12 && UINTVAL (operands[2]) == (1UL << UINTVAL 
(operands[4])) - 1"
-  "risbg\t%0,%3,64-,64-%4-1,%4"
+  "risbg\t%0,%3,,64-%4-1,%4"
   [(set_attr "op_type" "RIE")
(set_attr "z10prop" "z10_super_E1")])
 
@@ -4035,7 +4038,7 @@
  (match_operand:GPR 3 "nonimmediate_operand" "0")))
(clobber (reg:CC CC_REGNUM))]
   "TARGET_Z10"
-  "rsbg\t%0,%1,64-,63-%2,%2"
+  "rsbg\t%0,%1,,63-%2,%2"
   [(set_attr "op_type" "RIE")])
 
 ;; unsigned {int,long} a, b
@@ -4050,7 +4053,7 @@
  (match_operand:GPR 3 "nonimmediate_operand" "0")))
(clobber (reg:CC CC_REGNUM))]
   "TARGET_Z10"
-  "rsbg\t%0,%1,64-+%2,63,64-%2"
+  "rsbg\t%0,%1,%2,63,64-%2"
   [(set_attr "op_type" "RIE")])
 
 ;; These two are generated by combine for s.bf &= val.
diff --git a/gcc/testsuite/gcc.target/s390/md/rXsbg_mode_sXl.c 
b/gcc/testsuite/gcc.target/s390/md/rXsbg_mode_sXl.c
index 178a537..ad442da 100644
--- a/gcc/testsuite/gcc.target/s390/md/rXsbg_mode_sXl.c
+++ b/gcc/testsuite/gcc.target/s390/md/rXsbg_mode_sXl.c
@@ -39,28 +39,28 @@ rosbg_si_sll (unsigned int a, unsigned int b)
 {
   return a | (b << 1);
 }
-/* { dg-final { scan-assembler-times "rosbg\t%r.,%r.,64-32,63-1,1" 1 } } */
+/* { dg-final { scan-assembler-times "rosbg\t%r.,%r.,32,63-1,1" 1 } } */
 
 __attribute__ ((noinline)) unsigned int
 rosbg_si_srl (unsigned int a, unsigned int b)
 {
   return a | (b >> 2);
 }
-/* { dg-final { scan-assembler-times "rosbg\t%r.,%r.,64-32\\+2,63,64-2" 1 } } 
*/
+/* { dg-final { scan-assembler-times "rosbg\t%r.,%r.,32\\+2,63,64-2" 1 } } */
 
 __attribute__ ((noinline)) unsigned int
 rxsbg_si_sll (unsigned int a, unsigned int b)
 {
   return a ^ (b << 1);
 }
-/* { dg-final { sca

Re: [PATCH 2/3] S/390: Enable wraparound in s390_contiguous_bitmask_p.

[Dominik Vogt]

On Tue, Sep 20, 2016 at 01:37:18PM +0100, Dominik Vogt wrote:
> The following series of patches improves usage of the risbg and
> risbgn instructions on s390/s390x.  The patches have been
> regression tested on s390 and s390x and pass the Spec2006
> testsuite without any negative effects.

> Patch 2 enables wraparound of bit ranges and rewrites portion of
> the bitrange calculation code to do so.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany
gcc/ChangeLog

* config/s390/predicates.md ("contiguous_bitmask_operand"): Adapt to new
interface of s390_contiguous_bitmask_p.
("contiguous_bitmask_nowrap_operand"): New predicate.
* ("*anddi3_cc", "*anddi3_cconly", "*anddi3"): Replace NxxDq with NxxDw.
* config/s390/constraints.md ("NxxDw", "NxxSq"): Adapt to new interface
of s390_contiguous_bitmask_p.
* ("NxxDw"): Rename NxxDq constraint to NxxDw.
("NxxSw"): New constraint.
* config/s390/s390.md ("*andsi3_zarch"): Enable bitmask wraparound.
* config/s390/s390-protos.h (s390_contiguous_bitmask_p): Updated
interface.
(s390_contiguous_bitmask_nowrap_p): Export.
* config/s390/s390.c (s390_contiguous_bitmask_nowrap_p): New name of
former s390_contiguous_bitmask_p.
(s390_contiguous_bitmask_p): Use s390_contiguous_bitmask_nowrap_p to
detect contiguous bit ranges with wraparound.  Change signature to
return START and END position instead of POS and LENGTH.
(s390_contiguous_bitmask_vector_p): Remove extra code for continous bit
ranges with wraparound.
(s390_extzv_shift_ok): Use s390_contiguous_bitmask_nowrap_p.
(s390_contiguous_bitmask_vector_p,s390_extzv_shift_ok,print_operand):
Adapt to new signature of s390_contiguous_bitmask_p.
>From 2de05c8978eb0e681bead0c7a877e9fa10293440 Mon Sep 17 00:00:00 2001
From: Dominik Vogt 
Date: Wed, 10 Aug 2016 14:45:12 +0100
Subject: [PATCH 2/3] S/390: Enable wraparound in s390_contiguous_bitmask_p.

---
 gcc/config/s390/constraints.md |  18 +++-
 gcc/config/s390/predicates.md  |  12 ++-
 gcc/config/s390/s390-protos.h  |   2 +-
 gcc/config/s390/s390.c | 200 +
 gcc/config/s390/s390.md|   8 +-
 5 files changed, 151 insertions(+), 89 deletions(-)

diff --git a/gcc/config/s390/constraints.md b/gcc/config/s390/constraints.md
index 190cdc9..ee505d0 100644
--- a/gcc/config/s390/constraints.md
+++ b/gcc/config/s390/constraints.md
@@ -55,7 +55,12 @@
 ;; D,S,H:   mode of the containing operand
 ;; 0,F: value of the other parts (F - all bits set)
 ;; --
-;; xx[DS]q  satisfies s390_contiguous_bitmask_p for DImode or SImode
+;; xxDq satisfies s390_contiguous_bitmask_p for DImode
+;;  (with possible wraparound of the one-bit range)
+;; xxSw satisfies s390_contiguous_bitmask_p for SImode
+;;  (with possible wraparound of the one-bit range)
+;; xxSq satisfies s390_contiguous_bitmask_nowrap_p for SImode
+;;  (without wraparound of the one-bit range)
 ;;
 ;; The constraint matches if the specified part of a constant
 ;; has a value different from its other parts.  If the letter x
@@ -346,15 +351,20 @@
   (and (match_code "const_int")
(match_test "s390_N_constraint_str (\"xQH0\", ival)")))
 
-(define_constraint "NxxDq"
+(define_constraint "NxxDw"
   "@internal"
   (and (match_code "const_int")
-   (match_test "s390_contiguous_bitmask_p (ival, 64, NULL, NULL)")))
+   (match_test "s390_contiguous_bitmask_p (ival, true, 64, NULL, NULL)")))
 
 (define_constraint "NxxSq"
   "@internal"
   (and (match_code "const_int")
-   (match_test "s390_contiguous_bitmask_p (ival, 32, NULL, NULL)")))
+   (match_test "s390_contiguous_bitmask_p (ival, false, 32, NULL, NULL)")))
+
+(define_constraint "NxxSw"
+  "@internal"
+  (and (match_code "const_int")
+   (match_test "s390_contiguous_bitmask_p (ival, true, 32, NULL, NULL)")))
 
 ;;
 ;; Double-letter constraints starting with O follow.
diff --git a/gcc/config/s390/predicates.md b/gcc/config/s390/predicates.md
index 75e4cb8..5b57344 100644
--- a/gcc/config/s390/predicates.md
+++ b/gcc/config/s390/predicates.md
@@ -176,10 +176,20 @@
   return false;
 })
 
+; Predicate that always allows wraparound of the one-bit range.
 (define_predicate "contiguous_bitmask_operand"
   (match_code "const_int")
 {
-  return s390_contiguous_bitmask_p (INTVAL (op), GET_MODE_BITSIZE (mode), 
NULL, NULL);
+  return s390_contiguous_bitmask_p (INTVAL (op), true,
+GET_MODE_BITSIZE (mode), NULL, NULL);
+})
+
+; Same without wraparound.
+(define_predicate "contiguous_bitmask_nowrap_operand"
+  (match_code "const_int")
+{
+  return s390_contiguous_bitmask_p
+(INTVAL (op), false, GET_MODE_BITSIZE (mode), NULL, NULL);
 })
 
 ;; Return true if OP is ligitimate for any LOC i

Re: [PATCH 3/3] S/390: Improved risbg usage.

[Dominik Vogt]

On Tue, Sep 20, 2016 at 01:37:18PM +0100, Dominik Vogt wrote:
> The following series of patches improves usage of the risbg and
> risbgn instructions on s390/s390x.  The patches have been
> regression tested on s390 and s390x and pass the Spec2006
> testsuite without any negative effects.

> Patch 3 adds new patterns and tests based on the first two patches
> to make better use of the risbg and risbgn instructions.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany
gcc/ChangeLog

* config/s390/s390.md ("*extzv_zEC12", "*extzv_z10")
("*extzv"):
Correct a typo in a comment.
Merged patterns.
("*insv_zEC12", "*insv_z10")
("*insv"): Ditto.
("*insv_zEC12_appendbitsleft")
("*insv_appendbitsleft")
("*insv_z10_appendbitsleft"): Ditto.
("*insv_zEC12_noshift", "*insv_z10_noshift")
("*insv_noshift"): Ditto.
Provide pattern with operands switched.
("*pre_z10_extv"):
Use new subst patterns.
("*extzvdi_lshiftrt", "*_ior_and_sr_ze")
("*extvsidi", "*_and_subregdi_rotr")
("*_and_subregdi_rotl", "*_di_and_rot")
("*insv_z10_noshift_cc", "*insv_z10_noshift_cconly")
("*__ior_and_lshiftrt")
("*_sidi_ior_and_lshiftrt")
("*trunc_sidi_and_subreg_lshrt"):
New patterns.
("*extzv__sll", "*extzv__srl")
("*extzv__srl")
("*extzv__sll"): Renamed patterns, use risbgn
on zEC12.
("SINT"): New mode_iterator with SI, HI, QI.
* config/s390/subst.md ("clobbercc_or_nocc_subst", "z10_or_zEC12_cond")
("clobbercc_or_nocc", "risbg_n"): New constructs for risbg pattern
duplication.
gcc/testsuite/ChangeLog

* gcc.target/s390/risbg-ll-1.c: Ported risbg tests from llvm.
* gcc.target/s390/risbg-ll-2.c: Ditto.
* gcc.target/s390/risbg-ll-3.c: Ditto.
>From cdba47ef23798cb17812f21516d40dcd24c18a17 Mon Sep 17 00:00:00 2001
From: Dominik Vogt 
Date: Fri, 5 Aug 2016 08:49:04 +0100
Subject: [PATCH 3/3] S/390: Improved risbg usage.

---
 gcc/config/s390/s390.md| 251 +++
 gcc/config/s390/subst.md   |  21 ++
 gcc/testsuite/gcc.target/s390/risbg-ll-1.c | 498 +
 gcc/testsuite/gcc.target/s390/risbg-ll-2.c | 123 +++
 gcc/testsuite/gcc.target/s390/risbg-ll-3.c |  44 +++
 5 files changed, 871 insertions(+), 66 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/s390/risbg-ll-1.c
 create mode 100644 gcc/testsuite/gcc.target/s390/risbg-ll-2.c
 create mode 100644 gcc/testsuite/gcc.target/s390/risbg-ll-3.c

diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index 2848b41..9de442f 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -606,6 +606,7 @@
 ;; same template.
 (define_mode_iterator INT [(DI "TARGET_ZARCH") SI HI QI])
 (define_mode_iterator DINT [(TI "TARGET_ZARCH") DI SI HI QI])
+(define_mode_iterator SINT [SI HI QI])
 
 ;; This iterator allows some 'ashift' and 'lshiftrt' pattern to be defined from
 ;; the same template.
@@ -3750,25 +3751,93 @@
 }
 })
 
-(define_insn "*extzv_zEC12"
+(define_insn "*extzv"
   [(set (match_operand:GPR 0 "register_operand" "=d")
   (zero_extract:GPR
 (match_operand:GPR 1 "register_operand" "d")
 (match_operand 2 "const_int_operand" "")   ; size
-(match_operand 3 "const_int_operand" "")))] ; start]
-  "TARGET_ZEC12"
-  "risbgn\t%0,%1,64-%2,128+63,%3+%2" ; dst, src, start, end, shift
-  [(set_attr "op_type" "RIE")])
+(match_operand 3 "const_int_operand" ""))) ; start
+  ]
+  ""
+  "\t%0,%1,64-%2,128+63,%3+%2" ; dst, src, start, end, 
shift
+  [(set_attr "op_type" "RIE")
+   (set_attr "z10prop" "z10_super_E1")])
 
-(define_insn "*extzv_z10"
-  [(set (match_operand:GPR 0 "register_operand" "=d")
-  (zero_extract:GPR
-   (match_operand:GPR 1 "register_operand" "d")
-   (match_operand 2 "const_int_operand" "")   ; size
-   (match_operand 3 "const_int_operand" ""))) ; start
-   (clobber (reg:CC CC_REGNUM))]
-  "TARGET_Z10"
-  "risbg\t%0,%1,64-%2,128+63,+%3+%2" ; dst, src, start, end, shift
+; 64 bit: (a & -16) | ((b >> 8) & 15)
+(define_insn "*extzvdi_lshiftrt"
+  [(set (zero_extract:DI (match_operand:DI 0 "register_operand" "+d")
+(match_operand 1 "const_int_operand" "")  ; size
+(match_operand 2 "const_int_operand" "")) ; start
+   (lshiftrt:DI (match_operand:DI 3 "register_operand" "d")
+(match_operand:DI 4 "nonzero_shift_count_operand" "")))]
+  "
+   && 64 - UINTVAL (operands[4]) >= UINTVAL (operands[1])"
+  "\t%0,%3,%2,%2+%1-1,128-%2-%1-%4"
+  [(set_attr "op_type" "RIE")
+   (set_attr "z10prop" "z10_super_E1")])
+
+; 32 bit: (a & -16) | ((b >> 8) & 15)
+(define_insn "*_ior_and_sr_ze"
+  [(set (match_operand:SI 0 "register_operand" "=d")
+   (ior:SI (and:SI
+(match_operand:SI 1 "register_operand" "0")
+

Re: [accaf, fortran, 4/4] Allocatable components support in derived typed coarrays

[Andre Vehreschild]

Hi Andreas,

thanks for the report. I better use + there instead of the fixed number of
digits. Fixed as obvious as r240262.

Thanks again.

- Andre

On Tue, 20 Sep 2016 14:18:46 +0200
Andreas Schwab  wrote:

> On Sep 19 2016, Andre Vehreschild  wrote:
> 
> > Index: gcc/testsuite/gfortran.dg/coarray_allocate_7.f08
> > ===
> > --- gcc/testsuite/gfortran.dg/coarray_allocate_7.f08(nicht existent)
> > +++ gcc/testsuite/gfortran.dg/coarray_allocate_7.f08(Arbeitskopie)
> > @@ -0,0 +1,27 @@
> > +! { dg-do run }
> > +! { dg-options "-fcoarray=lib -lcaf_single -fdump-tree-original" }
> > +
> > +! Contributed by Damian Rouson
> > +! Checking whether (de-)registering of coarrays works.
> > +
> > +program main
> > +
> > +  implicit none
> > +
> > +  type mytype
> > +integer, allocatable :: indices(:)
> > +  end type
> > +
> > +  type(mytype), save :: object[*]
> > +  integer :: i,me
> > +
> > +  me=this_image() ! me is always 1 here
> > +  object%indices=[(i,i=1,me)]
> > +  if ( size(object%indices) /= 1 ) call abort()
> > +  ! therefore no array is present here and no array test needed.
> > +  if ( object%indices(1) /= 1 ) call abort()
> > +end program
> > +
> > +! { dg-final { scan-tree-dump-times "_gfortran_caf_register
> > \\(D.\[0-9\]{4}, 1, &\\(\\(struct mytype\\) \\*object\\).indices.token,
> > &\\(\\(struct mytype\\) \\*object\\).indices, 0B, 0B, 0\\);" 2
> > "original" } } +! { dg-final { scan-tree-dump-times
> > "_gfortran_caf_deregister \\(&\\(\\(struct mytype\\)
> > \\*object\\).indices.token, 0B, 0B, 0\\);" 1 "original" } }
> > +  
> 
> FAIL: gfortran.dg/coarray_allocate_7.f08   -O0   scan-tree-dump-times
> original "_gfortran_caf_register \\(D.[0-9]{4}, 1, &\\(\\(struct mytype\\)
> \\*object\\).indices.token, &\\(\\(struct mytype\\) \\*object\\).indices, 0B,
> 0B, 0\\);" 2 PASS: gfortran.dg/coarray_allocate_7.f08   -O0
> scan-tree-dump-times original "_gfortran_caf_deregister \\(&\\(\\(struct
> mytype\\) \\*object\\).indices.token, 0B, 0B, 0\\);" 1
> 
> $ grep _gfortran_caf_register coarray_allocate_7.f08.003t.original 
>   _gfortran_caf_register (28, 0, (void * *) &caf_token.0, (void *) &desc.2,
> 0B, 0B, 0); _gfortran_caf_register (D.986, 1, &((struct mytype)
> *object).indices.token, &((struct mytype) *object).indices, 0B, 0B, 0);
> _gfortran_caf_register (D.986, 1, &((struct mytype) *object).indices.token,
> &((struct mytype) *object).indices, 0B, 0B, 0);
> 
> Andreas.
> 


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 
Index: gcc/testsuite/ChangeLog
===
--- gcc/testsuite/ChangeLog	(Revision 240261)
+++ gcc/testsuite/ChangeLog	(Arbeitskopie)
@@ -1,3 +1,8 @@
+2016-09-20  Andre Vehreschild  
+
+	* gfortran.dg/coarray_allocate_7.f08: Using + instead of fixed number
+	of digits expected.
+
 2016-09-20  Richard Biener  
 
 	PR tree-optimization/77646
Index: gcc/testsuite/gfortran.dg/coarray_allocate_7.f08
===
--- gcc/testsuite/gfortran.dg/coarray_allocate_7.f08	(Revision 240261)
+++ gcc/testsuite/gfortran.dg/coarray_allocate_7.f08	(Arbeitskopie)
@@ -22,6 +22,6 @@
   if ( object%indices(1) /= 1 ) call abort()
 end program
 
-! { dg-final { scan-tree-dump-times "_gfortran_caf_register \\(D.\[0-9\]{4}, 1, &\\(\\(struct mytype\\) \\*object\\).indices.token, &\\(\\(struct mytype\\) \\*object\\).indices, 0B, 0B, 0\\);" 2 "original" } }
+! { dg-final { scan-tree-dump-times "_gfortran_caf_register \\(D.\[0-9\]+, 1, &\\(\\(struct mytype\\) \\*object\\).indices.token, &\\(\\(struct mytype\\) \\*object\\).indices, 0B, 0B, 0\\);" 2 "original" } }
 ! { dg-final { scan-tree-dump-times "_gfortran_caf_deregister \\(&\\(\\(struct mytype\\) \\*object\\).indices.token, 0B, 0B, 0\\);" 1 "original" } }
 

Re: Re: [PATCH, 5.x/6.x/7.x] Be more conservative in early inliner if FDO is enabled

[Yuan, Pengfei]

> >> > Btw, It occurs to me that then win in code-size might be purely due to 
> >> > the
> >> > smaller base value for the TU size we use to compute the maximum unit
> >> > growth with ... any idea how to improve it on this side?  Say, computing
> >> > the TU size before early optimization (uh, probably not ...)
> >> >
> >> > That said, the inliner always completely fills its budged, that is, 
> >> > increase
> >> > the unit by max-unit-growth?
> >>
> >> What I'm trying to say is that rather than limiting early inlining we 
> >> should
> >> maybe decrease inline-unit-growth when FDO is in effect?  Because we
> >> can better control where the inlining goes.  If there is over 8% reduction
> >> in size benchmarking (unpatched) compiler on Firefox with FDO and
> >> --param inline-unit-growth=12 might show if the results are the same.
> >>
> >> Richard.
> >>
> >> > Richard.
> >
> > AFAIK, param inline-unit-growth only affects pass_ipa_inline. However, the 
> > code
> > size reduction achieved with my patch is from pass_early_inline.
> 
> Any inlining you don't do at early inlining time will a) decrease the
> size inline-unit-growth
> is computed on b) just delays inlining to IPA inlining (with the now
> more constrained size budget).

Delaying inlining from early inlining to IPA inlining can save size because 
profile
feedback is effective at IPA inlining and inlining of cold functions can be 
avoided.

Otherwise, inlining done at early inlining can not be canceled later.

Yuan, Pengfei

> Richard.
> 
> > Yuan, Pengfei

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

On Tue, 20 Sep 2016, Martin Sebor wrote:

> That said, since this specifier formats a vec, it seems that
> it might be useful to be able to format vectors of other elements,
> such as short and long.  With that in mind, would adding a new V
> length modifier instead be a more regular way to extend the pretty
> printer to these types?  The V length modifier would have to be
> accepted in conjunction with other length modifiers (h, l, etc
> and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept

That's much harder to support in format checking (which expects one length 
modifier, not a combination like that).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

On Tue, 20 Sep 2016, Prathamesh Kulkarni wrote:

> Could someone please take a look at the change to c-format.c, I am not sure
> if I have added that correctly.

Any changes to these GCC formats also require tests to be updated 
(gcc.dg/format/gcc_diag*).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] manual: _Bool has trap representations

[Joseph Myers]

On Tue, 20 Sep 2016, Florian Weimer wrote:

> On 09/19/2016 11:26 PM, Joseph Myers wrote:
> > On powerpc-darwin _Bool is 4-byte not 1-byte, so saying values are
> > represented as bytes isn't accurate for all systems supported by GCC.
> 
> Interesting.
> 
> Is the treatment of 0/1/the rest still the same there?

Yes.

-- 
Joseph S. Myers
jos...@codesourcery.com

[PATCH][simple-object] Early LTO debug "debug copier"

[Richard Biener]

This implements (for ELF sofar) the Early LTO debug information copier.
To remind people what this is about with Early LTO Debug the compile-stage
emits the DWARF DIEs as they are at dwarf2out_early_finish into the
object files that also contain the LTO IL.  On ELF they end up in
sections prefixed by .gnu.debuglto_ but otherwise with the usual
DWARF debug section names and flags.  They can appear alongside regular
DWARF debug sections for the fat part of the object (if compiled with
-flto -ffat-lto-objects - the default on targets where linker plugin
support is not available).

Simple C hello world compiled with -g -flto -ffat-lto-objects:

Section Headers:
  [Nr] Name  TypeAddress  OffSize   ES 
Flg Lk Inf Al
  [ 0]   NULL 00 00 00  
0   0  0
  [ 1] .text PROGBITS 40 c6 00  
AX  0   0  1
  [ 2] .data PROGBITS 000106 00 00  
WA  0   0  1
  [ 3] .bss  NOBITS   000106 00 00  
WA  0   0  1
  [ 4] .gnu.debuglto_.debug_info PROGBITS 000106 
99 00   E  0   0  1
  [ 5] .rela.gnu.debuglto_.debug_info RELA 
0011b0 c0 18   I 27   4  8
  [ 6] .gnu.debuglto_.debug_abbrev PROGBITS 00019f 
88 00   E  0   0  1
  [ 7] .gnu.debuglto_.debug_str PROGBITS 000227 
bf 01 MSE  0   0  1
  [ 8] .gnu.lto_.inline.34f47533603cde40 PROGBITS 
0002e6 2b 00   E  0   0  1
  [ 9] .gnu.lto_main.34f47533603cde40 PROGBITS 
000311 0004db 00   E  0   0  1
  [10] .gnu.lto_.symbol_nodes.34f47533603cde40 PROGBITS
 0007ec 4c 00   E  0   0  1
  [11] .gnu.lto_.refs.34f47533603cde40 PROGBITS 
000838 0f 00   E  0   0  1
  [12] .gnu.lto_.decls.34f47533603cde40 PROGBITS 
000847 00039d 00   E  0   0  1
  [13] .gnu.lto_.symtab.34f47533603cde40 PROGBITS 
000be4 14 00   E  0   0  1
  [14] .gnu.lto_.optsPROGBITS 000bf8 b5 00   
E  0   0  1
  [15] .debug_info   PROGBITS 000cad e1 00  
0   0  1
  [16] .rela.debug_info  RELA 001270 000120 18   
I 27  15  8
  [17] .debug_abbrev PROGBITS 000d8e a1 00  
0   0  1
  [18] .debug_arangesPROGBITS 000e2f 30 00  
0   0  1
  [19] .rela.debug_aranges RELA 001390 30 
18   I 27  18  8
  [20] .debug_line   PROGBITS 000e5f 4f 00  
0   0  1
  [21] .rela.debug_line  RELA 0013c0 18 18   
I 27  20  8
  [22] .comment  PROGBITS 000eae 42 01  
MS  0   0  1
  [23] .note.GNU-stack   PROGBITS 000ef0 00 00  
0   0  1
  [24] .eh_frame PROGBITS 000ef0 40 00   
A  0   0  8
  [25] .rela.eh_frameRELA 0013d8 18 18   
I 27  24  8
  [26] .shstrtab STRTAB   0013f0 0001b9 00  
0   0  1
  [27] .symtab   SYMTAB   000f30 000258 18 
28  22  8
  [28] .strtab   STRTAB   001188 25 00  
0   0  1

Symbol table '.symtab' contains 25 entries:
   Num:Value  Size TypeBind   Vis  Ndx Name
...
22:  0 NOTYPE  WEAK   HIDDEN 4 gt.c.675919dd
23:    198 FUNCGLOBAL DEFAULT1 main
24: 0001 1 OBJECT  GLOBAL DEFAULT  COM __gnu_lto_v1

important parts to notice is that there are .gnu.debuglto_ sections
with relocations and with symbols we have to preserve.

The task of the simple-object worker is to copy the .gnu.debuglto_
sections to a new object, stripping the section name mangling
(and removing the Exclude flag).  The result of the operation to the
above sections is

Section Headers:
  [Nr] Name  TypeAddress  OffSize   ES 
Flg Lk Inf Al
  [ 0]   NULL 00 00 00  
0   0  0
  [ 1]   NULL 0007c0 00 00   
E  0   0  1
  [ 2]   NULL 0007c0 00 00   
E  0   0  1
  [ 3]   NULL 0007c0 00 00   
E  0   0  1
  [ 4] .debug_info   PROGBITS 0007c0 99 00  
0   0  1
  [ 5] .rela.gnu.debuglto_.debug_info RELA 
000860 c0 18   I 27   4  8
  [ 6] .debug_abbrev PROGBITS 000920 88 00  
0   0  1
  [ 7] .debug_strPROGBITS 0009a8 bf 01  
MS  0

Re: [GCC][PATCH] Add __artificial__ attribute to Aarch64 NEON intrinsics

[James Greenhalgh]

On Tue, Sep 20, 2016 at 10:09:14AM +0100, James Greenhalgh wrote:
> On Tue, Sep 13, 2016 at 01:15:39PM +0100, Tamar Christina wrote:
>>
> Thanks, I applied this following your script above, and committed it as
> revision 240256, after tweaking one part of the script to replace a double
> space after __attribute__ introduced by the second sed command.

This patch caused a few new failures for AArch64:

  Failures:
gcc.target/aarch64/vect_int32x2x4_1.c
gcc.target/aarch64/vdup_lane_2.c
gcc.target/aarch64/vect_combine_zeroes_1.c

  Bisected to: 


  Author: jgreenhalgh 
  Date:   Tue Sep 20 09:06:13 2016 +

[GCC][PATCH] Add __artificial__ attribute to Aarch64 NEON intrinsics

Committed on behalf of Tamar Christina .

gcc/

* config/aarch64/arm_neon.h: Add gnu_inline and artificial
attributes to all inlined functions and make them extern.



git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@240256 
138bc75d-0d04-0410-961f-82ee72b054a4

Looking in to it, I see that the following intrinsics:

  vst2_s64
  vst2_u64
  vst2_f64
  vst2_s8
  vst3_s64
  vst3_u64
  vst3_f64
  vst3_s8
  vst4_s64
  vst4_u64
  vst4_f64
  vst4_s8

Are getting inlined, but their body is output in the assembly.

Looking in arm_neon.h, the reason seems clear:

  __extension__ extern __inline void
  vst2_s64 (int64_t * __a, int64x1x2_t val)
  {

These intrinsics have always been missing
__attribute__ ((__always_inline__)) and consequently were not updated by your
script to have __gnu_inline__, and now they are marked extern - they miss GNU
Inline semantics and keep their body after inlining.

A patch adding the correct attributes to these intrisnics is preapproved.

Thanks,
James

PR77661 - Fix: --enable-maintainer-mode causes in-tree-build of MPC to fail

[Tobias Burnus]

Currently, --enable-maintainer-mode causes an in-tree build MPC to fail (cf. 
PR).
The attached patch fixes this for --enable-maintainer-mode.

OK for the trunk?

Cheers,

Tobias

PS: Thanks to Richard for the suggestion.
	PR boot/77661
	* Makefile.def: Don't pass --enable-maintainer-mode on to an
	in-tree build MPC.
	* Makefile.in: Regenerate.

diff --git a/Makefile.def b/Makefile.def
index 05316a4..3b3f3ea 100644
--- a/Makefile.def
+++ b/Makefile.def
@@ -65,3 +65,3 @@ host_modules= { module= mpfr; lib_path=src/.libs; bootstrap=true;
 host_modules= { module= mpc; lib_path=src/.libs; bootstrap=true;
-		extra_configure_flags='--disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@';
+		extra_configure_flags='--disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode';
 		no_install= true; };
diff --git a/Makefile.in b/Makefile.in
index 117fbf5..9005405 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -14101,3 +14101,3 @@ configure-mpc:
 	  $(HOST_CONFIGARGS) --build=${build_alias} --host=${host_alias} \
-	  --target=${target_alias} --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ \
+	  --target=${target_alias} --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode \
 	  || exit 1
@@ -14137,3 +14137,3 @@ configure-stage1-mpc:
 	  $(STAGE1_CONFIGURE_FLAGS) \
-	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@
+	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode
 @endif mpc-bootstrap
@@ -14171,3 +14171,3 @@ configure-stage2-mpc:
 	  $(STAGE2_CONFIGURE_FLAGS) \
-	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@
+	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode
 @endif mpc-bootstrap
@@ -14205,3 +14205,3 @@ configure-stage3-mpc:
 	  $(STAGE3_CONFIGURE_FLAGS) \
-	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@
+	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode
 @endif mpc-bootstrap
@@ -14239,3 +14239,3 @@ configure-stage4-mpc:
 	  $(STAGE4_CONFIGURE_FLAGS) \
-	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@
+	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode
 @endif mpc-bootstrap
@@ -14273,3 +14273,3 @@ configure-stageprofile-mpc:
 	  $(STAGEprofile_CONFIGURE_FLAGS) \
-	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@
+	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode
 @endif mpc-bootstrap
@@ -14307,3 +14307,3 @@ configure-stagefeedback-mpc:
 	  $(STAGEfeedback_CONFIGURE_FLAGS) \
-	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@
+	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode
 @endif mpc-bootstrap
@@ -14341,3 +14341,3 @@ configure-stageautoprofile-mpc:
 	  $(STAGEautoprofile_CONFIGURE_FLAGS) \
-	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@
+	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode
 @endif mpc-bootstrap
@@ -14375,3 +14375,3 @@ configure-stageautofeedback-mpc:
 	  $(STAGEautofeedback_CONFIGURE_FLAGS) \
-	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@
+	  --disable-shared @extra_mpc_gmp_configure_flags@ @extra_mpc_mpfr_configure_flags@ --disable-maintainer-mode
 @endif mpc-bootstrap

Re: PR77661 - Fix: --enable-maintainer-mode causes in-tree-build of MPC to fail

[Richard Biener]

On Tue, Sep 20, 2016 at 3:21 PM, Tobias Burnus
 wrote:
> Currently, --enable-maintainer-mode causes an in-tree build MPC to fail (cf. 
> PR).
> The attached patch fixes this for --enable-maintainer-mode.
>
> OK for the trunk?

Ok if nobody objects within 48 hours.

Richard.

> Cheers,
>
> Tobias
>
> PS: Thanks to Richard for the suggestion.

Re: [v3 PATCH] PR libstdc++/77619

[Jonathan Wakely]

On 18/09/16 21:07 +0300, Ville Voutilainen wrote:

diff --git a/libstdc++-v3/include/bits/stl_construct.h 
b/libstdc++-v3/include/bits/stl_construct.h
index 3d12628..c7ca1f8 100644
--- a/libstdc++-v3/include/bits/stl_construct.h
+++ b/libstdc++-v3/include/bits/stl_construct.h
@@ -83,6 +83,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  ::new(static_cast(__p)) _T1(__value);
}
#endif


Blank line here please.


+  template
+inline void
+_Construct_novalue(_T1* __p)
+{ ::new(static_cast(__p)) _T1; }

  /**
   * Destroy the object pointed to by a pointer type.




diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h 
b/libstdc++-v3/include/bits/stl_uninitialized.h
index c5c81fb..b4213d5 100644
--- a/libstdc++-v3/include/bits/stl_uninitialized.h
+++ b/libstdc++-v3/include/bits/stl_uninitialized.h
@@ -640,6 +644,104 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
allocator<_Tp>&)
{ return std::__uninitialized_default_n(__first, __n); }

+  template
+struct __uninitialized_default_novalue_1
+{
+  template
+static void
+__uninit_default_novalue(_ForwardIterator __first,
+_ForwardIterator __last)
+{

 ...

+   }


The left brace is indented with spaces and the right one with a tab,
might as well make them the same.


+};
+
+  template<>
+struct __uninitialized_default_novalue_1
+{
+  template
+static void
+__uninit_default_novalue(_ForwardIterator __first,
+_ForwardIterator __last)
+{
+   }


Ditto.


+};
+
+  template
+struct __uninitialized_default_novalue_n_1
+{
+  template
+static _ForwardIterator
+__uninit_default_novalue_n(_ForwardIterator __first, _Size __n)
+{
+ _ForwardIterator __cur = __first;
+ __try
+   {
+ for (; __n > 0; --__n, ++__cur)
+   std::_Construct_novalue(std::__addressof(*__cur));
+ return __cur;
+   }
+ __catch(...)
+   {
+ std::_Destroy(__first, __cur);
+ __throw_exception_again;
+   }
+   }


Ditto.


+};
+
+  template<>
+struct __uninitialized_default_novalue_n_1
+{
+  template
+static _ForwardIterator
+__uninit_default_novalue_n(_ForwardIterator __first, _Size __n)
+{
+   }


Ditto.


+};
+
+  // __uninitialized_default_novalue
+  // Fills [first, last) with std::distance(first, last) default-initialized
+  // value_types(s).
+  template
+inline void
+__uninitialized_default_novalue(_ForwardIterator __first,
+   _ForwardIterator __last)
+{
+  typedef typename iterator_traits<_ForwardIterator>::value_type
+   _ValueType;
+  // trivial types can have deleted assignment
+  const bool __assignable = is_copy_assignable<_ValueType>::value;


Aha! A non-whitespace comment ... __assignable isn't used. If it's not
needed it can be removed.


+  std::__uninitialized_default_novalue_1<
+   is_trivially_default_constructible<_ValueType>::value>::
+   __uninit_default_novalue(__first, __last);
+}
+
+  // __uninitialized_default_n
+  // Fills [first, first + n) with n default-initialized value_type(s).
+  template
+inline _ForwardIterator
+__uninitialized_default_novalue_n(_ForwardIterator __first, _Size __n)
+{
+  typedef typename iterator_traits<_ForwardIterator>::value_type
+   _ValueType;
+  // trivial types can have deleted assignment
+  const bool __assignable = is_copy_assignable<_ValueType>::value;


Ditto.


+  return __uninitialized_default_novalue_n_1<
+   is_trivially_default_constructible<_ValueType>::value>::
+   __uninit_default_novalue_n(__first, __n);
+}

  template
@@ -669,6 +771,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   random_access_iterator_tag)
{ return std::uninitialized_copy(__first, __first + __n, __result); }

+  template
+pair<_InputIterator, _ForwardIterator>
+__uninitialized_copy_n_pair(_InputIterator __first, _Size __n,
+  _ForwardIterator __result, input_iterator_tag)
+{
+  _ForwardIterator __cur = __result;
+  __try
+   {
+ for (; __n > 0; --__n, ++__first, ++__cur)
+   std::_Construct(std::__addressof(*__cur), *__first);
+ return {__first, __cur};
+   }
+  __catch(...)
+   {
+ std::_Destroy(__result, __cur);
+ __throw_exception_again;
+   }
+}
+
+  template
+inline pair<_RandomAccessIterator, _ForwardIterator>
+__uninitialized_copy_n_pair(_RandomAccessIterator __first, _Size __n,
+  _ForwardIterator __result,
+  random_access_iterator_tag)
+{
+  auto second = uninitialized_copy(__first, __first + __n, __result);
+  auto first = std::next(__first, __n

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/20/16 13:29, Kyrill Tkachov wrote:
>
> arm bootstrap is now failing:
> $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in
> boolean context [-Werror=int-in-bool-context]
>  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
> ~^~
> $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro
> 'TARGET_ARM_FP'
> if (TARGET_ARM_FP)
>
>
> The full definition of TARGET_ARM_FP is:
> #define TARGET_ARM_FP\
>(!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
>  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>: 0)
>
> We want it set to 0 when there's no FP but when FP is available we set
> it to a bitmask
> to suggest the level that is available. That seems like a legitimate use
> to me.
>

Ok, I see, sorry for that.

I think I will have to suppress the warning if the conditional is in
a macro somehow.

Can you work around that for a few days?


Bernd.

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Richard Earnshaw (lists)]

On 08/09/16 12:00, Kyrill Tkachov wrote:
> Hi all,
> 
> This is a rebase and slight respin of [1] that I sent out last November
> to change all uses of
> sprintf to snprintf in the arm backend.
> 
> Bootstrapped and tested on arm-none-linux-gnueabihf.
> Ok for trunk?
> 
> Thanks,
> Kyrill
> 
> [1] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html
> 
> 2016-09-08  Kyrylo Tkachov  
> 
> * config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
> rather than sprintf.
> (arm_set_fixed_conv_libfunc): Likewise.
> (arm_option_override): Likewise.
> (neon_output_logic_immediate): Likewise.
> (neon_output_shift_immediate): Likewise.
> (arm_output_multireg_pop): Likewise.
> (vfp_output_vstmd): Likewise.
> (output_move_vfp): Likewise.
> (output_move_neon): Likewise.
> (output_return_instruction): Likewise.
> (arm_elf_asm_cdtor): Likewise.
> (arm_output_shift): Likewise.
> (arm_output_iwmmxt_shift_immediate): Likewise.
> (arm_output_iwmmxt_tinsr): Likewise.
> * config/arm/neon.md (*neon_mov, VDX): Likewise.
> (*neon_mov, VQXMOV): Likewise.
> (neon_vc_insn): Likewise.
> (neon_vc_insn_unspec): Likewise.

Most of these should assert that truncation did not occur (it's an
internal error if the buffers aren't large enough).  In a few cases we
should call output_operand_lossage on failure since it might be due to a
user writing inline assembly and overflowing a buffer.

I don't think we should silently accept a truncated write.

R.

> 
> arm-snprintf.patch
> 
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 
> 946f308ca84e232af8af6eca4813464914cbd59c..8ff6c0e18f7c2a2eed72e56402ed582c9f692d2d
>  100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -2388,9 +2388,10 @@ arm_set_fixed_optab_libfunc (optab optable, 
> machine_mode mode,
>char buffer[50];
>  
>if (num_suffix == 0)
> -sprintf (buffer, "__gnu_%s%s", funcname, modename);
> +snprintf (buffer, sizeof (buffer), "__gnu_%s%s", funcname, modename);
>else
> -sprintf (buffer, "__gnu_%s%s%d", funcname, modename, num_suffix);
> +snprintf (buffer, sizeof (buffer), "__gnu_%s%s%d", funcname,
> +   modename, num_suffix);
>  
>set_optab_libfunc (optable, mode, buffer);
>  }
> @@ -2409,8 +2410,8 @@ arm_set_fixed_conv_libfunc (convert_optab optable, 
> machine_mode to,
>&& ALL_FRACT_MODE_P (from) == ALL_FRACT_MODE_P (to))
>  maybe_suffix_2 = "2";
>  
> -  sprintf (buffer, "__gnu_%s%s%s%s", funcname, fromname, toname,
> -maybe_suffix_2);
> +  snprintf (buffer, sizeof (buffer), "__gnu_%s%s%s%s", funcname,
> + fromname, toname, maybe_suffix_2);
>  
>set_conv_libfunc (optable, to, from, buffer);
>  }
> @@ -3163,7 +3164,8 @@ arm_option_override (void)
>if (!arm_selected_tune)
>  arm_selected_tune = &all_cores[arm_selected_cpu->core];
>  
> -  sprintf (arm_arch_name, "__ARM_ARCH_%s__", arm_selected_cpu->arch);
> +  snprintf (arm_arch_name, sizeof (arm_arch_name),
> + "__ARM_ARCH_%s__", arm_selected_cpu->arch);
>insn_flags = arm_selected_cpu->flags;
>arm_base_arch = arm_selected_cpu->base_arch;
>  
> @@ -12735,9 +12737,11 @@ neon_output_logic_immediate (const char *mnem, rtx 
> *op2, machine_mode mode,
>gcc_assert (is_valid != 0);
>  
>if (quad)
> -sprintf (templ, "%s.i%d\t%%q0, %%2", mnem, width);
> +snprintf (templ, sizeof (templ), "%s.i%d\t%%q0, %%2",
> +   mnem, width);
>else
> -sprintf (templ, "%s.i%d\t%%P0, %%2", mnem, width);
> +snprintf (templ, sizeof (templ), "%s.i%d\t%%P0, %%2",
> +   mnem, width);
>  
>return templ;
>  }
> @@ -12757,9 +12761,11 @@ neon_output_shift_immediate (const char *mnem, char 
> sign, rtx *op2,
>gcc_assert (is_valid != 0);
>  
>if (quad)
> -sprintf (templ, "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
> +snprintf (templ, sizeof (templ),
> +   "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
>else
> -sprintf (templ, "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
> +snprintf (templ, sizeof (templ),
> +   "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
>  
>return templ;
>  }
> @@ -17858,17 +17864,17 @@ arm_output_multireg_pop (rtx *operands, bool 
> return_pc, rtx cond, bool reverse,
>conditional = reverse ? "%?%D0" : "%?%d0";
>/* Can't use POP if returning from an interrupt.  */
>if ((regno_base == SP_REGNUM) && update && !(interrupt_p && return_pc))
> -sprintf (pattern, "pop%s\t{", conditional);
> +snprintf (pattern, sizeof (pattern), "pop%s\t{", conditional);
>else
>  {
>/* Output ldmfd when the base register is SP, otherwise output ldmia.
>   It's just a convention, their semantics are identical.  */
>if (regno_base == SP_REGNUM)
> - sprintf (pattern, "ldmfd%s\t", conditional);
> + snprintf (pattern, sizeof (pattern), "ldmfd%s\t", conditional);
>else if (update)
> - spri

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Kyrill Tkachov]

On 20/09/16 15:11, Bernd Edlinger wrote:

On 09/20/16 13:29, Kyrill Tkachov wrote:

arm bootstrap is now failing:
$SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in
boolean context [-Werror=int-in-bool-context]
  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
 ~^~
$SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro
'TARGET_ARM_FP'
 if (TARGET_ARM_FP)


The full definition of TARGET_ARM_FP is:
#define TARGET_ARM_FP\
(!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
: 0)

We want it set to 0 when there's no FP but when FP is available we set
it to a bitmask
to suggest the level that is available. That seems like a legitimate use
to me.


Ok, I see, sorry for that.

I think I will have to suppress the warning if the conditional is in
a macro somehow.

Can you work around that for a few days?


I changed to:
 if (TARGET_ARM_FP != 0)
in my tree to allow the build to proceed but encountered another bootstrap 
failure:
In file included from ./tm.h:38:0,
 from $SRC/gcc/backend.h:28,
 from $SRC/gcc/regrename.c:23:
$SRC/gcc/regrename.c: In function 'void rename_chains()':
$SRC/gcc/config/arm/arm.h:915:4: error: ?: using integer constants in boolean 
context [-Werror=int-in-bool-context]
   (TARGET_ARM \
   ~
? ARM_HARD_FRAME_POINTER_REGNUM  \
^~
: THUMB_HARD_FRAME_POINTER_REGNUM)
~~
$SRC/gcc/regrename.c:484:8: note: in expansion of macro 
'HARD_FRAME_POINTER_REGNUM'
|| (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
^
That looks like a legitimate bug in regrename.c ?
The full condition is:
   if (fixed_regs[reg] || global_regs[reg]
   || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
   && reg == HARD_FRAME_POINTER_REGNUM)
   || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
   && reg == FRAME_POINTER_REGNUM))

The condition in the second || looks bogus (what use testing if a register is 
'non-zero').

So this looks like a useful catch by the warning, though I'm not sure at first 
glance how
to fix it.
Kyrill



Bernd.

Re: PR35503 - warn for restrict pointer

[Martin Sebor]

On 09/20/2016 07:00 AM, Joseph Myers wrote:

On Tue, 20 Sep 2016, Martin Sebor wrote:


That said, since this specifier formats a vec, it seems that
it might be useful to be able to format vectors of other elements,
such as short and long.  With that in mind, would adding a new V
length modifier instead be a more regular way to extend the pretty
printer to these types?  The V length modifier would have to be
accepted in conjunction with other length modifiers (h, l, etc
and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept


That's much harder to support in format checking (which expects one length
modifier, not a combination like that).


I haven't looked into it detail but since the format checker supports
one-to-two character sequences of length modifiers (h or hh, etc) it
should be possible to extend it to handle one-to-three character
sequences (h, hV, or hhV, etc.)  The V character would not be a new
length modifier on its own but instead be recognized as part of
a longer length modifier sequence.  Do you see a problem with that?

Martin

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

On Tue, 20 Sep 2016, Martin Sebor wrote:

> > That's much harder to support in format checking (which expects one length
> > modifier, not a combination like that).
> 
> I haven't looked into it detail but since the format checker supports
> one-to-two character sequences of length modifiers (h or hh, etc) it
> should be possible to extend it to handle one-to-three character
> sequences (h, hV, or hhV, etc.)  The V character would not be a new
> length modifier on its own but instead be recognized as part of
> a longer length modifier sequence.  Do you see a problem with that?

You could arrange for length modifiers to have extra variants like that 
(at present they have single and double variants listed), but it's clearly 
more complicated than a modifier that's used on its own.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Implement -Wimplicit-fallthrough (version 8)

[Marek Polacek]

On Fri, Sep 09, 2016 at 06:44:44PM +0200, Jakub Jelinek wrote:
> Hi!
 
Finally getting back to this...

> On Thu, Sep 01, 2016 at 03:40:49PM +0200, Marek Polacek wrote:
> > @@ -1749,6 +1758,16 @@ c_parser_declaration_or_fndef (c_parser *parser, 
> > bool fndef_ok,
> >  {
> >if (auto_type_p)
> > error_at (here, "%<__auto_type%> in empty declaration");
> > +  else if (specs->typespec_kind == ctsk_none && specs->attrs
> > +  && is_attribute_p ("fallthrough",
> > + get_attribute_name (specs->attrs)))
> > +   {
> > + if (fallthru_attr_p != NULL)
> > +   *fallthru_attr_p = true;
> 
> What happens if there are more than one attribute in specs->attrs?
> Either fallthrough not being the first one, or there are other attributes
> afterwards?
> Like:
>   __attribute__((unused, fallthrough)) ;
> or
>   __attribute__((fallthrough, unused)) ;
 
I only accepted __attribute__((fallthrough)); or __attribute__((fallthrough,
unused));, but that is probably wrong.  Fixed in another version of the patch,
where I introduced maybe_attribute_fallthrough_p that detects various valid
and invalid cases.

> > + tree fn = build_call_expr_internal_loc (here, IFN_FALLTHROUGH,
> > + void_type_node, 0);
> > + add_stmt (fn);
> > +   }
> >else if (empty_ok)
> > shadow_tag (specs);
> >else
> > @@ -4845,9 +4864,11 @@ c_parser_compound_statement_nostart (c_parser 
> > *parser)
> > {
> >   last_label = false;
> >   mark_valid_location_for_stdc_pragma (false);
> > + bool fallthru_attr_p = false;
> >   c_parser_declaration_or_fndef (parser, true, true, true, true,
> > -true, NULL, vNULL);
> > - if (last_stmt)
> > +true, NULL, vNULL, NULL,
> > +&fallthru_attr_p);
> > + if (last_stmt && !fallthru_attr_p)
> > pedwarn_c90 (loc, OPT_Wdeclaration_after_statement,
> >  "ISO C90 forbids mixed declarations and code");
> >   last_stmt = false;
> 
> So, for fallthru_attr_p here you have a guarantee it has been
> __attribute__((fallthrough)) ; which is actualy not a declaration, but
> (empty) statement?  If so, I'd say that for fallthru_attr_p in that case you
> don't want to clear last_stmt, but want to set it (or let it be unchanged?).
> I mean:
>   int a;
>   a = 5;
>   __attribute__((fallthrough)) ;
>   int b;
> would for -std=c99 -pedantic-errors not error out on the declaration of b
> being after the statements (a = 5 and empty stmt).
> What about
>   int a;
>   __attribute__((fallthrough)) ;
>   int b;
> ?  Shall we error on that too?  Pedantically it is also a declaration after
> a statement.
 
Yeah, fixed now.

> > @@ -5327,6 +5360,27 @@ c_parser_statement_after_labels (c_parser *parser, 
> > bool *if_p,
> >   gcc_assert (c_dialect_objc ());
> >   c_parser_objc_synchronized_statement (parser);
> >   break;
> > +   case RID_ATTRIBUTE:
> > + {
> > +   /* Allow '__attribute__((fallthrough));'.  */
> > +   tree attrs = c_parser_attributes (parser);
> > +   if (attrs != NULL_TREE
> > +   && is_attribute_p ("fallthrough", get_attribute_name (attrs)))
> 
> See above.
 
Fixed with maybe_attribute_fallthrough_p.

> > --- gcc/gcc/cp/parser.c
> > +++ gcc/gcc/cp/parser.c
> > @@ -5135,6 +5135,31 @@ cp_parser_primary_expression (cp_parser *parser,
> > case RID_AT_SELECTOR:
> >   return cp_parser_objc_expression (parser);
> >  
> > +   case RID_ATTRIBUTE:
> > + {
> > +   /* This might be __attribute__((fallthrough));.  */
> > +   tree attr = cp_parser_gnu_attributes_opt (parser);
> > +   if (attr != NULL_TREE
> > +   && is_attribute_p ("fallthrough", get_attribute_name (attr)))
> 
> Again.

Fixed with maybe_attribute_fallthrough_p.

> > @@ -10585,14 +10610,25 @@ cp_parser_statement (cp_parser* parser, tree 
> > in_statement_expr,
> > }
> >/* Look for an expression-statement instead.  */
> >statement = cp_parser_expression_statement (parser, 
> > in_statement_expr);
> > +
> > +  /* Handle [[fallthrough]];.  */
> > +  if (std_attrs != NULL_TREE
> > + && is_attribute_p ("fallthrough", get_attribute_name (std_attrs))
> > + && statement == NULL_TREE)
> 
> And again.
 
Fixed with maybe_attribute_fallthrough_p.

> > +   {
> > + statement = build_call_expr_internal_loc (statement_location,
> > +   IFN_FALLTHROUGH,
> > +   void_type_node, 0);
> > + finish_expr_stmt (statement);
> > + std_attrs = NULL_TREE;
> > +   }
> >  }
> >  
> > diff --git gcc/gcc/cp/pt.c gcc/gcc/cp/pt.c
> > index b0f0664..60c5d43 100644
> > --- gcc/gcc/cp/pt.c
> > +++ gcc/gcc/cp/pt.c
> > @@ -16556,7 +16556,9 @@ tsubst_copy_and_build (tree t,
> > tree ret;
> > 

Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)

[David Malcolm]

On Mon, 2016-09-19 at 11:35 -0600, Jeff Law wrote:
> On 09/16/2016 03:27 PM, David Malcolm wrote:
> > > We should also twiddle how we represent registers in the dumps.
> > > Identifying hard regs by name (so we can map back to a hard reg
> > > if
> > > the
> > > hard regs change), identifying pseudos by number that isn't
> > > affected
> > > if
> > > the hard register set changes ie, p0, p1, p2, p3 where the number
> > > is
> > > REGNO (x) - FIRST_PSEUDO_REGISTER. identifying the virtual
> > > registers,
> > > etc.
> > 
> > Good idea.
> > 
> > I don't know if you saw this yet, but the patch has logic from
> > renumbering pseudos on load (see class regno_remapper), but dumping
> > them as p0, p1 etc and reloading them as such seems much easier for
> > everyone.
> And just an FYI, I think we should do this unconditionally in our
> dumps.

To summarize so far: you want every pseudo to have its regno dumped
with a 'p' prefix, and renumber them on dump (and then on load).
OK.

When dumping regnos, would you want another distinction between
virtuals and non-virtuals in the dump?  For example, given that
LAST_VIRTUAL_POINTER_REGISTER is ((FIRST_VIRTUAL_REGISTER) + 4), this
could mean:

  v0, v1, ..., v4  for the virtual regnos

and, for pseudos that aren't virtual regnos:
  p0, p1, ...
or
  p5, p6, ...
depending on whether we want to start numbering the pseudos at p0 for
LAST_VIRTUAL_REGISTER + 1, or whether we regard v0 as the first pseduo,
and hence p5 is the first non-virtual pseudo.   FWIW I think starting
at p5 is the better approach, given that:
  #define FIRST_VIRTUAL_REGISTER(FIRST_PSEUDO_REGISTER)

Either way, this would give things like:
  (reg/f:DI v1 virtual-stack-vars)
  (reg:DI p78)

In a similar vein, how about adding a "h" prefix for the hard regnos?
That way every regno would have a one-char prefix telling you what kind
of reg it is (and you can directly see whether or not you need to
offset the number by FIRST_PSEUDO_REGISTER to get at the "real" regno).

This would give things (for x86_64) like:
  (reg/i:SI h0 ax)
  (reg/i:SF h21 xmm0)

> > 
> > > The key being rather than put a ton of smarts/hacks in a reader,
> > > we
> > > should work to have the RTL writer give us something more useful.
> > >  That
> > > may mean simple changes to the output, or some conditional
> > > changes
> > > (like
> > > not emitting the INSN_CODE or its name).
> > 
> > That would make the reader a lot less grim; it has a lot of warts
> > for
> > dealing with all the special cases in the current output format.
> That's the idea.
> > 
> > But maybe it is useful to be able to deal with the current output
> > format.  For example, I was able to look at PR 71779 and find some
> > fragmentary RTL dumps there (comment #2) and use them.  I *did*
> > need to
> > edit them a little, so maybe it's OK to require a little editing
> > (especially with older dumps... to what extent has the format
> > changed
> > over the years?)
> It's changed, but not in radical ways.
> 
> I think the case we want to cater to is dumping something from the 
> current compiler rather than picking up some crusty RTL and creating
> a 
> testcase from that.  By "current" I explicitly want the ability to 
> refine the output to make the reader easier to implement.
> 
> Jeff

Re: Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)

[Bernd Schmidt]

On 09/20/2016 04:32 PM, David Malcolm wrote:


To summarize so far: you want every pseudo to have its regno dumped
with a 'p' prefix, and renumber them on dump (and then on load).
OK.


Renumbering is not helpful because it interferes with the view you have 
in the debugger. So, IMO just a prefix, and maybe



  (reg/f:DI v1 virtual-stack-vars)


this.


Bernd

Re: Implement -Wimplicit-fallthrough (version 8)

[Jakub Jelinek]

On Tue, Sep 20, 2016 at 04:24:35PM +0200, Marek Polacek wrote:
> > (to skip over the koenig_p etc. cases) and fallthrough into the argument
> > handling and add another if (function == NULL_TREE) handling after that,
> > which would just build another internal call with the tsubsted arguments.
>  
> I added the assert here, but I spent time implementing this, and it didn't
> really work, because without a testcase it was hard to say whether what I
> had was correct.  E.g., how to determine the return type of the internal
> function, etc.

I agree that without the testcase it is hard and that assert is good enough
for now.  For return type of the internal function, I'd just tsubst the
TREE_TYPE of the CALL_EXPR though.

Jakub

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/20/16 16:20, Kyrill Tkachov wrote:
>
> On 20/09/16 15:11, Bernd Edlinger wrote:
>> On 09/20/16 13:29, Kyrill Tkachov wrote:
>>> arm bootstrap is now failing:
>>> $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in
>>> boolean context [-Werror=int-in-bool-context]
>>>   : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>>>  ~^~
>>> $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro
>>> 'TARGET_ARM_FP'
>>>  if (TARGET_ARM_FP)
>>>
>>>
>>> The full definition of TARGET_ARM_FP is:
>>> #define TARGET_ARM_FP\
>>> (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
>>>   : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>>> : 0)
>>>
>>> We want it set to 0 when there's no FP but when FP is available we set
>>> it to a bitmask
>>> to suggest the level that is available. That seems like a legitimate use
>>> to me.
>>>
>> Ok, I see, sorry for that.
>>
>> I think I will have to suppress the warning if the conditional is in
>> a macro somehow.
>>
>> Can you work around that for a few days?
>
> I changed to:
>   if (TARGET_ARM_FP != 0)
> in my tree to allow the build to proceed but encountered another
> bootstrap failure:
> In file included from ./tm.h:38:0,
>   from $SRC/gcc/backend.h:28,
>   from $SRC/gcc/regrename.c:23:
> $SRC/gcc/regrename.c: In function 'void rename_chains()':
> $SRC/gcc/config/arm/arm.h:915:4: error: ?: using integer constants in
> boolean context [-Werror=int-in-bool-context]
> (TARGET_ARM \
> ~
>  ? ARM_HARD_FRAME_POINTER_REGNUM  \
>  ^~
>  : THUMB_HARD_FRAME_POINTER_REGNUM)
>  ~~
> $SRC/gcc/regrename.c:484:8: note: in expansion of macro
> 'HARD_FRAME_POINTER_REGNUM'
>  || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
>  ^
> That looks like a legitimate bug in regrename.c ?
> The full condition is:
> if (fixed_regs[reg] || global_regs[reg]
> || (!HARD_FRAME_POINTER_IS_FRAME_POINTER &&
> frame_pointer_needed
> && reg == HARD_FRAME_POINTER_REGNUM)
> || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
> && reg == FRAME_POINTER_REGNUM))
>
> The condition in the second || looks bogus (what use testing if a
> register is 'non-zero').
>
> So this looks like a useful catch by the warning, though I'm not sure at
> first glance how
> to fix it.
> Kyrill
>

I assume HARD_FRAME_POINTER_REGNUM is never zero.

That looks like it should be

  || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)


Bernd.

Re: [PATCH] Optimise the fpclassify builtin to perform integer operations when possible

[Jeff Law]

On 09/20/2016 06:00 AM, Tamar Christina wrote:



On 16/09/16 20:49, Jeff Law wrote:

On 09/12/2016 10:19 AM, Tamar Christina wrote:

Hi All,
+
+  /* Re-interpret the float as an unsigned integer type
+ with equal precision.  */
+  int_arg_type = build_nonstandard_integer_type (TYPE_PRECISION
(type), 0);
+  int_arg = fold_build1_loc (loc, INDIRECT_REF, int_arg_type,
+  fold_build1_loc (loc, NOP_EXPR,
+   build_pointer_type (int_arg_type),
+fold_build1_loc (loc, ADDR_EXPR,
+ build_pointer_type (type), arg)));

Doesn't this make ARG addressable?  Which in turn means ARG won't be
exposed to the gimple/ssa optimizers.Or is it the case that when
fpclassify is used its argument is already in memory (and thus
addressable?)


I believe that it is the case that when fpclassify is use the argument
is already addressable, but I am not 100% certain. I may be able to do
this differently so I'll come back to you on this one.
The more I think about it, the more I suspect ARG is only going to 
already be marked as addressable if it has already had its address taken.



But I think we can look at this as an opportunity.  If ARG is already 
addressable, then it's most likely going to be living in memory (there 
are exceptions).  If ARG is most likely going to be living in memory, 
then we clearly want to use your fast integer path, regardless of the 
target.


If ARG is not addressable, then it's not as clear as the object is 
likely going to be assigned into an FP register.  Integer operations on 
the an FP register likely will force a sequence where we dump the 
register into memory, load from memory into a GPR, then bit test on the 
GPR.  That gets very expensive on some architectures.


Could we defer lowering in the case where the object is not addressable 
until gimple->rtl expansion time?  That's the best time to introduce 
target dependencies into the code we generate.



Jeff

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Jason Merrill]

On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger
 wrote:
> On 09/20/16 13:29, Kyrill Tkachov wrote:
>>
>> arm bootstrap is now failing:
>> $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in
>> boolean context [-Werror=int-in-bool-context]
>>  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>> ~^~
>> $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro
>> 'TARGET_ARM_FP'
>> if (TARGET_ARM_FP)
>>
>>
>> The full definition of TARGET_ARM_FP is:
>> #define TARGET_ARM_FP\
>>(!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
>>  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>>: 0)
>>
>> We want it set to 0 when there's no FP but when FP is available we set
>> it to a bitmask
>> to suggest the level that is available. That seems like a legitimate use
>> to me.
>>
>
> Ok, I see, sorry for that.
>
> I think I will have to suppress the warning if the conditional is in
> a macro somehow.

from_macro_expansion_at will help with that.

Though it seems to me that the issue here is more that (TARGET_FP16 ?
14 : 12) is not in a boolean context, it's in an integer context; only
the outer ?: is in a boolean context.

I also still think the warning message should be changed.

Jason

Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments

[Jeff Law]

On 09/20/2016 04:38 AM, Bernd Schmidt wrote:



On 09/20/2016 01:16 AM, Segher Boessenkool wrote:

On Mon, Sep 19, 2016 at 06:07:46PM -0400, Trevor Saunders wrote:

- JUMP_LABEL can be a return which is not an insn.  That also seems
  like something that should be improved at some point.

The JUMP_LABEL field within a JUMP_INSN?  That sounds like a bug.


yes, see the comment before the declaration of
rtx_jump_insn::jump_label ()
it can be either a possibly deleted label, or a RETURN or SIMPLE_RETURN
expression.  Memory usage concerns aside its a tempting design to
change, but at the moment itts definitely intended to work this way,
there's plenty of code that checks ANY_RETURN_P (JUMP_LABEL (x)).


Yes.  But rtl.texi still says:

  Return insns count as jumps, but since they do not refer to any
  labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.


And the comment at the top of jump.c:

   Each CODE_LABEL has a count of the times it is used
   stored in the LABEL_NUSES internal field, and each JUMP_INSN
   has one label that it refers to stored in the
   JUMP_LABEL internal field.  With this we can detect labels that
   become unused because of the deletion of all the jumps that
   formerly used them.  The JUMP_LABEL info is sometimes looked
   at by later passes.  For return insns, it contains either a
   RETURN or a SIMPLE_RETURN rtx.

It's intentional, and really there's nothing wrong with it, or with
operands[] containing CODE_LABELs. The problem is trying to force a
static type system onto a dynamically typed data structure.
But what this doesn't answer is why we stuff RETURN/SIMPLE_RETURN into 
the JUMP_LABEL field of a return.  I simply can't remember any rationale 
behind that.


I suspect that if we dig further, we'll find that we can accomplish 
whatever the goal was behind that decision in a way that easily works in 
a world where we have separated rtx objects from objects that are part 
of the insn chain.


Just to be clear, I don't see us going to a world where everything we 
have as an rtx today is a statically typed object. But there are things 
that I think make sense to carve out, the most obvious being objects 
which are part of the insn chain.


jeff

Re: Report DejaGnu ERROR messages in compare_tests

[Jeff Law]

On 09/20/2016 03:26 AM, Christophe Lyon wrote:

Hello,

We recently faced a problem where a DejaGnu error went un-noticed
(https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01879.html).

To help identify these problems earlier, here is a patch for
compare_tests that will report such cases as:

New tests that FAIL:

arm-sim: (DejaGnu) proc "scan-dump-tree-not fail_test optimized" does not exist.


OK?

OK.
jeff

Re: [BUILDROBOT] vax-netbsdelf / vax-linux: ‘ELIMINABLE_REGS’ was not declared in this scope

[Jeff Law]

On 09/19/2016 04:24 PM, Bernd Edlinger wrote:

On 09/19/16 23:51, Jeff Law wrote:

On 09/17/2016 05:29 PM, Bernd Edlinger wrote:

On 09/17/16 22:29, Jan-Benedict Glaw wrote:

On Fri, 2016-09-09 21:40:38 +, Bernd Edlinger
 wrote:

Hi,

I think it is time to remove support for
INITIAL_FRAME_POINTER_OFFSET, which is no longer
used by any target today.  This removes a bunch of conditional code,
and fixes a few bits
in the documentation.  I'd say that part of the documentation is
quite out of sync, but I just
have to stop somewhere.


Bootstrapped and reg-tested on x86_64-pc-linux.gnu


The vax backend doesn't yet define ELIMINABLE_REGS.



Oh, yes.  I see.  What a hack.

Then we should define it.

But simply returning zero for the fp to sp offset is not ok,
and even if the offset is not used for register eliminations
it should still be correct for rtx_addr_can_trap_p
to know the safe stack frame offset ranges.

I would assume a small performance improvement, when
rtx_addr_can_trap_p has correct data available.

How about this patch, it should at least fix the bootstrap.
Is it OK for trunk?

OK.
jeff


Jeff,  I am sorry.

But I think I got the sign of the elimination offset wrong,
because I peeked at a stack-grows-upward target (hppa).
And the doc/tm.texi is not a big help either.

Funny, I almost called that out as something to double check :-)




With my limited testing this did not make any difference.

Actually I have not touched any vax since I was a student.

And that is already quite a while ago...

I hope you don't mind when I commit it this way:
+#define INITIAL_ELIMINATION_OFFSET(FROM, TO, OFFSET) \
+  ((OFFSET) = get_frame_size ())

That's fine.

jeff

Re: [C++ PATCH] Fix C++ violation in g++.jason/init3.C (PR testsuite/63299)

[Jeff Law]

On 09/19/2016 04:04 PM, Jakub Jelinek wrote:

Hi!

As valgrind reports, this testcase allocates the string with new [], but
deallocates with delete str.

Fixed thusly, regtested on x86_64-linux and i686-linux, ok for trunk?

2016-09-19  Maxim Ostapenko  
Jakub Jelinek  

PR testsuite/63299
* g++.old-deja/g++.jason/init3.C (My_string::~My_string): Use delete[]
instead of delete.

OK.
jeff

Re: Report DejaGnu ERROR messages in dg-extract-results

[Jeff Law]

On 09/20/2016 03:26 AM, Christophe Lyon wrote:

Hello,

We recently faced a problem where a DejaGnu error went un-noticed
(https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01879.html).

To help identify these problems earlier, here is a patch for
dg-extract-results that makes such error obvious in the final summary:

=== gcc Summary ===

# of DejaGnu errors 1
# of expected passes104885
# of unexpected failures43
# of unexpected successes   2
# of expected failures  276
# of unsupported tests  2284


OK?

OK.
jeff

Re: [C++ PATCH] Fix ICE with class fields with invalid types (PR c++/77626)

[Jason Merrill]

OK.

On Mon, Sep 19, 2016 at 5:57 PM, Jakub Jelinek  wrote:
> Hi!
>
> layout_class_type for FIELD_DECLs with error_mark_node skips further
> processing, so such fields don't have DECL_FIELD_OFFSET and
> DECL_FIELD_BIT_OFFSET, thus byte_position ICEs on it.
>
> So, when we walk in constexpr handling all FIELD_DECLs, this patch fixes it
> by skipping over fields with error_mark_node types.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-09-19  Jakub Jelinek  
>
> PR c++/77626
> * constexpr.c (cxx_fold_indirect_ref): Don't call byte_position on
> FIELD_DECLs with error_mark_node type.  Remove useless break; after
> return.
>
> * g++.dg/other/pr77626.C: New test.
>
> --- gcc/cp/constexpr.c.jj   2016-09-16 22:22:00.0 +0200
> +++ gcc/cp/constexpr.c  2016-09-19 13:00:02.542599407 +0200
> @@ -2894,13 +2894,11 @@ cxx_fold_indirect_ref (location_t loc, t
>   tree field = TYPE_FIELDS (optype);
>   for (; field; field = DECL_CHAIN (field))
> if (TREE_CODE (field) == FIELD_DECL
> +   && TREE_TYPE (field) != error_mark_node
> && integer_zerop (byte_position (field))
> && (same_type_ignoring_top_level_qualifiers_p
> (TREE_TYPE (field), type)))
> - {
> -   return fold_build3 (COMPONENT_REF, type, op, field, 
> NULL_TREE);
> -   break;
> - }
> + return fold_build3 (COMPONENT_REF, type, op, field, NULL_TREE);
> }
>  }
>else if (TREE_CODE (sub) == POINTER_PLUS_EXPR
> @@ -2972,14 +2970,12 @@ cxx_fold_indirect_ref (location_t loc, t
>   tree field = TYPE_FIELDS (op00type);
>   for (; field; field = DECL_CHAIN (field))
> if (TREE_CODE (field) == FIELD_DECL
> +   && TREE_TYPE (field) != error_mark_node
> && tree_int_cst_equal (byte_position (field), op01)
> && (same_type_ignoring_top_level_qualifiers_p
> (TREE_TYPE (field), type)))
> - {
> -   return fold_build3 (COMPONENT_REF, type, op00,
> -   field, NULL_TREE);
> -   break;
> - }
> + return fold_build3 (COMPONENT_REF, type, op00,
> + field, NULL_TREE);
> }
> }
>  }
> --- gcc/testsuite/g++.dg/other/pr77626.C.jj 2016-09-19 13:22:57.895418630 
> +0200
> +++ gcc/testsuite/g++.dg/other/pr77626.C2016-09-19 13:22:21.0 
> +0200
> @@ -0,0 +1,13 @@
> +// PR c++/77626
> +// { dg-do compile }
> +
> +struct B;  // { dg-message "forward declaration of" }
> +struct A { struct B b; };  // { dg-error "has incomplete type" }
> +void bar (int);
> +
> +void
> +foo ()
> +{
> +  A a;
> +  bar ((int &) a);
> +}
>
> Jakub

Re: [C++ PATCH] Fix ICE on operator"" template (PR c++/77638)

[Jason Merrill]

OK.

On Mon, Sep 19, 2016 at 5:54 PM, Jakub Jelinek  wrote:
> Hi!
>
> As the testcase shows for C++14 and up, for 1 argument template we don't ICE
> even if the template argument is invalid, because it checks
>   if (TREE_TYPE (parm) != char_type_node
>   || !TEMPLATE_PARM_PARAMETER_PACK (DECL_INITIAL (parm)))
> and if parm is error_mark_node, then it doesn't have char_type_node type.
> But, for 2 argument template, if both the template arguments have
> error_mark_node type the type test succeeds and we ICE, because DEC_INITIAL
> on error_mark_node is not valid.
>
> The following testcase fixes the ICE and results in the same diagnostics
> that used to be emitted for C++11 already before the patch.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-09-19  Jakub Jelinek  
>
> PR c++/77638
> * parser.c (cp_parser_template_declaration_after_parameter): For 2
> argument operator"" template set ok to false for
> parm == error_mark_node.
>
> * g++.dg/cpp0x/udlit-tmpl-arg-neg2.C: New test.
>
> --- gcc/cp/parser.c.jj  2016-09-19 10:33:51.0 +0200
> +++ gcc/cp/parser.c 2016-09-19 11:29:25.724937375 +0200
> @@ -25722,7 +25722,8 @@ cp_parser_template_declaration_after_par
>   tree type = INNERMOST_TEMPLATE_PARMS (parm_type);
>   tree parm_list = TREE_VEC_ELT (parameter_list, 1);
>   tree parm = INNERMOST_TEMPLATE_PARMS (parm_list);
> - if (TREE_TYPE (parm) != TREE_TYPE (type)
> + if (parm == error_mark_node
> + || TREE_TYPE (parm) != TREE_TYPE (type)
>   || !TEMPLATE_PARM_PARAMETER_PACK (DECL_INITIAL (parm)))
> ok = false;
> }
> --- gcc/testsuite/g++.dg/cpp0x/udlit-tmpl-arg-neg2.C.jj 2016-09-19 
> 11:34:58.680674929 +0200
> +++ gcc/testsuite/g++.dg/cpp0x/udlit-tmpl-arg-neg2.C2016-09-19 
> 11:33:54.0 +0200
> @@ -0,0 +1,7 @@
> +// PR c++/77638
> +// { dg-do compile { target c++11 } }
> +
> +template// { dg-error "'T' has not been declared" }
> +int operator"" _foo ();// { dg-error "has invalid parameter 
> list" }
> +template   // { dg-error "'T' has not been declared" }
> +int operator"" _bar ();// { dg-error "has invalid parameter 
> list" }
>
> Jakub

Re: [C++ PATCH] Fix parser ICE with [[...]] and [[unused,...]] (PR c++/77637)

[Jason Merrill]

OK.

On Mon, Sep 19, 2016 at 5:50 PM, Jakub Jelinek  wrote:
> Hi!
>
> As the testcase shows, while the grammar has
> attribute-list:
>   attribute[opt]
>   attribute-list , attribute[opt]
>   attribute ...
>   attribute-list , attribute ...
> we are actually parsing it as if the last 2 lines were
>   attribute[opt] ...
>   attribute-list , attribute[opt] ...
> and ICEing when there is no attribute.
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk/6.3?
>
> 2016-09-19  Jakub Jelinek  
>
> PR c++/77637
> * parser.c (cp_parser_std_attribute_list): Reject ... without
> preceding attribute.
>
> * g++.dg/cpp0x/gen-attrs-62.C: New test.
>
> --- gcc/cp/parser.c.jj  2016-09-16 22:19:39.0 +0200
> +++ gcc/cp/parser.c 2016-09-19 10:33:51.481904739 +0200
> @@ -24220,8 +24220,12 @@ cp_parser_std_attribute_list (cp_parser
>if (token->type == CPP_ELLIPSIS)
> {
>   cp_lexer_consume_token (parser->lexer);
> - TREE_VALUE (attribute)
> -   = make_pack_expansion (TREE_VALUE (attribute));
> + if (attribute == NULL_TREE)
> +   error_at (token->location,
> + "expected attribute before %<...%>");
> + else
> +   TREE_VALUE (attribute)
> + = make_pack_expansion (TREE_VALUE (attribute));
>   token = cp_lexer_peek_token (parser->lexer);
> }
>if (token->type != CPP_COMMA)
> --- gcc/testsuite/g++.dg/cpp0x/gen-attrs-62.C.jj2016-09-19 
> 10:37:59.414772726 +0200
> +++ gcc/testsuite/g++.dg/cpp0x/gen-attrs-62.C   2016-09-19 10:37:28.0 
> +0200
> @@ -0,0 +1,5 @@
> +// PR c++/77637
> +// { dg-do compile { target c++11 } }
> +
> +int [[...]] a; // { dg-error "expected attribute before '...'" }
> +int [[,,...]] b;   // { dg-error "expected attribute before '...'" }
>
> Jakub

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Jeff Law]

On 09/20/2016 08:38 AM, Bernd Edlinger wrote:

On 09/20/16 16:20, Kyrill Tkachov wrote:


On 20/09/16 15:11, Bernd Edlinger wrote:

On 09/20/16 13:29, Kyrill Tkachov wrote:

arm bootstrap is now failing:
$SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in
boolean context [-Werror=int-in-bool-context]
  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
 ~^~
$SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro
'TARGET_ARM_FP'
 if (TARGET_ARM_FP)


The full definition of TARGET_ARM_FP is:
#define TARGET_ARM_FP\
(!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
  : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
: 0)

We want it set to 0 when there's no FP but when FP is available we set
it to a bitmask
to suggest the level that is available. That seems like a legitimate use
to me.


Ok, I see, sorry for that.

I think I will have to suppress the warning if the conditional is in
a macro somehow.

Can you work around that for a few days?


I changed to:
  if (TARGET_ARM_FP != 0)
in my tree to allow the build to proceed but encountered another
bootstrap failure:
In file included from ./tm.h:38:0,
  from $SRC/gcc/backend.h:28,
  from $SRC/gcc/regrename.c:23:
$SRC/gcc/regrename.c: In function 'void rename_chains()':
$SRC/gcc/config/arm/arm.h:915:4: error: ?: using integer constants in
boolean context [-Werror=int-in-bool-context]
(TARGET_ARM \
~
 ? ARM_HARD_FRAME_POINTER_REGNUM  \
 ^~
 : THUMB_HARD_FRAME_POINTER_REGNUM)
 ~~
$SRC/gcc/regrename.c:484:8: note: in expansion of macro
'HARD_FRAME_POINTER_REGNUM'
 || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
 ^
That looks like a legitimate bug in regrename.c ?
The full condition is:
if (fixed_regs[reg] || global_regs[reg]
|| (!HARD_FRAME_POINTER_IS_FRAME_POINTER &&
frame_pointer_needed
&& reg == HARD_FRAME_POINTER_REGNUM)
|| (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
&& reg == FRAME_POINTER_REGNUM))

The condition in the second || looks bogus (what use testing if a
register is 'non-zero').

So this looks like a useful catch by the warning, though I'm not sure at
first glance how
to fix it.
Kyrill



I assume HARD_FRAME_POINTER_REGNUM is never zero.
It could be zero.  It's just a hard register number.  No target has the 
property that its hard frame pointer register is 0 though :-)




That looks like it should be

  || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)

I'm not even entirely sure why this is two conditions.

if (fixed_regs[regs] || global_regs[reg]
|| (frame_pointer_needed
&& (reg == HARD_FRAME_POINTER_REGNUM
|| reg == FRAME_POINTER_REGNUM

?


Bernd Schmidt ought to be able to guide us.

jeff

Re: Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)

[Jeff Law]

On 09/20/2016 08:34 AM, Bernd Schmidt wrote:

On 09/20/2016 04:32 PM, David Malcolm wrote:


To summarize so far: you want every pseudo to have its regno dumped
with a 'p' prefix, and renumber them on dump (and then on load).
OK.


Renumbering is not helpful because it interferes with the view you have
in the debugger. So, IMO just a prefix, and maybe
Yea, I guess it does since we want the numbers in the dump to be the 
same that we used to access the arrays.  So prefixing in the dump with 
adjustment of the number in the reader.





  (reg/f:DI v1 virtual-stack-vars)


this.
Doesn't the same apply to the number of virtual stack regs?  Those are 
in the same array as pseudos.  So ISTM we prefix in the dump, but do 
adjustment of the number in the reader?


jeff

[PATCH GCC][v2]Simplify alias check code generation in vectorizer

[Bin Cheng]

Hi,
I originally posted a patch improving code generation for alias check in 
vectorizer at https://gcc.gnu.org/ml/gcc-patches/2016-06/msg00929.html.  Here 
it's the 2nd version (better) patch.  It detects data reference pair in which 
the two references are only different to each other wrto index.  In this case 
the patch generates intersect range checks based on index of address of 
reference, rather than the address of reference.  Take example from patch's 
comment, given below two data references:

   DR_A   DR_B
  data-ref arr[i] arr[j]
  base_object  arrarr
  index{i_0, +, 1}_loop   {j_0, +, 1}_loop

The addresses and their index can be depicted as:

|<- ADDR_A->|  |<- ADDR_B->|
 --->
|   |   |   |   |  |   |   |   |   |
 --->
i_0 ... i_0+4  j_0 ... j_0+4

We can create expression based on index rather than address:  (i_0 + 4 < j_0 || 
j_0 + 4 < i_0).

Also take example from spec2k/200.sixtrack/DACOP, gcc needs to generate alias 
check for three pairs:
//pair 1
dr_a:
(Data Ref:
  bb: 8
  stmt: _92 = da.cc[_27];
  ref: da.cc[_27];
)
dr_b:
(Data Ref:
  bb: 8
  stmt: da.cc[_93] = _92;
  ref: da.cc[_93];
)
//pair 2
dr_a:
(Data Ref:
  bb: 8
  stmt: pretmp_29 = da.i2[_27];
  ref: da.i2[_27];
)
dr_b:
(Data Ref:
  bb: 8
  stmt: da.i2[_93] = pretmp_29;
  ref: da.i2[_93];
)
//pair 3
dr_a:
(Data Ref:
  bb: 8
  stmt: pretmp_28 = da.i1[_27];
  ref: da.i1[_27];
)
dr_b:
(Data Ref:
  bb: 8
  stmt: da.i1[_93] = pretmp_28;
  ref: da.i1[_93];
)

With this patch, code can be improved to:

  :
  _37 = (unsigned int) _6;
  _106 = (unsigned int) _52;
  _107 = _37 - _106;
  _108 = _107 > 7;
  _109 = (integer(kind=8)) _2;
  _110 = _109 + 3;
  _111 = (integer(kind=8)) _52;
  _112 = _111 + -1;
  _113 = _110 < _112;
  _114 = (integer(kind=8)) _52;
  _115 = _114 + 2;
  _116 = (integer(kind=8)) _2;
  _117 = _115 < _116;
  _118 = _113 | _117;
  _119 = _108 & _118;
  _120 = (integer(kind=8)) _2;
  _121 = _120 + 3;
  _122 = (integer(kind=8)) _52;
  _123 = _122 + -1;
  _124 = _121 < _123;
  _125 = (integer(kind=8)) _52;
  _126 = _125 + 2;
  _127 = (integer(kind=8)) _2;
  _128 = _126 < _127;
  _129 = _124 | _128;
  _130 = _119 & _129;
  _131 = (integer(kind=8)) _2;
  _132 = _131 + 3;
  _133 = (integer(kind=8)) _52;
  _134 = _133 + -1;
  _135 = _132 < _134;
  _136 = (integer(kind=8)) _52;
  _137 = _136 + 2;
  _138 = (integer(kind=8)) _2;
  _139 = _137 < _138;
  _140 = _135 | _139;
  _141 = _130 & _140;
  if (_141 != 0)
goto ;
  else
goto ;

Note this patch doesn't do local CSE, and common sub-expressions will be 
optimized by later passes.  I think this is OK because the redundancy is far 
away from loops thus won't have bad effect (there is an opposite example/PR).
Bootstrap and test on x86_64 and AArch64, is it OK?

Thanks,
bin

2016-09-19  Bin Cheng  

* tree-vect-loop-manip.c (create_intersect_range_checks_index): New.
(create_intersect_range_checks): New.
(vect_create_cond_for_alias_checks): Call above function.diff --git a/gcc/tree-vect-loop-manip.c b/gcc/tree-vect-loop-manip.c
index 01d6bb1..c7d130e 100644
--- a/gcc/tree-vect-loop-manip.c
+++ b/gcc/tree-vect-loop-manip.c
@@ -2263,6 +2263,179 @@ vect_create_cond_for_align_checks (loop_vec_info 
loop_vinfo,
 *cond_expr = part_cond_expr;
 }
 
+/* Given two data references and segment lengths described by DR_A and DR_B,
+   create expression checking if the two addresses ranges intersect with
+   each other based on index of the two addresses.  This can only be done
+   if DR_A and DR_B referring to the same (array) object and the index is
+   the only difference.  For example:
+
+   DR_A   DR_B
+  data-ref arr[i] arr[j]
+  base_object  arrarr
+  index{i_0, +, 1}_loop   {j_0, +, 1}_loop
+
+   The addresses and their index are like:
+
+|<- ADDR_A->|  |<- ADDR_B->|
+ --->
+|   |   |   |   |  |   |   |   |   |
+ --->
+i_0 ... i_0+4  j_0 ... j_0+4
+
+   We can create expression based on index rather than address:
+ (i_0 + 4 < j_0 || j_0 + 4 < i_0).  */
+
+static bool
+create_intersect_range_checks_index (loop_vec_info loop_vinfo, tree *cond_expr,
+const dr_with_seg_len& dr_a,
+const dr_with_seg_len& dr_b)
+{
+  if (integer_zerop (DR_STEP (dr_a.dr))
+  || integer_zerop (DR_STEP (dr_b.dr))
+  || DR_NUM_DIMENSIONS (dr_a.dr) != DR_NUM_DI

Re: [PATCH] Tree-level fix for PR 69526

[Jeff Law]

On 09/20/2016 06:31 AM, Robin Dapp wrote:

Hi,


I meant to do sth like

Index: tree-ssa-propagate.c
===
--- tree-ssa-propagate.c(revision 240133)
+++ tree-ssa-propagate.c(working copy)
@@ -1105,10 +1105,10 @@ substitute_and_fold_dom_walker::before_d
   /* Replace real uses in the statement.  */
   did_replace |= replace_uses_in (stmt, get_value_fn);

-  /* If we made a replacement, fold the statement.  */
-  if (did_replace)
+  /* Fold the statement.  */
+  if (fold_stmt (&i, follow_single_use_edges))
{
- fold_stmt (&i, follow_single_use_edges);
+ did_replace = true;
  stmt = gsi_stmt (i);
}

this would need compile-time cost evaluation (and avoid the tree-vrp.c
folding part
of your patch).


Using this causes the simplifications to be performed in ccp1 instead of
fwprop1. I noticed two tests failing that rely on propagation being
performed in fwprop. Should these be adapted or rather the patch be changed?
ISTM this is an indication that something changed the IL without folding 
prior to ccp & forwprop.  That's not in and of itself bad, but may be 
worth investigating to see if whatever prior pass that made this change 
ought to be adjusted.


That investigation would likely guide you with what to do with the 
testcase.  If you find that the earlier pass should have folded, then 
you fix it and change the testcase to verify the earlier pass folded 
properly.  Else you change the testcase to verify ccp folds the statement.


I'm going to let Richi own the review on this.  Just thought I'd chime 
in on that one topic.

jeff

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Schmidt]

On 09/20/2016 05:18 PM, Jeff Law wrote:

I assume HARD_FRAME_POINTER_REGNUM is never zero.

It could be zero.  It's just a hard register number.  No target has the
property that its hard frame pointer register is 0 though :-)


git blame to the rescue. The current state comes from one of tbsaunde's 
cleanup patches:


> diff --git a/gcc/regrename.c b/gcc/regrename.c
index 174d3b5..e5248a5 100644
--- a/gcc/regrename.c
+++ b/gcc/regrename.c
@@ -442,12 +442,10 @@ rename_chains (void)
continue;

   if (fixed_regs[reg] || global_regs[reg]
-#if !HARD_FRAME_POINTER_IS_FRAME_POINTER
- || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)
-#else
- || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)
-#endif
- )
+ || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
+ && reg == HARD_FRAME_POINTER_REGNUM)
+ || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
+ && reg == FRAME_POINTER_REGNUM))
continue;

   COPY_HARD_REG_SET (this_unavailable, unavailable);

Looks like it never got reviewed and was committed as preapproved.

The #ifdef we had before looks odd too. Maybe this should just read

 if (fixed_regs[reg] || global_regs[reg]
 || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM))


Bernd

Re: Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)

[Bernd Schmidt]

On 09/20/2016 05:20 PM, Jeff Law wrote:

On 09/20/2016 08:34 AM, Bernd Schmidt wrote:



  (reg/f:DI v1 virtual-stack-vars)


this.

Doesn't the same apply to the number of virtual stack regs?  Those are
in the same array as pseudos.  So ISTM we prefix in the dump, but do
adjustment of the number in the reader?


I meant the virtual-stack-vars name. But maybe we're already doing that, 
I can't remember.



Bernd

Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

Time for another latest & greatest version of -Wimplicit-fallthrough.
Hope this time it's near its end.

Changes from last time:

* I addressed Jakub's comment, for details see
  
* in the C FE:
  case 0:
__attribute)) ((fallthrough));
  case 1:
...
  now works
* we've discussed cases like
  [[fallthrough]]; lab1: case 1:;
  and
  [[fallthrough]]; lab2:; case 2:;

  This was discussed on the C++ committee reflector and it was concluded that
  the former should work and the latter should not.  So I implemented that.
  But there's no way to tell the difference between "lab1: case 1:;" and
  "lab2:; case 2:;" so the null statements are ignored.  Another problem is
  what to do with "[[fallthrough]]; lab1: (void) 0; case 2:".  Currently it's
  not rejected because we optimize "(void) 0;" out in gimplify_expr.  And I
  couldn't figure out how to circumvent this (in gimplify_expr you can't see
  what the next statement is).  Though this seems more like a pedantic issue
  and probably not a real-world issue.  (E.g.
  "[[fallthrough]]; lab1: if (0) { ... } case 2:"
  should be rejected.)
 
Jason, would you mind looking at the C++ FE parts again?

Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk?

2016-09-20  Marek Polacek  
Jakub Jelinek  

PR c/7652
gcc/
* common.opt (Wimplicit-fallthrough): New option.
* doc/extend.texi: Document statement attributes and the fallthrough
attribute.
* doc/invoke.texi: Document -Wimplicit-fallthrough.
* gimple.h (gimple_call_internal_p): New function.
* gimplify.c (struct gimplify_ctx): Add in_switch_expr.
(struct label_entry): New struct.
(find_label_entry): New function.
(case_label_p): New function.
(collect_fallthrough_labels): New function.
(last_stmt_in_scope): New function.
(should_warn_for_implicit_fallthrough): New function.
(warn_implicit_fallthrough_r): New function.
(maybe_warn_implicit_fallthrough): New function.
(expand_FALLTHROUGH_r): New function.
(expand_FALLTHROUGH): New function.
(gimplify_switch_expr): Call maybe_warn_implicit_fallthrough and
expand_FALLTHROUGH for the innermost GIMPLE_SWITCH.
(gimplify_label_expr): New function.
(gimplify_case_label_expr): Set location.
(gimplify_expr): Call gimplify_label_expr.
* internal-fn.c (expand_FALLTHROUGH): New function.
* internal-fn.def (FALLTHROUGH): New internal function.
* langhooks.c (lang_GNU_OBJC): New function.
* langhooks.h (lang_GNU_OBJC): Declare.
* system.h (gcc_fallthrough): Define.
* tree-core.h: Add FALLTHROUGH_LABEL_P comment.
* tree.h (FALLTHROUGH_LABEL_P): Define.
gcc/c-family/
* c-common.c (c_common_attribute_table): Add fallthrough attribute.
(handle_fallthrough_attribute): New function.
(maybe_attribute_fallthrough_p): New function.
* c-common.h (maybe_attribute_fallthrough_p): Declare.
gcc/c/
* c-parser.c (struct c_token): Add flags field.
(c_lex_one_token): Pass it to c_lex_with_flags.
(c_parser_declaration_or_fndef): Turn __attribute__((fallthrough));
into IFN_FALLTHROUGH.
(c_parser_label): Set FALLTHROUGH_LABEL_P on labels.  Handle
attribute fallthrough after a case label or default label.
(c_parser_statement_after_labels): Handle RID_ATTRIBUTE.
gcc/cp/
* constexpr.c (cxx_eval_internal_function): Handle IFN_FALLTHROUGH.
(potential_constant_expression_1): Likewise.
* constraint.cc (function_concept_check_p): Check fn for null.
* parser.c (cp_parser_primary_expression): Handle RID_ATTRIBUTE.
(cp_parser_statement): Handle fallthrough attribute.
(cp_parser_label_for_labeled_statement): Set FALLTHROUGH_LABEL_P on
labels.
(cp_parser_std_attribute): Handle fallthrough attribute.
(cp_parser_check_std_attribute): Detect duplicated fallthrough
attribute.
* pt.c (tsubst_copy_and_build): Handle internal functions.
(instantiation_dependent_scope_ref_p): Return if the expression is
null.
gcc/testsuite/
* c-c++-common/Wimplicit-fallthrough-1.c: New test.
* c-c++-common/Wimplicit-fallthrough-10.c: New test.
* c-c++-common/Wimplicit-fallthrough-11.c: New test.
* c-c++-common/Wimplicit-fallthrough-12.c: New test.
* c-c++-common/Wimplicit-fallthrough-13.c: New test.
* c-c++-common/Wimplicit-fallthrough-14.c: New test.
* c-c++-common/Wimplicit-fallthrough-15.c: New test.
* c-c++-common/Wimplicit-fallthrough-16.c: New test.
* c-c++-common/Wimplicit-fallthrough-17.c: New test.
* c-c++-common/Wimplicit-fallthrough-18.c: New test.
* c-c++-common/Wimplicit-fallthrough-19.c: New test.
* c-c++-common/Wimplicit-fal

[PATCH] [AArch64] Add missing attributes to arm_neon.h

[Tamar Christina]

Hi All,

This patch fixes the regression failures introduced by r240256.
It adds some missing attributes to the functions:

 * vst2_s64
 * vst2_u64
 * vst2_f64
 * vst2_s8
 * vst3_s64
 * vst3_u64
 * vst3_f64
 * vst3_s8
 * vst4_s64
 * vst4_u64
 * vst4_f64
 * vst4_s8

These were always missing but the change of marking the
functions extern instead of static exposed them.

The patch has been pre-approved to be checked in but I don't
have commit rights.
https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01293.html

Can someone check it in for me?

Fixes failures:

gcc.target/aarch64/vect_int32x2x4_1.c
gcc.target/aarch64/vdup_lane_2.c
gcc.target/aarch64/vect_combine_zeroes_1.c

gcc/
2016-09-20  Tamar Christina  

* config/aarch64/arm_neon.h
(vst2_s64, vst2_u64, vst2_f64, vst2_s8): Add missing attributes.
(vst3_s64, vst3_u64, vst3_f64, vst3_s8): Likewise.
(vst4_s64, vst4_u64, vst4_f64, vst4_s8): Likewise.

Thanks,
Tamardiff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h
index b4db87b0bc60d22306da810381b62920646aac9b..c463e3b698a47b9b5c5a04e0fb7fff1f71817af1 100644
--- a/gcc/config/aarch64/arm_neon.h
+++ b/gcc/config/aarch64/arm_neon.h
@@ -26102,6 +26102,7 @@ vst1q_lane_u64 (uint64_t *__a, uint64x2_t __b, const int __lane)
 /* vstn */
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst2_s64 (int64_t * __a, int64x1x2_t val)
 {
   __builtin_aarch64_simd_oi __o;
@@ -26114,6 +26115,7 @@ vst2_s64 (int64_t * __a, int64x1x2_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst2_u64 (uint64_t * __a, uint64x1x2_t val)
 {
   __builtin_aarch64_simd_oi __o;
@@ -26126,6 +26128,7 @@ vst2_u64 (uint64_t * __a, uint64x1x2_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst2_f64 (float64_t * __a, float64x1x2_t val)
 {
   __builtin_aarch64_simd_oi __o;
@@ -26138,6 +26141,7 @@ vst2_f64 (float64_t * __a, float64x1x2_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst2_s8 (int8_t * __a, int8x8x2_t val)
 {
   __builtin_aarch64_simd_oi __o;
@@ -26397,6 +26401,7 @@ vst2q_f64 (float64_t * __a, float64x2x2_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst3_s64 (int64_t * __a, int64x1x3_t val)
 {
   __builtin_aarch64_simd_ci __o;
@@ -26411,6 +26416,7 @@ vst3_s64 (int64_t * __a, int64x1x3_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst3_u64 (uint64_t * __a, uint64x1x3_t val)
 {
   __builtin_aarch64_simd_ci __o;
@@ -26425,6 +26431,7 @@ vst3_u64 (uint64_t * __a, uint64x1x3_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst3_f64 (float64_t * __a, float64x1x3_t val)
 {
   __builtin_aarch64_simd_ci __o;
@@ -26439,6 +26446,7 @@ vst3_f64 (float64_t * __a, float64x1x3_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst3_s8 (int8_t * __a, int8x8x3_t val)
 {
   __builtin_aarch64_simd_ci __o;
@@ -26731,6 +26739,7 @@ vst3q_f64 (float64_t * __a, float64x2x3_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst4_s64 (int64_t * __a, int64x1x4_t val)
 {
   __builtin_aarch64_simd_xi __o;
@@ -26747,6 +26756,7 @@ vst4_s64 (int64_t * __a, int64x1x4_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst4_u64 (uint64_t * __a, uint64x1x4_t val)
 {
   __builtin_aarch64_simd_xi __o;
@@ -26763,6 +26773,7 @@ vst4_u64 (uint64_t * __a, uint64x1x4_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst4_f64 (float64_t * __a, float64x1x4_t val)
 {
   __builtin_aarch64_simd_xi __o;
@@ -26779,6 +26790,7 @@ vst4_f64 (float64_t * __a, float64x1x4_t val)
 }
 
 __extension__ extern __inline void
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
 vst4_s8 (int8_t * __a, int8x8x4_t val)
 {
   __builtin_aarch64_simd_xi __o;

Re: [PATCH] Fix PR tree-optimization/77550

[Bernd Edlinger]

On 09/20/16 09:41, Richard Biener wrote:
> On Mon, 19 Sep 2016, Bernd Edlinger wrote:
>
>> On 09/19/16 11:25, Richard Biener wrote:
>>> On Sun, 18 Sep 2016, Bernd Edlinger wrote:
>>>
 Hi,

 this PR shows that in vectorizable_store and vectorizable_load
 as well, the vector access always uses the first dr as the alias
 type for the whole access.  But that is not right, if they are
 different types, like in this example.

 So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr))
 by an alias type that is correct for all references in the whole
 access group.  With this patch we fall back to ptr_type_node, which
 can alias anything, if the group consists of different alias sets.


 Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
 Is it OK for trunk and gcc-6-branch?
>>>
>>> +/* Function get_group_alias_ptr_type.
>>> +
>>> +   Return the alias type for the group starting at FIRST_STMT
>>> +   containing GROUP_SIZE elements.  */
>>> +
>>> +static tree
>>> +get_group_alias_ptr_type (gimple *first_stmt, int group_size)
>>> +{
>>> +  struct data_reference *first_dr, *next_dr;
>>> +  gimple *next_stmt;
>>> +  int i;
>>> +
>>> +  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
>>> +  next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt));
>>> +  for (i = 1; i < group_size && next_stmt; i++)
>>> +{
>>>
>>>
>>> there is no need to pass in group_size, it's enough to walk
>>> GROUP_NEXT_ELEMENT until it becomes NULL.
>>>
>>> Ok with removing the redundant arg.
>>>
>>> Thanks,
>>> Richard.
>>>
>>
>> Hmmm, I'm afraid this needs one more iteration.
>>
>>
>> I tried first to check if there are no stmts after the group_size
>> and noticed there are cases when group_size is 0,
>> for instance in gcc.dg/torture/pr36244.c.
>>
>> I think there is a bug in vectorizable_load, here:
>>
>>if (grouped_load)
>>  {
>>first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
>> /* For SLP vectorization we directly vectorize a subchain
>>without permutation.  */
>> if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
>>   first_stmt = ;
>>
>> group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt));
>>
>> = 0, and even worse:
>>
>> group_gap_adj = vf * group_size - nunits * vec_num;
>>
>> = -4 !
>>
>> apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT,
>
> Yes.  I'm not sure group_size or group_gap_adj are used in the
> slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists () case but moving
> the computation up before we re-set first_stmt is probably a good idea.
>
>> while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0]
>>
>> moving the GROUP_SIZE up before first_stmt is overwritten
>> results in no different code
>
> See above - it's eventually unused.  The load/store vectorization code
> is quite twisted ;)
>

Agreed.

Here is the new version of the patch:

Moved the goups_size up, and everything works fine.

Removed the parameter from get_group_alias_ptr_type.

I think in the case, where first_stmt is not set to
GROUP_FIRST_ELEMENT (stmt_info) but directly to stmt,
it is likely somewhere in a list, so it is not necessary
to walk the GROUP_NEXT_ELEMENT, so I would like to call
reference_alias_ptr_type directly in that case.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk and gcc-6 branch?


Thanks
Bernd.
gcc:
2016-09-18  Bernd Edlinger  

PR tree-optimization/77550
* tree-vect-stmts.c (create_array_ref): Change parameters.
(get_group_alias_ptr_type): New function.
(vectorizable_store, vectorizable_load): Use get_group_alias_ptr_type.

testsuite:
2016-09-18  Bernd Edlinger  

PR tree-optimization/77550
* g++.dg/pr77550.C: New test.
Index: gcc/tree-vect-stmts.c
===
--- gcc/tree-vect-stmts.c	(revision 240251)
+++ gcc/tree-vect-stmts.c	(working copy)
@@ -170,11 +170,10 @@ write_vector_array (gimple *stmt, gimple_stmt_iter
(and its group).  */
 
 static tree
-create_array_ref (tree type, tree ptr, struct data_reference *first_dr)
+create_array_ref (tree type, tree ptr, tree alias_ptr_type)
 {
-  tree mem_ref, alias_ptr_type;
+  tree mem_ref;
 
-  alias_ptr_type = reference_alias_ptr_type (DR_REF (first_dr));
   mem_ref = build2 (MEM_REF, type, ptr, build_int_cst (alias_ptr_type, 0));
   /* Arrays have the same alignment as their type.  */
   set_ptr_info_alignment (get_ptr_info (ptr), TYPE_ALIGN_UNIT (type), 0);
@@ -5432,6 +5431,35 @@ ensure_base_align (stmt_vec_info stmt_info, struct
 }
 
 
+/* Function get_group_alias_ptr_type.
+
+   Return the alias type for the group starting at FIRST_STMT.  */
+
+static tree
+get_group_alias_ptr_type (gimple *first_stmt)
+{
+  struct data_reference *first_dr, *next_dr;
+  gimple *next_stmt;
+
+  first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
+  next_stmt

Re: [PATCH] [AArch64] Add missing attributes to arm_neon.h

[James Greenhalgh]

On Tue, Sep 20, 2016 at 04:37:42PM +0100, Tamar Christina wrote:
> Hi All,
> 
> This patch fixes the regression failures introduced by r240256.
> It adds some missing attributes to the functions:
> 
>  * vst2_s64
>  * vst2_u64
>  * vst2_f64
>  * vst2_s8
>  * vst3_s64
>  * vst3_u64
>  * vst3_f64
>  * vst3_s8
>  * vst4_s64
>  * vst4_u64
>  * vst4_f64
>  * vst4_s8
> 
> These were always missing but the change of marking the
> functions extern instead of static exposed them.
> 
> The patch has been pre-approved to be checked in but I don't
> have commit rights.
> https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01293.html
> 
> Can someone check it in for me?

Done. r240271.

> gcc/
> 2016-09-20  Tamar Christina  
> 
>   * config/aarch64/arm_neon.h
>   (vst2_s64, vst2_u64, vst2_f64, vst2_s8): Add missing attributes.
>   (vst3_s64, vst3_u64, vst3_f64, vst3_s8): Likewise.
>   (vst4_s64, vst4_u64, vst4_f64, vst4_s8): Likewise.
> 

Thanks for the prompt fix,
James

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Kyrill Tkachov]

On 20/09/16 16:30, Bernd Schmidt wrote:

On 09/20/2016 05:18 PM, Jeff Law wrote:

I assume HARD_FRAME_POINTER_REGNUM is never zero.

It could be zero.  It's just a hard register number.  No target has the
property that its hard frame pointer register is 0 though :-)


git blame to the rescue. The current state comes from one of tbsaunde's cleanup 
patches:

> diff --git a/gcc/regrename.c b/gcc/regrename.c
index 174d3b5..e5248a5 100644
--- a/gcc/regrename.c
+++ b/gcc/regrename.c
@@ -442,12 +442,10 @@ rename_chains (void)
continue;

   if (fixed_regs[reg] || global_regs[reg]
-#if !HARD_FRAME_POINTER_IS_FRAME_POINTER
- || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)
-#else
- || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)
-#endif
- )
+ || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
+ && reg == HARD_FRAME_POINTER_REGNUM)
+ || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
+ && reg == FRAME_POINTER_REGNUM))
continue;

   COPY_HARD_REG_SET (this_unavailable, unavailable);

Looks like it never got reviewed and was committed as preapproved.

The #ifdef we had before looks odd too. Maybe this should just read

 if (fixed_regs[reg] || global_regs[reg]
 || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM))



I'll try bootstrapping and testing this change on arm-none-linux-gnueabihf.

Thanks,
Kyrill



Bernd

[C++ PATCH] Fix constexpr switch handling (PR c++/77467, take 2)

[Jakub Jelinek]

On Mon, Sep 19, 2016 at 02:34:17PM -0400, Jason Merrill wrote:
> > For STATEMENT_LIST that is called by cxx_eval_constant_expression,
> > for BIND_EXPR if we are lucky enough that BIND_EXPR_BODY is a STATEMENT_LIST
> > too (otherwise I assume even my patch doesn't fix it, it would need to
> > verify that).  If body is some other statement, then it really should be
> > skipped, but it isn't, because cxx_eval_constant_expression ignores it.
> 
> > I wonder if we e.g. cxx_eval_constant_expression couldn't early in the
> > function for if (*jump_target) return immediately unless code is something
> > like STATEMENT_LIST or BIND_EXPR with BIND_EXPR_BODY being STATEMENT_LIST,
> > or perhaps in the future other construct containing other stmts.
> 
> We might assert !jump_target before the call to
> cxx_eval_store_expression, to make sure we don't accidentally evaluate
> one when we're trying to jump.

I've done that.

> > I've beeing thinking about TRY block, but at least on the testcases I've
> > tried it has been rejected in constexpr functions, I think one can't branch
> > into statement expressions, so that should be fine, OpenMP/OpenACC
> > constructs are hopefully also rejected in constexpr, what else?
> 
> LOOP_EXPR, COND_EXPR?

Looking at those and adding further testcases revealed lots of other issues,
which the following patch attempts to deal with.

One is that handling the CASE_LABEL_EXPR or LABEL_EXPR only inside of
cxx_eval_statement_list doesn't really work well, because as the testcase
shows, for labeled null statements inside say if then block or else block
there is no STATEMENT_LIST wrapping it up.

Another is that the default: label can be arbitrarily nested (in inner
STATEMENT_LIST, or LOOP_EXPR body, or COND_EXPR then or else block, so
trying to handle it in cxx_eval_statement_list also works only in the
simplest switch case, we really need to process (with initial skipping mode)
the whole SWITCH_EXPR body and if we get through everything and (as
optimization) determine there has been a default: label, restart processing
it again with a flag to satisfy switches (jump_target) with the default:
label of the current switch.

Another thing is that the skipping of statements in cxx_eval_statement_list
didn't really work correctly for COND_EXPR/LOOP_EXPR, so the patch pretty
much removes all the skip/label/etc. handling from cxx_eval_statement_list.

And, lastly the handling of LOOP_EXPR and COND_EXPR when initially skipping
had to be changed.  For COND_EXPR, we don't want to evaluate the condition
in that case (one can't goto into the condition expression), and need to
process then block with skipping and, if it isn't skipped at its end, not
process else block, otherwise process it.  And for LOOP_EXPR, if still
skipping we'd need not to iterate again on the body, as the whole loop has
been bypassed in that case.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-09-20  Jakub Jelinek  

PR c++/77467
* constexpr.c (enum constexpr_switch_state): New.
(struct constexpr_ctx): Add css_state field.
(label_matches): Add CTX and STMT arguments, remove I and
DEFAULT_LABEL.  For CASE_LABEL_EXPR assert ctx->css_state != NULL,
handle default labels according to css_state.
(cxx_eval_statement_list): Remove statement skipping, label_matches
and default_label handling code.
(cxx_eval_loop_expr): Exit after first iteration even if
switches (jump_target).
(cxx_eval_switch_expr): Set up css_state field in ctx, if default
label has been seen in the body, but no cases matched, evaluate
the body second time.
(cxx_eval_constant_expression): Handle stmt skipping and label_matches
here.  Handle PREDICT_EXPR.  For MODIFY_EXPR or INIT_EXPR, assert
statement is not skipped.  For COND_EXPR during skipping, don't
evaluate condition, just the then block and if still skipping at the
end also the else block.
(cxx_eval_outermost_constant_expr): Adjust constexpr_ctx initializer.
(is_sub_constant_expr): Likewise.

* g++.dg/cpp1y/constexpr-77467.C: New test.

--- gcc/cp/constexpr.c.jj   2016-09-19 16:35:32.835574406 +0200
+++ gcc/cp/constexpr.c  2016-09-20 12:51:48.270305879 +0200
@@ -900,6 +900,18 @@ struct constexpr_call_hasher : ggc_ptr_h
   static bool equal (constexpr_call *, constexpr_call *);
 };
 
+enum constexpr_switch_state {
+  /* Used when processing a switch for the first time by cxx_eval_switch_expr
+ and default: label for that switch has not been seen yet.  */
+  css_default_not_seen,
+  /* Used when processing a switch for the first time by cxx_eval_switch_expr
+ and default: label for that switch has been seen already.  */
+  css_default_seen,
+  /* Used when processing a switch for the second time by
+ cxx_eval_switch_expr, where default: label should match.  */
+  css_default_processing
+};
+
 /* The constexp

Re: Implement -Wimplicit-fallthrough (version 9)

[Jason Merrill]

On 09/20/2016 11:33 AM, Marek Polacek wrote:

@@ -5135,6 +5135,30 @@ cp_parser_primary_expression (cp_parser *parser,
case RID_AT_SELECTOR:
  return cp_parser_objc_expression (parser);

+   case RID_ATTRIBUTE:


Attribute handling doesn't belong in this function; we don't want to 
allow attributes anywhere we see a primary-expression.  This should be 
either in cp_parser_expression_statement or cp_parser_statement.


Jason

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/20/16 18:12, Kyrill Tkachov wrote:
>
> On 20/09/16 16:30, Bernd Schmidt wrote:
>> On 09/20/2016 05:18 PM, Jeff Law wrote:
 I assume HARD_FRAME_POINTER_REGNUM is never zero.
>>> It could be zero.  It's just a hard register number.  No target has the
>>> property that its hard frame pointer register is 0 though :-)
>>
>> git blame to the rescue. The current state comes from one of
>> tbsaunde's cleanup patches:
>>
>> > diff --git a/gcc/regrename.c b/gcc/regrename.c
>> index 174d3b5..e5248a5 100644
>> --- a/gcc/regrename.c
>> +++ b/gcc/regrename.c
>> @@ -442,12 +442,10 @@ rename_chains (void)
>> continue;
>>
>>if (fixed_regs[reg] || global_regs[reg]
>> -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER
>> - || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)
>> -#else
>> - || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)
>> -#endif
>> - )
>> + || (!HARD_FRAME_POINTER_IS_FRAME_POINTER &&
>> frame_pointer_needed
>> + && reg == HARD_FRAME_POINTER_REGNUM)
>> + || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
>> + && reg == FRAME_POINTER_REGNUM))
>> continue;
>>
>>COPY_HARD_REG_SET (this_unavailable, unavailable);
>>
>> Looks like it never got reviewed and was committed as preapproved.
>>
>> The #ifdef we had before looks odd too. Maybe this should just read
>>
>>  if (fixed_regs[reg] || global_regs[reg]
>>  || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM))
>>

I think this breaks the symmetry with the statement above.

how about this (similar to what Jeff suggested):

Index: regrename.c
===
--- regrename.c (revision 240268)
+++ regrename.c (working copy)
@@ -464,8 +464,7 @@
if (frame_pointer_needed)
  {
add_to_hard_reg_set (&unavailable, Pmode, FRAME_POINTER_REGNUM);
-  if (!HARD_FRAME_POINTER_IS_FRAME_POINTER)
-   add_to_hard_reg_set (&unavailable, Pmode, 
HARD_FRAME_POINTER_REGNUM);
+  add_to_hard_reg_set (&unavailable, Pmode, HARD_FRAME_POINTER_REGNUM);
  }

FOR_EACH_VEC_ELT (id_to_chain, i, this_head)
@@ -479,10 +478,9 @@
 continue;

if (fixed_regs[reg] || global_regs[reg]
- || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
- && reg == HARD_FRAME_POINTER_REGNUM)
- || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
- && reg == FRAME_POINTER_REGNUM))
+ || (frame_pointer_needed
+ && (reg == HARD_FRAME_POINTER_REGNUM
+ || reg == FRAME_POINTER_REGNUMi)))
 continue;

COPY_HARD_REG_SET (this_unavailable, unavailable);


FRAME_POINTER_REGNUM is always special, and it
may be identical with HFP.  But it is not necessary
to use HARD_FRAME_POINTER_IS_FRAME_POINTER at all.

Bernd.

>
> I'll try bootstrapping and testing this change on arm-none-linux-gnueabihf.
>
> Thanks,
> Kyrill
>
>>
>> Bernd
>

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Kyrill Tkachov]

On 20/09/16 17:12, Kyrill Tkachov wrote:


On 20/09/16 16:30, Bernd Schmidt wrote:

On 09/20/2016 05:18 PM, Jeff Law wrote:

I assume HARD_FRAME_POINTER_REGNUM is never zero.

It could be zero.  It's just a hard register number.  No target has the
property that its hard frame pointer register is 0 though :-)


git blame to the rescue. The current state comes from one of tbsaunde's cleanup 
patches:

> diff --git a/gcc/regrename.c b/gcc/regrename.c
index 174d3b5..e5248a5 100644
--- a/gcc/regrename.c
+++ b/gcc/regrename.c
@@ -442,12 +442,10 @@ rename_chains (void)
continue;

   if (fixed_regs[reg] || global_regs[reg]
-#if !HARD_FRAME_POINTER_IS_FRAME_POINTER
- || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)
-#else
- || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)
-#endif
- )
+ || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
+ && reg == HARD_FRAME_POINTER_REGNUM)
+ || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
+ && reg == FRAME_POINTER_REGNUM))
continue;

   COPY_HARD_REG_SET (this_unavailable, unavailable);

Looks like it never got reviewed and was committed as preapproved.

The #ifdef we had before looks odd too. Maybe this should just read

 if (fixed_regs[reg] || global_regs[reg]
 || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM))



I'll try bootstrapping and testing this change on arm-none-linux-gnueabihf.



... and I'm hitting the same issue in sel-sched.c:
   if (fixed_regs[regno]
   || global_regs[regno]
   || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
   && regno == HARD_FRAME_POINTER_REGNUM)
   || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
   && regno == FRAME_POINTER_REGNUM)

I think the condition was copied from regrename.c (or the other way around),
so I'll fix it the same way.

Kyrill



Thanks,
Kyrill



Bernd

Re: [PATCH] More trivial bits from early LTO debug merge

[David Edelsohn]

dbxout.c has two definitions of the structure: dbx and xcoff.  The
patch forgot to update the xcoff one (lucky AIX :-).

* dbxout.c (xcoff_debug_hooks):  Add filename parameter to early_finish hook.

Thanks, David

Index: dbxout.c
===
--- dbxout.c(revision 240270)
+++ dbxout.c(working copy)
@@ -388,8 +388,8 @@ const struct gcc_debug_hooks xcoff_debug_hooks =
 {
   dbxout_init,
   dbxout_finish,
+  debug_nothing_charstar,
   debug_nothing_void,
-  debug_nothing_void,
   debug_nothing_int_charstar,
   debug_nothing_int_charstar,
   dbxout_start_source_file,

Re: [PATCH, docs] invoke.texi: random copy-editing

[Gerald Pfeifer]

On Mon, 5 Sep 2016, Sandra Loosemore wrote:
> Adjective phrases immediately before the noun they modify are 
> hyphenated. This is the same reason why we write "floating-point 
> arithmetic" but "floating point", unhyphenated, as a noun.

Thanks for the explanation / background, Sandra.

Below is the patch I just committed based on your other feedback.

Gerald

2016-09-20  Gerald Pfeifer  

* doc/invoke.texi (Warning Options): Simplify language.
(Optimize Options): Complete sentence.

Index: doc/invoke.texi
===
--- doc/invoke.texi (revision 240270)
+++ doc/invoke.texi (working copy)
@@ -3752,8 +3752,8 @@
 @code{register}.
 
 @item
-(C++ only) A base class is not initialized in a derived class's copy
-constructor.
+(C++ only) A base class is not initialized in the copy constructor
+of a derived class.
 
 @end itemize
 
@@ -9128,9 +9128,9 @@
 optimizers.
 
 When profile feedback is available (see @option{-fprofile-generate}) the actual
-recursion depth can be guessed from probability that function recurses via a
-given call expression.  This parameter limits inlining only to call expressions
-whose probability exceeds the given threshold (in percents).
+recursion depth can be guessed from the probability that function recurses
+via a given call expression.  This parameter limits inlining only to call
+expressions whose probability exceeds the given threshold (in percents).
 The default value is 10.
 
 @item early-inlining-insns

Re: [patch, libgomp, OpenACC] Additional enter/exit data map handling

[Cesar Philippidis]

On 08/29/2016 12:46 AM, Chung-Lin Tang wrote:

> Index: oacc-parallel.c
> ===
> --- oacc-parallel.c   (revision 239814)
> +++ oacc-parallel.c   (working copy)
> @@ -38,15 +38,23 @@
>  #include 
>  #include 
>  
> +/* Returns the number of mappings associated with the pointer or pset. PSET
> +   have three mappings, whereas pointer have two.  */
> +
>  static int
> -find_pset (int pos, size_t mapnum, unsigned short *kinds)
> +find_pointer (int pos, size_t mapnum, unsigned short *kinds)
>  {
>if (pos + 1 >= mapnum)
>  return 0;
>  
>unsigned char kind = kinds[pos+1] & 0xff;
>  
> -  return kind == GOMP_MAP_TO_PSET;
> +  if (kind == GOMP_MAP_TO_PSET)
> +return 3;
> +  else if (kind == GOMP_MAP_POINTER)
> +return 2;
> +
> +  return 0;
>  }

Is this still necessary with the firstprivatization of subarrays
pointers? Well, it might be for fortran. Conceptually, the gimplifier
should prune out those unnecessary firstprivate pointer clauses for
executable constructs such as enter/exit data and update.

Actually, this is one area in the spec where the intent of enter/exit
data conflicts with what it describes. If you look at the runtime
documentation for, say, acc_create, it states that

  acc_create (pvar, n*sizeof(var))

is equivalent to

  acc enter data create (pvar[n])

And to free acc_create, you use acc_delete. So in theory, you should be
able to

  #pragma acc enter data create (pvar[n])
  acc_free (pvar)

but this may result in a memory leak if the pointer mapping isn't freed.

Fortran is somewhat special because of the pointer sets. I'm not sure if
its possible to make the OpenACC runtime API compatible with enter/exit
data.

>  static void goacc_wait (int async, int num_waits, va_list *ap);
> @@ -298,7 +306,9 @@ GOACC_enter_exit_data (int device, size_t mapnum,
>  
>if (kind == GOMP_MAP_FORCE_ALLOC
> || kind == GOMP_MAP_FORCE_PRESENT
> -   || kind == GOMP_MAP_FORCE_TO)
> +   || kind == GOMP_MAP_FORCE_TO
> +   || kind == GOMP_MAP_TO
> +   || kind == GOMP_MAP_ALLOC)
>   {
> data_enter = true;
> break;
> @@ -312,31 +322,39 @@ GOACC_enter_exit_data (int device, size_t mapnum,
> kind);
>  }
>  
> +  /* In c, non-pointers and arrays are represented by a single data clause.
> + Dynamically allocated arrays and subarrays are represented by a data
> + clause followed by an internal GOMP_MAP_POINTER.
> +
> + In fortran, scalars and not allocated arrays are represented by a
> + single data clause. Allocated arrays and subarrays have three mappings:
> + 1) the original data clause, 2) a PSET 3) a pointer to the array data.
> +  */
> +
>if (data_enter)
>  {
>for (i = 0; i < mapnum; i++)
>   {
> unsigned char kind = kinds[i] & 0xff;
>  
> -   /* Scan for PSETs.  */
> -   int psets = find_pset (i, mapnum, kinds);
> +   /* Scan for pointers and PSETs.  */
> +   int pointer = find_pointer (i, mapnum, kinds);
>  
> -   if (!psets)
> +   if (!pointer)
>   {
> switch (kind)
>   {
> - case GOMP_MAP_POINTER:
> -   gomp_acc_insert_pointer (1, &hostaddrs[i], &sizes[i],
> - &kinds[i]);
> + case GOMP_MAP_ALLOC:
> +   acc_present_or_create (hostaddrs[i], sizes[i]);
> break;
>   case GOMP_MAP_FORCE_ALLOC:
> acc_create (hostaddrs[i], sizes[i]);
> break;
> - case GOMP_MAP_FORCE_PRESENT:
> + case GOMP_MAP_TO:
> acc_present_or_copyin (hostaddrs[i], sizes[i]);
> break;
>   case GOMP_MAP_FORCE_TO:
> -   acc_present_or_copyin (hostaddrs[i], sizes[i]);
> +   acc_copyin (hostaddrs[i], sizes[i]);
> break;

Thanks for correcting that. I had some of those data mappings wrong.

>   default:
> gomp_fatal (" GOACC_enter_exit_data UNHANDLED kind 
> 0x%.2x",
> @@ -346,12 +364,16 @@ GOACC_enter_exit_data (int device, size_t mapnum,
>   }
> else
>   {
> -   gomp_acc_insert_pointer (3, &hostaddrs[i], &sizes[i], &kinds[i]);
> +   if (!acc_is_present (hostaddrs[i], sizes[i]))
> + {
> +   gomp_acc_insert_pointer (pointer, &hostaddrs[i],
> +&sizes[i], &kinds[i]);
> + }
> /* Increment 'i' by two because OpenACC requires fortran
>arrays to be contiguous, so each PSET is associated with
>one of MAP_FORCE_ALLOC/MAP_FORCE_PRESET/MAP_FORCE_TO, and
>one MAP_POINTER.  */
> -   i += 2;
> +   i += pointer - 1;
>   }
>   }
>  }
> @@ -360,19 +382,15 @@ GOACC_enter_exit_data (int device, size_t mapnum,
>{
>   unsign

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/20/16 18:29, Kyrill Tkachov wrote:
>>
>> I'll try bootstrapping and testing this change on
>> arm-none-linux-gnueabihf.
>>
>
> ... and I'm hitting the same issue in sel-sched.c:
> if (fixed_regs[regno]
> || global_regs[regno]
> || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
> && regno == HARD_FRAME_POINTER_REGNUM)
> || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
> && regno == FRAME_POINTER_REGNUM)
>
> I think the condition was copied from regrename.c (or the other way
> around),
> so I'll fix it the same way.
>
> Kyrill
>
>

Interesting that this warning shakes so many bugs out of the gcc-tree,
but hit zero times for the whole linux and even openssl.  Even for the
more aggressive variant I had before.

I would have expected to see at least some false positives there,
but there were none.


Bernd.

libgo patch committed: Fix type passed to __splitstack_find

[Ian Lance Taylor]

PR 77642 points out that since the change to generate runtime types
from Go libgo is passing the wrong type to __splitstack_find: it is
passing uintptr* to a function that expects size_t*.  This breaks on
s390.  This patch, based on one from Andreas Krebbel in the PR, fixes
the problem.  Bootstrapped and ran Go testsuite on
x86_64-pc-linux-gnu.  Committed to mainline.

Ian
Index: gcc/go/gofrontend/MERGE
===
--- gcc/go/gofrontend/MERGE (revision 240146)
+++ gcc/go/gofrontend/MERGE (working copy)
@@ -1,4 +1,4 @@
-b34c93bf00ec4f2ad043ec89ff96989e0d1b26aa
+80720773ac1a3433b7de59ffa5c04744123247c3
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/runtime/proc.c
===
--- libgo/runtime/proc.c(revision 240053)
+++ libgo/runtime/proc.c(working copy)
@@ -2052,9 +2052,13 @@ doentersyscall()
 
// Leave SP around for GC and traceback.
 #ifdef USING_SPLIT_STACK
-   g->gcstack = __splitstack_find(nil, nil, &g->gcstacksize,
-  &g->gcnextsegment, &g->gcnextsp,
-  &g->gcinitialsp);
+   {
+ size_t gcstacksize;
+ g->gcstack = __splitstack_find(nil, nil, &gcstacksize,
+&g->gcnextsegment, &g->gcnextsp,
+&g->gcinitialsp);
+ g->gcstacksize = (uintptr)gcstacksize;
+   }
 #else
{
void *v;
@@ -2099,9 +2103,13 @@ runtime_entersyscallblock(void)
 
// Leave SP around for GC and traceback.
 #ifdef USING_SPLIT_STACK
-   g->gcstack = __splitstack_find(nil, nil, &g->gcstacksize,
-  &g->gcnextsegment, &g->gcnextsp,
-  &g->gcinitialsp);
+   {
+ size_t gcstacksize;
+ g->gcstack = __splitstack_find(nil, nil, &gcstacksize,
+&g->gcnextsegment, &g->gcnextsp,
+&g->gcinitialsp);
+ g->gcstacksize = (uintptr)gcstacksize;
+   }
 #else
g->gcnextsp = (byte *) &p;
 #endif

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Kyrill Tkachov]

On 20/09/16 15:13, Richard Earnshaw (lists) wrote:

On 08/09/16 12:00, Kyrill Tkachov wrote:

Hi all,

This is a rebase and slight respin of [1] that I sent out last November
to change all uses of
sprintf to snprintf in the arm backend.

Bootstrapped and tested on arm-none-linux-gnueabihf.
Ok for trunk?

Thanks,
Kyrill

[1] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html

2016-09-08  Kyrylo Tkachov  

 * config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
 rather than sprintf.
 (arm_set_fixed_conv_libfunc): Likewise.
 (arm_option_override): Likewise.
 (neon_output_logic_immediate): Likewise.
 (neon_output_shift_immediate): Likewise.
 (arm_output_multireg_pop): Likewise.
 (vfp_output_vstmd): Likewise.
 (output_move_vfp): Likewise.
 (output_move_neon): Likewise.
 (output_return_instruction): Likewise.
 (arm_elf_asm_cdtor): Likewise.
 (arm_output_shift): Likewise.
 (arm_output_iwmmxt_shift_immediate): Likewise.
 (arm_output_iwmmxt_tinsr): Likewise.
 * config/arm/neon.md (*neon_mov, VDX): Likewise.
 (*neon_mov, VQXMOV): Likewise.
 (neon_vc_insn): Likewise.
 (neon_vc_insn_unspec): Likewise.

Most of these should assert that truncation did not occur (it's an
internal error if the buffers aren't large enough).  In a few cases we
should call output_operand_lossage on failure since it might be due to a
user writing inline assembly and overflowing a buffer.

I don't think we should silently accept a truncated write.


Ok, I'm proposing a new function defined in system.h
(though I'm open to suggestions for other location).
It would be something like:

static inline int ATTRIBUTE_PRINTF_3
gcc_snprintf (char *str, size_t size, const char *format, ...)
{
  va_list ap;
  va_start(ap, format);
  size_t res = vsnprintf (str, size, format, ap);
  va_end (ap);
  /* vsnprintf returns >= size if input was truncated.  */
  gcc_assert (res < size);
  return res;
}

Would that be acceptable?

Thanks,
Kyrill



R.


arm-snprintf.patch


diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 
946f308ca84e232af8af6eca4813464914cbd59c..8ff6c0e18f7c2a2eed72e56402ed582c9f692d2d
 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -2388,9 +2388,10 @@ arm_set_fixed_optab_libfunc (optab optable, machine_mode 
mode,
char buffer[50];
  
if (num_suffix == 0)

-sprintf (buffer, "__gnu_%s%s", funcname, modename);
+snprintf (buffer, sizeof (buffer), "__gnu_%s%s", funcname, modename);
else
-sprintf (buffer, "__gnu_%s%s%d", funcname, modename, num_suffix);
+snprintf (buffer, sizeof (buffer), "__gnu_%s%s%d", funcname,
+ modename, num_suffix);
  
set_optab_libfunc (optable, mode, buffer);

  }
@@ -2409,8 +2410,8 @@ arm_set_fixed_conv_libfunc (convert_optab optable, 
machine_mode to,
&& ALL_FRACT_MODE_P (from) == ALL_FRACT_MODE_P (to))
  maybe_suffix_2 = "2";
  
-  sprintf (buffer, "__gnu_%s%s%s%s", funcname, fromname, toname,

-  maybe_suffix_2);
+  snprintf (buffer, sizeof (buffer), "__gnu_%s%s%s%s", funcname,
+   fromname, toname, maybe_suffix_2);
  
set_conv_libfunc (optable, to, from, buffer);

  }
@@ -3163,7 +3164,8 @@ arm_option_override (void)
if (!arm_selected_tune)
  arm_selected_tune = &all_cores[arm_selected_cpu->core];
  
-  sprintf (arm_arch_name, "__ARM_ARCH_%s__", arm_selected_cpu->arch);

+  snprintf (arm_arch_name, sizeof (arm_arch_name),
+   "__ARM_ARCH_%s__", arm_selected_cpu->arch);
insn_flags = arm_selected_cpu->flags;
arm_base_arch = arm_selected_cpu->base_arch;
  
@@ -12735,9 +12737,11 @@ neon_output_logic_immediate (const char *mnem, rtx *op2, machine_mode mode,

gcc_assert (is_valid != 0);
  
if (quad)

-sprintf (templ, "%s.i%d\t%%q0, %%2", mnem, width);
+snprintf (templ, sizeof (templ), "%s.i%d\t%%q0, %%2",
+ mnem, width);
else
-sprintf (templ, "%s.i%d\t%%P0, %%2", mnem, width);
+snprintf (templ, sizeof (templ), "%s.i%d\t%%P0, %%2",
+ mnem, width);
  
return templ;

  }
@@ -12757,9 +12761,11 @@ neon_output_shift_immediate (const char *mnem, char 
sign, rtx *op2,
gcc_assert (is_valid != 0);
  
if (quad)

-sprintf (templ, "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
+snprintf (templ, sizeof (templ),
+ "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
else
-sprintf (templ, "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
+snprintf (templ, sizeof (templ),
+ "%s.%c%d\t%%P0, %%P1, %%2", mnem, sign, width);
  
return templ;

  }
@@ -17858,17 +17864,17 @@ arm_output_multireg_pop (rtx *operands, bool 
return_pc, rtx cond, bool reverse,
conditional = reverse ? "%?%D0" : "%?%d0";
/* Can't use POP if returning from an interrupt.  */
if ((regno_base == SP_REGNUM) && update && !(interrupt_p && return_pc))
-sprintf (pattern, "pop%s\t{", conditional);
+snprintf (pattern, sizeof (pattern), "pop%s\t

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Richard Earnshaw (lists)]

On 20/09/16 17:49, Kyrill Tkachov wrote:
> 
> On 20/09/16 15:13, Richard Earnshaw (lists) wrote:
>> On 08/09/16 12:00, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> This is a rebase and slight respin of [1] that I sent out last November
>>> to change all uses of
>>> sprintf to snprintf in the arm backend.
>>>
>>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>> Ok for trunk?
>>>
>>> Thanks,
>>> Kyrill
>>>
>>> [1] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00937.html
>>>
>>> 2016-09-08  Kyrylo Tkachov  
>>>
>>>  * config/arm/arm.c (arm_set_fixed_optab_libfunc): Use snprintf
>>>  rather than sprintf.
>>>  (arm_set_fixed_conv_libfunc): Likewise.
>>>  (arm_option_override): Likewise.
>>>  (neon_output_logic_immediate): Likewise.
>>>  (neon_output_shift_immediate): Likewise.
>>>  (arm_output_multireg_pop): Likewise.
>>>  (vfp_output_vstmd): Likewise.
>>>  (output_move_vfp): Likewise.
>>>  (output_move_neon): Likewise.
>>>  (output_return_instruction): Likewise.
>>>  (arm_elf_asm_cdtor): Likewise.
>>>  (arm_output_shift): Likewise.
>>>  (arm_output_iwmmxt_shift_immediate): Likewise.
>>>  (arm_output_iwmmxt_tinsr): Likewise.
>>>  * config/arm/neon.md (*neon_mov, VDX): Likewise.
>>>  (*neon_mov, VQXMOV): Likewise.
>>>  (neon_vc_insn): Likewise.
>>>  (neon_vc_insn_unspec): Likewise.
>> Most of these should assert that truncation did not occur (it's an
>> internal error if the buffers aren't large enough).  In a few cases we
>> should call output_operand_lossage on failure since it might be due to a
>> user writing inline assembly and overflowing a buffer.
>>
>> I don't think we should silently accept a truncated write.
> 
> Ok, I'm proposing a new function defined in system.h
> (though I'm open to suggestions for other location).
> It would be something like:
> 
> static inline int ATTRIBUTE_PRINTF_3
> gcc_snprintf (char *str, size_t size, const char *format, ...)
> {
>   va_list ap;
>   va_start(ap, format);
>   size_t res = vsnprintf (str, size, format, ap);
>   va_end (ap);
>   /* vsnprintf returns >= size if input was truncated.  */
>   gcc_assert (res < size);
>   return res;
> }
> 
> Would that be acceptable?
> 

Yes, that's sort of what I had in mind.

Not sure if it needs to be inline, though; *printf are not generally
performance-critical functions and this bloats code somewhat with two
function calls for each invocation.

R.

> Thanks,
> Kyrill
> 
>>
>> R.
>>
>>> arm-snprintf.patch
>>>
>>>
>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>> index
>>> 946f308ca84e232af8af6eca4813464914cbd59c..8ff6c0e18f7c2a2eed72e56402ed582c9f692d2d
>>> 100644
>>> --- a/gcc/config/arm/arm.c
>>> +++ b/gcc/config/arm/arm.c
>>> @@ -2388,9 +2388,10 @@ arm_set_fixed_optab_libfunc (optab optable,
>>> machine_mode mode,
>>> char buffer[50];
>>>   if (num_suffix == 0)
>>> -sprintf (buffer, "__gnu_%s%s", funcname, modename);
>>> +snprintf (buffer, sizeof (buffer), "__gnu_%s%s", funcname,
>>> modename);
>>> else
>>> -sprintf (buffer, "__gnu_%s%s%d", funcname, modename, num_suffix);
>>> +snprintf (buffer, sizeof (buffer), "__gnu_%s%s%d", funcname,
>>> +  modename, num_suffix);
>>>   set_optab_libfunc (optable, mode, buffer);
>>>   }
>>> @@ -2409,8 +2410,8 @@ arm_set_fixed_conv_libfunc (convert_optab
>>> optable, machine_mode to,
>>> && ALL_FRACT_MODE_P (from) == ALL_FRACT_MODE_P (to))
>>>   maybe_suffix_2 = "2";
>>>   -  sprintf (buffer, "__gnu_%s%s%s%s", funcname, fromname, toname,
>>> -   maybe_suffix_2);
>>> +  snprintf (buffer, sizeof (buffer), "__gnu_%s%s%s%s", funcname,
>>> +fromname, toname, maybe_suffix_2);
>>>   set_conv_libfunc (optable, to, from, buffer);
>>>   }
>>> @@ -3163,7 +3164,8 @@ arm_option_override (void)
>>> if (!arm_selected_tune)
>>>   arm_selected_tune = &all_cores[arm_selected_cpu->core];
>>>   -  sprintf (arm_arch_name, "__ARM_ARCH_%s__", arm_selected_cpu->arch);
>>> +  snprintf (arm_arch_name, sizeof (arm_arch_name),
>>> +"__ARM_ARCH_%s__", arm_selected_cpu->arch);
>>> insn_flags = arm_selected_cpu->flags;
>>> arm_base_arch = arm_selected_cpu->base_arch;
>>>   @@ -12735,9 +12737,11 @@ neon_output_logic_immediate (const char
>>> *mnem, rtx *op2, machine_mode mode,
>>> gcc_assert (is_valid != 0);
>>>   if (quad)
>>> -sprintf (templ, "%s.i%d\t%%q0, %%2", mnem, width);
>>> +snprintf (templ, sizeof (templ), "%s.i%d\t%%q0, %%2",
>>> +  mnem, width);
>>> else
>>> -sprintf (templ, "%s.i%d\t%%P0, %%2", mnem, width);
>>> +snprintf (templ, sizeof (templ), "%s.i%d\t%%P0, %%2",
>>> +  mnem, width);
>>>   return templ;
>>>   }
>>> @@ -12757,9 +12761,11 @@ neon_output_shift_immediate (const char
>>> *mnem, char sign, rtx *op2,
>>> gcc_assert (is_valid != 0);
>>>   if (quad)
>>> -sprintf (templ, "%s.%c%d\t%%q0, %%q1, %%2", mnem, sign, width);
>>> +snprintf (templ, sizeof (te

[PATCH, i386]: Fix PR77621 (target part), handle Atom V2DFmode tuning through vector cost infrastructure

[Uros Bizjak]

Hello!

Attached patch improves Atom V2DFmode vectorization by penalizing V2DF
mode insns through vector cost infrastructure. Looking at Agner Fog's
instruction tables, it is evident that only V2DFmode arithmetic insns
(e.g. addpd, maxpd, sqrtpd, ...) have increased latencies, and we
shouldn't fully disable V2DFmode vectorization with a "big hammer"
approach.

As suggested in the PR, we now increase the cost of problematic insns
in ix86_add_stmt_cost. This is enough to prevent vectorization of
V2DFmode loops, but we still allow cases where a single problematic
insn would prevent vectorization of a complex, mostly integer loop.

(BTW: The factor 5 is arbitrary, but based on the factor between
latencies of V2DFmode  and DFmode insns).

2016-09-20  Uros Bizjak  

PR target/77621
* config/i386/i386.c (ix86_preferred_simd_mode) :
Don't return word_mode for !TARGET_VECTORIZE_DOUBLE.
(ix86_add_stmt_cost): Penalize DFmode vector operations
for !TARGET_VECTORIZE_DOUBLE.

testsuite/ChangeLog:

2016-09-20  Uros Bizjak  

PR target/77621
* gcc.target/i386/pr77621.c: New test.
* gcc.target/i386/vect-double-2.c: Update scan-tree-dump-times
pattern, loop should vectorize with -mtune=atom.

Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

Committed to mainline SVN.

Uros.
Index: config/i386/i386.c
===
--- config/i386/i386.c  (revision 240263)
+++ config/i386/i386.c  (working copy)
@@ -49554,9 +49554,7 @@ ix86_preferred_simd_mode (machine_mode mode)
return V4SFmode;
 
 case DFmode:
-  if (!TARGET_VECTORIZE_DOUBLE)
-   return word_mode;
-  else if (TARGET_AVX512F)
+  if (TARGET_AVX512F)
return V8DFmode;
   else if (TARGET_AVX && !TARGET_PREFER_AVX128)
return V4DFmode;
@@ -49647,9 +49645,14 @@ ix86_add_stmt_cost (void *data, int count, enum ve
   tree vectype = stmt_info ? stmt_vectype (stmt_info) : NULL_TREE;
   int stmt_cost = ix86_builtin_vectorization_cost (kind, vectype, misalign);
 
+  /* Penalize DFmode vector operations for !TARGET_VECTORIZE_DOUBLE.  */
+  if (kind == vector_stmt && !TARGET_VECTORIZE_DOUBLE
+  && vectype && GET_MODE_INNER (TYPE_MODE (vectype)) == DFmode)
+stmt_cost *= 5;  /* FIXME: The value here is arbitrary.  */
+
   /* Statements in an inner loop relative to the loop being
  vectorized are weighted more heavily.  The value here is
-  arbitrary and could potentially be improved with analysis.  */
+ arbitrary and could potentially be improved with analysis.  */
   if (where == vect_body && stmt_info && stmt_in_inner_loop_p (stmt_info))
 count *= 50;  /* FIXME.  */
 
Index: testsuite/gcc.target/i386/pr77621.c
===
--- testsuite/gcc.target/i386/pr77621.c (nonexistent)
+++ testsuite/gcc.target/i386/pr77621.c (working copy)
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -mtune=atom -msse2 -fdump-tree-vect-stats" } */
+
+void
+foo (double *x, int *y)
+{
+  int i;
+  for (i = 0; i < 8; i++)
+x[i] -= y[i] * x[i + 1];
+}
+
+/* { dg-final { scan-tree-dump-not "Vectorized loops: 1" "vect" } } */
Index: testsuite/gcc.target/i386/vect-double-2.c
===
--- testsuite/gcc.target/i386/vect-double-2.c   (revision 240263)
+++ testsuite/gcc.target/i386/vect-double-2.c   (working copy)
@@ -31,4 +31,4 @@ sse2_test (void)
 }
 }
 
-/* { dg-final { scan-tree-dump-not "vectorized 1 loops" "vect" } } */
+/* { dg-final { scan-tree-dump-times "Vectorized loops: 1" 1 "vect" } } */

Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Martin Sebor]

On 09/16/2016 12:19 PM, Jason Merrill wrote:

On 09/14/2016 01:03 PM, Martin Sebor wrote:

Attached is an updated patch that does away with the "overlapping"
warning and instead issues the same warnings as the C front end
(though with more context).

In struct flexmems_t I've retained the NEXT array even though only
the first element is used to diagnose problems.  The second element
is used by the find_flexarrays function to differentiate structs
with flexible array members in unions (which are valid) from those
in structs (which are not).

FWIW, I think this C restriction on structs is unnecessary and will
propose to remove it so that's valid to define a struct that contains
another struct (possibly recursively) with a flexible array member.
I.e., I think this should be made valid in C (and accepted without
the pedantic warning that GCC, and with this patch also G++, issues):


Agreed.


This is now N2083:
  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2083.htm




+  /* Is FLD a typedef for an anonymous struct?  */
+  bool anon_type_p
+   = (TREE_CODE (fld) == TYPE_DECL
+  && DECL_IMPLICIT_TYPEDEF_P (fld)
+  && anon_aggrname_p (DECL_NAME (fld)));


We talked earlier about handling typedefs in
cp_parser_{simple,single}_declaration, so that we catch

typedef struct { int a[]; } B;

or at least adding a FIXME comment here explaining that looking at
typedefs is a temporary hack.


I've added a FIXME comment.




+  /* Type of the member.  */
+  tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld)
: fld;


Why set "fldtype" to be a TYPE_DECL rather than its type?


I'm not sure I understand the question but (IIRC) the purpose of
this code is to detect invalid uses of flexible array members in
typedefs such as this:

   struct S { typedef struct { int i, a[]; } X2 [2]; };

Sadly, it doesn't do anything for

   struct X { int i, a[]; };
   struct S { typedef struct X X2 [2]; };


+  /* Determine the type of the array element or object referenced
+by the member so that it can be checked for flexible array
+members if it hasn't been yet.  */
+  tree eltype = TREE_CODE (fld) == FIELD_DECL ? fldtype :
TREE_TYPE (fld);


Given the above, this seems equivalent to

tree eltype = TREE_TYPE (fld);


Yes.




+  if (RECORD_OR_UNION_TYPE_P (eltype))
+   {

...

+ if (fmem->array && !fmem->after[bool (pun)])
+   {
+ /* Once the member after the flexible array has been found
+we're done.  */
+ fmem->after[bool (pun)] = fld;
+ break;
+   }

...

  if (field_nonempty_p (fld))
{

...

  /* Remember the first non-static data member after the flexible
 array member, if one has been found, or the zero-length
array
 if it has been found.  */
  if (fmem->array && !fmem->after[bool (pun)])
fmem->after[bool (pun)] = fld;
}


Can we do this in one place rather than two?


You mean merge the two assignments to fmem->after[bool (pun)] into
one?  I'm not sure.  There's quite a bit of conditional logic in
between them, including a recursive call that might set fmem.  What
would be gained by rewriting it all to merge the two assignments?
If it could be done I worry that I'd just end up duplicating the
conditions instead.




+ if (eltype == fldtype || TYPE_UNNAMED_P (eltype))


This is excluding arrays, so we don't diagnose, say,


struct A
{
  int i;
  int ar[];
};

struct B {
  A a[2];
};


Should we complain elsewhere about an array of a class with a flexible
array member?


Yes, there are outstanding gaps/bugs that remain to be fixed.  I tried
to limit this fix to not much more than the reported regression.  I did
end up tightening other things up as I found more gaps during testing
(though I probably should have resisted).  Sometimes I find it hard to
know where to stop but I feel like I need to draw the line somewhere.
I of course agree that the outstanding bugs should be fixed as well
but I'd prefer to do it in a separate change.


+ /* Does the field represent an anonymous struct?  */
+ bool anon_p = !DECL_NAME (fld) && ANON_AGGR_TYPE_P
(eltype);


You don't need to check DECL_NAME; ANON_AGGR_TYPE_P by itself indicates
that we're dealing with an anonymous struct/union.


Thanks, I've removed the variable.




   Similarly, PSTR is set to the outermost struct of which T is a member
   if one such struct exists, otherwise to NULL.  */

...

  find_flexarrays (eltype, fmem, false, pun,
   !pstr && TREE_CODE (t) == RECORD_TYPE ?
fld : pstr);

...

+diagnose_invalid_flexarray (const flexmems_t *fmem)
+{
+  if (fmem->array && fmem->enclosing
+  && pedwarn (location_of (fmem->enclosing), OPT_Wpedantic,
+ TYPE_DOMAIN (TREE_TYPE (fmem->array))
+ ? G_("invalid use of %q#T with a zero-size array "
+   

Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments

[Segher Boessenkool]

On Tue, Sep 20, 2016 at 08:52:46AM -0600, Jeff Law wrote:
> >- JUMP_LABEL can be a return which is not an insn.  That also seems
> >  like something that should be improved at some point.
> The JUMP_LABEL field within a JUMP_INSN?  That sounds like a bug.
> >>>
> >>>yes, see the comment before the declaration of
> >>>rtx_jump_insn::jump_label ()
> >>>it can be either a possibly deleted label, or a RETURN or SIMPLE_RETURN
> >>>expression.  Memory usage concerns aside its a tempting design to
> >>>change, but at the moment itts definitely intended to work this way,
> >>>there's plenty of code that checks ANY_RETURN_P (JUMP_LABEL (x)).
> >>
> >>Yes.  But rtl.texi still says:
> >>
> >>  Return insns count as jumps, but since they do not refer to any
> >>  labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.
> >
> >And the comment at the top of jump.c:
> >
> >   Each CODE_LABEL has a count of the times it is used
> >   stored in the LABEL_NUSES internal field, and each JUMP_INSN
> >   has one label that it refers to stored in the
> >   JUMP_LABEL internal field.  With this we can detect labels that
> >   become unused because of the deletion of all the jumps that
> >   formerly used them.  The JUMP_LABEL info is sometimes looked
> >   at by later passes.  For return insns, it contains either a
> >   RETURN or a SIMPLE_RETURN rtx.
> >
> >It's intentional, and really there's nothing wrong with it, or with
> >operands[] containing CODE_LABELs. The problem is trying to force a

I'm just pointing out the documentation is out-of-date here.  I'll do
a patch.

> >static type system onto a dynamically typed data structure.
> But what this doesn't answer is why we stuff RETURN/SIMPLE_RETURN into 
> the JUMP_LABEL field of a return.  I simply can't remember any rationale 
> behind that.

It is very inconvenient to parse the whole rtx to see if this is a
RETURN vs. a SIMPLE_RETURN (it can be inside a conditional or inside
a PARALLEL, etc.)

Ideally we would not have RETURN at all anymore, just SIMPLE_RETURN,
but that isn't going to happen any time soon.

> I suspect that if we dig further, we'll find that we can accomplish 
> whatever the goal was behind that decision in a way that easily works in 
> a world where we have separated rtx objects from objects that are part 
> of the insn chain.

We cannot easily extract the JUMP_LABEL of a normal jump from its rtx
pattern either (if at all).  So it seems the extra field of a JUMP_INSN
is here to stay.  We'll have to figure out how to nicely do INSN vs.
JUMP_INSN in an rtx_insn world.

> Just to be clear, I don't see us going to a world where everything we 
> have as an rtx today is a statically typed object. But there are things 
> that I think make sense to carve out, the most obvious being objects 
> which are part of the insn chain.

Agreed on both.  I have nightmares about the first, so please don't even
talk about going there :-)


Segher

Re: [patch,avr] Fix PR77326: CC_NONE might clobber condition code

[Denis Chertykov]

2016-09-20 13:07 GMT+03:00 Georg-Johann Lay :
> This fixes the case of CC_NONE (insn attribute for cc is "none"):
>
> Even in cases where the instructions of an insn do not change the condition
> code of the machine, they still might change some registers by clobber, set,
> etc.  If one such register overlaps an expression stored in
> cc_status.value1/2 we must reset cc_status as the value stored there no more
> represents the state of cc0.
>
> Ok to apply and backport?

Ok.
Please apply.

>
>
> Johann
>
> gcc/
> PR target/77326
> * config/avr/avr.c (avr_notice_update_cc) [CC_NONE]: If insn
> touches some regs mentioned in cc_status, do CC_STATUS_INIT.
>
> gcc/testsuite/
> PR target/77326
> * gcc.target/avr/torture/pr77326.c: New test.

Re: C/C++ PATCH to implement -Wbool-operation (PR c/77490)

[Marek Polacek]

On Mon, Sep 19, 2016 at 02:41:04PM -0400, Jason Merrill wrote:
> On 09/16/2016 05:01 AM, Marek Polacek wrote:
> > @@ -5853,7 +5853,16 @@ cp_build_unary_op (enum tree_code code, tree xarg, 
> > bool noconvert,
> >arg, true)))
> > errstring = _("wrong type argument to bit-complement");
> >else if (!noconvert && CP_INTEGRAL_TYPE_P (TREE_TYPE (arg)))
> > -   arg = cp_perform_integral_promotions (arg, complain);
> > +   {
> > + /* Warn if the expression has boolean value.  */
> > + location_t location = EXPR_LOC_OR_LOC (arg, input_location);
> 
> Let's move this variable to the beginning of the function; hopefully it will
> become a parameter soon.
 
Done.

> > + if ((TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE
> > +  || truth_value_p (TREE_CODE (arg)))
> 
> You shouldn't need to check truth_value_p; in C++ all truth operations have
> type bool.

Oops, force of habit...

> > + && warning_at (location, OPT_Wbool_operation,
> > +"%<~%> on a boolean expression"))
> 
> And let's refer to "expression of type bool" rather than "boolean
> expression".

Adjusted (and in the C FE too).

Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk?

2016-09-20  Marek Polacek  

PR c/77490
* c.opt (Wbool-operation): New.

* c-typeck.c (build_unary_op): Warn about bit not on expressions that
have boolean value.  Warn about ++/-- on booleans.

* typeck.c (cp_build_unary_op): Warn about bit not on expressions that
have boolean value.

* doc/invoke.texi: Document -Wbool-operation.

* c-c++-common/Wbool-operation-1.c: New test.
* gcc.dg/Wbool-operation-1.c: New test.

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 6cf915d..0e8d68a 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -315,6 +315,10 @@ Wbool-compare
 C ObjC C++ ObjC++ Var(warn_bool_compare) Warning LangEnabledBy(C ObjC C++ 
ObjC++,Wall)
 Warn about boolean expression compared with an integer value different from 
true/false.
 
+Wbool-operation
+C ObjC C++ ObjC++ Var(warn_bool_op) Warning LangEnabledBy(C ObjC C++ 
ObjC++,Wall)
+Warn about certain operations on boolean expressions.
+
 Wframe-address
 C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC C++ 
ObjC++,Wall)
 Warn when __builtin_frame_address or __builtin_return_address is used unsafely.
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 059ad1f..4006b1a 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -4196,6 +4196,22 @@ build_unary_op (location_t location, enum tree_code 
code, tree xarg,
  || (typecode == VECTOR_TYPE
  && !VECTOR_FLOAT_TYPE_P (TREE_TYPE (arg
{
+ tree e = arg;
+
+ /* Warn if the expression has boolean value.  */
+ while (TREE_CODE (e) == COMPOUND_EXPR)
+   e = TREE_OPERAND (e, 1);
+
+ if ((TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE
+  || truth_value_p (TREE_CODE (e)))
+ && warning_at (location, OPT_Wbool_operation,
+"%<~%> on an expression of type bool"))
+   {
+ gcc_rich_location richloc (location);
+ richloc.add_fixit_insert_before (location, "!");
+ inform_at_rich_loc (&richloc, "did you mean to use logical "
+ "not?");
+   }
  if (!noconvert)
arg = default_conversion (arg);
}
@@ -4306,6 +4322,16 @@ build_unary_op (location_t location, enum tree_code 
code, tree xarg,
"decrement of enumeration value is invalid in C++");
}
 
+  if (TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE)
+   {
+ if (code == PREINCREMENT_EXPR || code == POSTINCREMENT_EXPR)
+   warning_at (location, OPT_Wbool_operation,
+   "increment of an expression of type bool");
+ else
+   warning_at (location, OPT_Wbool_operation,
+   "decrement of an expression of type bool");
+   }
+
   /* Ensure the argument is fully folded inside any SAVE_EXPR.  */
   arg = c_fully_fold (arg, false, NULL);
 
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index c53a85a..dee17b3 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -5792,6 +5792,7 @@ cp_build_unary_op (enum tree_code code, tree xarg, bool 
noconvert,
 {
   /* No default_conversion here.  It causes trouble for ADDR_EXPR.  */
   tree arg = xarg;
+  location_t location = EXPR_LOC_OR_LOC (arg, input_location);
   tree argtype = 0;
   const char *errstring = NULL;
   tree val;
@@ -5853,7 +5854,14 @@ cp_build_unary_op (enum tree_code code, tree xarg, bool 
noconvert,
   arg, true)))
errstring = _("wrong type argument to bit-complement");
   else if (!noconvert && CP_INTEGRAL_TYPE_P (TREE_TYPE (arg)))
-   arg = cp_perform_integral_promotions (arg, c

Re: [PATCH] Optimise the fpclassify builtin to perform integer operations when possible

[Joseph Myers]

On Tue, 20 Sep 2016, Jeff Law wrote:

> Could we defer lowering in the case where the object is not addressable until
> gimple->rtl expansion time?  That's the best time to introduce target
> dependencies into the code we generate.

If we do that (remembering that -fsignaling-nans always wants the integer 
path), we need to make sure there are tests of fpclassify that reliably 
exercise both paths

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments

[Jeff Law]

On 09/20/2016 11:11 AM, Segher Boessenkool wrote:

On Tue, Sep 20, 2016 at 08:52:46AM -0600, Jeff Law wrote:

- JUMP_LABEL can be a return which is not an insn.  That also seems
 like something that should be improved at some point.

The JUMP_LABEL field within a JUMP_INSN?  That sounds like a bug.


yes, see the comment before the declaration of
rtx_jump_insn::jump_label ()
it can be either a possibly deleted label, or a RETURN or SIMPLE_RETURN
expression.  Memory usage concerns aside its a tempting design to
change, but at the moment itts definitely intended to work this way,
there's plenty of code that checks ANY_RETURN_P (JUMP_LABEL (x)).


Yes.  But rtl.texi still says:

 Return insns count as jumps, but since they do not refer to any
 labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.


And the comment at the top of jump.c:

  Each CODE_LABEL has a count of the times it is used
  stored in the LABEL_NUSES internal field, and each JUMP_INSN
  has one label that it refers to stored in the
  JUMP_LABEL internal field.  With this we can detect labels that
  become unused because of the deletion of all the jumps that
  formerly used them.  The JUMP_LABEL info is sometimes looked
  at by later passes.  For return insns, it contains either a
  RETURN or a SIMPLE_RETURN rtx.

It's intentional, and really there's nothing wrong with it, or with
operands[] containing CODE_LABELs. The problem is trying to force a


I'm just pointing out the documentation is out-of-date here.  I'll do
a patch.


static type system onto a dynamically typed data structure.

But what this doesn't answer is why we stuff RETURN/SIMPLE_RETURN into
the JUMP_LABEL field of a return.  I simply can't remember any rationale
behind that.


It is very inconvenient to parse the whole rtx to see if this is a
RETURN vs. a SIMPLE_RETURN (it can be inside a conditional or inside
a PARALLEL, etc.)
So it's similar to how we use JUMP_LABEL to find the jump target without 
having to dig through the whole rtx.





Ideally we would not have RETURN at all anymore, just SIMPLE_RETURN,
but that isn't going to happen any time soon.

And it wouldn't totally resolve this anyway.




I suspect that if we dig further, we'll find that we can accomplish
whatever the goal was behind that decision in a way that easily works in
a world where we have separated rtx objects from objects that are part
of the insn chain.


We cannot easily extract the JUMP_LABEL of a normal jump from its rtx
pattern either (if at all).  So it seems the extra field of a JUMP_INSN
is here to stay.  We'll have to figure out how to nicely do INSN vs.
JUMP_INSN in an rtx_insn world.

Both INSN and JUMP_INSNs are rtx_insn *s.

The problem is we're stuffing a (return) or (simple_return) rtx into a 
field that most of the time has an rtx_insn * (CODE_LABEL).


A hack-ish way to address this would be to create special CODE_LABELs 
that represent (return) and (simple_return) and stuff that into the 
JUMP_LABEL field.  At which point the JUMP_LABEL field should turn into 
an rtx_insn * and the as_a casts related to this wart go away.


There may be cleaner ways, but there's certainly ways to move forward here.




Just to be clear, I don't see us going to a world where everything we
have as an rtx today is a statically typed object. But there are things
that I think make sense to carve out, the most obvious being objects
which are part of the insn chain.


Agreed on both.  I have nightmares about the first, so please don't even
talk about going there :-)
While I could see some value in static typing enough to allow static 
checking things like we don't try to access out-of-range operands (say 
XEXP (x, 1)) where X is a unary code.  I don't see the level of pain to 
get there being worth it.


It's much like what we see in the tree world.  There's value in 
separating _DECL, _TYPE and _EXPR nodes.  But going deeper into in the 
_EXPR nodes seems like a huge level of pain.


jeff

Go patch committed: Avoid warning by using a local var for std::ofstream

[Ian Lance Taylor]

GCC PR 77625 is about a warning while compiling the Go frontend.  The
Go frontend called `new std::ofstream()`, and the warning is "error:
‘new’ of type ‘std::ofstream {aka std::basic_ofstream}’ with
extended alignment 16".  Frankly I'm not sure how this supposed to
work: shouldn't it be possible to write new std::ofstream()?  But the
problem is easy to avoid, since in this case the std::ofstream can be
a local variable, and that is a better approach anyhow.  This patch
was bootstrapped on x86_64-pc-linux-gnu.  Committed to mainline.

Ian
Index: gcc/go/gofrontend/MERGE
===
--- gcc/go/gofrontend/MERGE (revision 240275)
+++ gcc/go/gofrontend/MERGE (working copy)
@@ -1,4 +1,4 @@
-80720773ac1a3433b7de59ffa5c04744123247c3
+57d120d75be87c2a0da67e750f16929891f1b8f4
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: gcc/go/gofrontend/ast-dump.cc
===
--- gcc/go/gofrontend/ast-dump.cc   (revision 240053)
+++ gcc/go/gofrontend/ast-dump.cc   (working copy)
@@ -166,24 +166,24 @@ const char* kAstDumpFileExtension = ".du
 void
 Ast_dump_context::dump(Gogo* gogo, const char* basename)
 {
-  std::ofstream* out = new std::ofstream();
+  std::ofstream out;
   std::string dumpname(basename);
   dumpname += ".dump.ast";
-  out->open(dumpname.c_str());
+  out.open(dumpname.c_str());
 
-  if (out->fail())
+  if (out.fail())
 {
   error("cannot open %s:%m, -fgo-dump-ast ignored", dumpname.c_str());
   return;
 }
 
   this->gogo_ = gogo;
-  this->ostream_ = out;
+  this->ostream_ = &out;
 
   Ast_dump_traverse_blocks_and_functions adtbf(this);
   gogo->traverse(&adtbf);
 
-  out->close();
+  out.close();
 }
 
 // Dump a textual representation of a type to the

Re: [PATCH][ARM][v2] Use snprintf rather than sprintf

[Pedro Alves]

On 09/20/2016 05:49 PM, Kyrill Tkachov wrote:

> Ok, I'm proposing a new function defined in system.h
> (though I'm open to suggestions for other location).
> It would be something like:
> 
> static inline int ATTRIBUTE_PRINTF_3
> gcc_snprintf (char *str, size_t size, const char *format, ...)
> {
>   va_list ap;
>   va_start(ap, format);
>   size_t res = vsnprintf (str, size, format, ap);
>   va_end (ap);
>   /* vsnprintf returns >= size if input was truncated.  */
>   gcc_assert (res < size);
>   return res;
> }
> 
> Would that be acceptable?

gdb has had exactly that for eons:

int
xsnprintf (char *str, size_t size, const char *format, ...)
{
  va_list args;
  int ret;

  va_start (args, format);
  ret = vsnprintf (str, size, format, args);
  gdb_assert (ret < size);
  va_end (args);

  return ret;
}

Maybe reuse the same name?  It follows the naming scheme of
xmalloc, etc., with x meaning it never fails.

Even better would be to put this in libiberty.

And perhaps even better would be to get rid of the hardcoded
buffer sizes and use libiberty's existing xasprintf instead.

[PATCH, i386]: Use pow2p_hwi some more

[Uros Bizjak]

2016-09-20  Uros Bizjak  

* config/i386/i386.md (mult->ashift peephole2s): Use pow2p_hwi
instead of exact_log2.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32} ,
committed to mainline SVN.

Uros.
Index: config/i386/i386.md
===
--- config/i386/i386.md (revision 240276)
+++ config/i386/i386.md (working copy)
@@ -18304,7 +18304,7 @@
   [(set (match_operand:SWI48 0 "register_operand")
(mult:SWI48 (match_dup 0)
(match_operand:SWI48 1 "const_int_operand")))]
-  "exact_log2 (INTVAL (operands[1])) >= 0
+  "pow2p_hwi (INTVAL (operands[1]))
&& peep2_regno_dead_p (0, FLAGS_REG)"
   [(parallel [(set (match_dup 0) (ashift:SWI48 (match_dup 0) (match_dup 1)))
  (clobber (reg:CC FLAGS_REG))])]
@@ -18316,7 +18316,7 @@
  (mult:SI (match_operand:SI 1 "register_operand")
   (match_operand:SI 2 "const_int_operand"]
   "TARGET_64BIT
-   && exact_log2 (INTVAL (operands[2])) >= 0
+   && pow2p_hwi (INTVAL (operands[2]))
&& REGNO (operands[0]) == REGNO (operands[1])
&& peep2_regno_dead_p (0, FLAGS_REG)"
   [(parallel [(set (match_dup 0)

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

David, in the way of feedback, I spent hours debugging the simple
multiline test I added.  It seems that DejaGnu is exquisitely
sensitive to whitespaces in the multiline output.  I appreciate
that spacing is essential to lining up the caret and the tildes
with the text of the program but tests fail even when all the
expected output is lined up right in the multiline directive but
off by a space (or tab) with respect to the actual output.  That
DejaGnu output doesn't make this very clear at all makes debugging
these types of issues a pain.  I wonder if there's a better to do
this.


Gah; I'm sorry this was such a pain to do.

I tend to just copy&paste the stuff I want to quote directly from
Emacs's compilation buffer.


Yes, I did start with that.  The problem is that I have a script
that I run to help massage my patches to the format required by
the GCC Coding Conventions.  The script mainly replaces blocks
of spaces with tabs. So while my initial patch worked, the final
one didn't because of the tabs, and it took me hours to figure
out why.  Because I couldn't see the difference I thought it was
a bug in DejaGnu or some other tool that was different between
the two machines I was using for testing the two patches.  Until
it occurred to me to diff the test itself... (FWIW, I think this
style convention is bad and should be abolished in favor of using
only plain spaces.)



However a nit, which I think is why you had problems...


...


You have two pairs of dg-begin/end-multiline-output, but I think it's
better to have four; use a single begin/end pair for each call to
diagnostic_show_locus.  Hopefully doing that ought to make things a bit
less sensitive to whitespace, and easier to debug: each begin/end can
be handled by itself, whereas by combining them it's harder for
multiline.exp to detect them.  If combined, they can only be detected
if all of the dg-warning/dg-messages work (and are thus pruned by
prune.exp), if any aren't pruned, the multiline outputs will also fail.
 Maybe this exacerbated the problem?


Maybe.  I remember experimenting with the multiline directives when
I first added the test and was struggling to get it to work.  I think
the problems I was having were unrelated to tabs but probably had
something to do with the exact number of leading spaces.

FWIW, to make the multiline directives less prone to this type of
a problem it seems that instead of counting each leading space and
tab character and treating them as ordinary they could verify that
the first non-whitespace character on each line of the actual output
lines up with the first non-whitespace character of the expected
output with some constant offset.  That way spaces vs tabs shouldn't
matter as long as they were used consistently (or they could be made
not to matter at all if a tab was treated as a single space).  What
do you think about that?



So something like this, grouping the 4 diagnostics together with their
diagnostic_show_locus output by opening and closing comments (line
numbers will need adjusting):


Thanks.  Let me apply it and see how it works.



Another nit, if I may: FWIW I'm not in love with the wording of the messages.  
Sorry to bikeshed, but how about:
  warning: buffer overflow will occur when writing terminating NUL
and:
  note: formatted output of 2 bytes into a destination of size 1
or somesuch.


I won't claim the text of the messages is perfect but I did spend
a lot of time tweaking them.  That's not to say they can't be
improved but changing them means quite a bit of work adjusting
the tests.  At this point, I'd like to focus on getting the patch
committed.  After that, if there's still time, I'm happy to take
feedback and tweak the diagnostics based on it.

Thanks again for your help and the suggestions above!

Martin

Re: [PATCH] libstdc++/77641 fix std::variant for literal types

[Tim Shen]

On Mon, Sep 19, 2016 at 1:06 PM, Tim Shen  wrote:
> I believe that it's a "typo" from me - it should be
> is_trivially_destructible, not is_default_constructible (so that we
> can avoid using aligned_storage in the corresponding specialization).
> is_literal_type works, since literal types are trivially destructible.

Sorry I misunderstood your patch.

The underlying problem is that _Variant_storage wasn't always default
constructible, but it should be.

Jon, your fix doesn't fix the following constexpr variation of your test case:
  struct X {
constexpr X() { }
  };

  constexpr std::variant v1 = X{};

So I have a patch here to make _Variant_storage's internal union
default constructible.

Tested on x86_64-linux-gnu.


-- 
Regards,
Tim Shen


a.diff
Description: Binary data

Re: Fix libgo syscall test on Solaris

[Ian Lance Taylor]

On Mon, Sep 12, 2016 at 1:07 AM, Rainer Orth
 wrote:
> The libgo syscall test has been failing on Solaris for quite some time:
>
> exec_unix_test.go:174:19: error: reference to undefined identifier 
> 'syscall.Ioctl'
>   errno := syscall.Ioctl(tty.Fd(), syscall.TIOCGPGRP, 
> uintptr(unsafe.Pointer(&fpgrp)))
>^
> exec_unix_test.go:209:18: error: reference to undefined identifier 
> 'syscall.Ioctl'
>   errno = syscall.Ioctl(tty.Fd(), syscall.TIOCSPGRP, 
> uintptr(unsafe.Pointer(&fpgrp)))
>   ^
> FAIL: syscall
>
> The following patch fixes it, tested across the whole {i386-pc,
> sparc-sun}-solaris2.1[012] range.

Thanks.  Committed to mainline.

Ian

Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/20/16 16:51, Jason Merrill wrote:
> On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger
>  wrote:
>> On 09/20/16 13:29, Kyrill Tkachov wrote:
>>>
>>> arm bootstrap is now failing:
>>> $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in
>>> boolean context [-Werror=int-in-bool-context]
>>>   : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>>>  ~^~
>>> $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro
>>> 'TARGET_ARM_FP'
>>>  if (TARGET_ARM_FP)
>>>
>>>
>>> The full definition of TARGET_ARM_FP is:
>>> #define TARGET_ARM_FP\
>>> (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
>>>   : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>>> : 0)
>>>
>>> We want it set to 0 when there's no FP but when FP is available we set
>>> it to a bitmask
>>> to suggest the level that is available. That seems like a legitimate use
>>> to me.
>>>
>>
>> Ok, I see, sorry for that.
>>
>> I think I will have to suppress the warning if the conditional is in
>> a macro somehow.
>
> from_macro_expansion_at will help with that.
>
> Though it seems to me that the issue here is more that (TARGET_FP16 ?
> 14 : 12) is not in a boolean context, it's in an integer context; only
> the outer ?: is in a boolean context.
>
> I also still think the warning message should be changed.
>

I try this:

Index: c-common.c
===
--- c-common.c  (revision 240268)
+++ c-common.c  (working copy)
@@ -4652,7 +4652,8 @@
   TREE_OPERAND (expr, 0));

  case COND_EXPR:
-  if (warn_int_in_bool_context)
+  if (warn_int_in_bool_context
+ && !from_macro_expansion_at (EXPR_LOCATION (expr)))
{
  tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
  tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
@@ -4663,7 +4664,8 @@
  && (!integer_onep (val1)
  || !integer_onep (val2)))
warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
-   "?: using integer constants in boolean context");
+   "?: using integer constants in boolean context, "
+   "the expression will always evaluate to %");
}
/* Distribute the conversion into the arms of a COND_EXPR.  */
if (c_dialect_cxx ())


That will fix "if (TARGET_ARM_FP)" which is fine.

But this seems to suppress all such warnings from an assert
macro too.  Like for instance "assert(a?1:2)".

Sure, we would not be interested in a ?: that is part of the assert
macro itself, but the expression that is evaluated by the macro should
be checked, but that is no longer done, because the macro parameter is
now also from the macro expansion.  But it is initially from the
macro invocation point.  Ideas?


Thanks
Bernd.

[committed] Adjust fall through comment

[Marek Polacek]

This patch adjusts a fall through comment so that our -Wimplicit-fallthrough
comment parsing handles this well.

Applying to trunk.

2016-09-20  Marek Polacek  

* trans-intrinsic.c (conv_expr_ref_to_caf_ref): Adjust fall through
comment.

diff --git gcc/fortran/trans-intrinsic.c gcc/fortran/trans-intrinsic.c
index d6453c5..c6883dc 100644
--- gcc/fortran/trans-intrinsic.c
+++ gcc/fortran/trans-intrinsic.c
@@ -1365,7 +1365,7 @@ conv_expr_ref_to_caf_ref (stmtblock_t *block, gfc_expr 
*expr)
   handling easier.  */
stride = gfc_index_one_node;
 
- /* Intentionally fall through.  */
+ /* Fall through.  */
case DIMEN_ELEMENT:
  if (ref->u.ar.start[i])
{

Marek