Re: [PATCH] manual: _Bool has trap representations

[Florian Weimer]

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?

Thanks,
Florian

Re: PR35503 - warn for restrict pointer

[Martin Sebor]

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.

Martin

Re: [RFC][IPA-VRP] Early VRP Implementation

[kugan]

Hi Richard,

Thanks for the review.

On 19/09/16 22:56, Richard Biener wrote:

On Sun, Sep 18, 2016 at 10:50 PM, kugan
 wrote:

Hi Richard,



On 16/09/16 20:21, Richard Biener wrote:


On Fri, Sep 16, 2016 at 7:59 AM, kugan
 wrote:


Hi Richard,

Thanks for the review.

On 14/09/16 22:04, Richard Biener wrote:



On Tue, Aug 23, 2016 at 4:11 AM, Kugan Vivekanandarajah
 wrote:



Hi,

On 19 August 2016 at 21:41, Richard Biener 
wrote:



On Tue, Aug 16, 2016 at 9:45 AM, kugan
 wrote:



Hi Richard,





I am now having -ftree-evrp which is enabled all the time. But This
will
only be used for disabling the early-vrp. That is, early-vrp will be
run
when ftree-vrp is enabled and ftree-evrp is not explicitly disabled.
Is
this
OK?




Why would one want to disable early-vrp?  I see you do this in the
testsuite
for non-early VRP unit-tests but using -fdisable-tree-evrp1 there
would be ok as well.




Removed it altogether. I though that you wanted a way to disable
early-vrp for testing purposes.




But there is via the generic -fdisable-tree-DUMPFILE way.




OK. I didnt know about that.



Note that you want to have a custom valueize function instead of just
follow_single_use_edges as you want to valueize all SSA names
according
to their lattice value (if it has a single value).  You can use
vrp_valueize
for this though that gets you non-single-use edge following as well.
Eventually it's going to be cleaner to do what the SSA propagator does
and
before folding do

   did_replace = replace_uses_in (stmt, vrp_valueize);
   if (fold_stmt (, follow_single_use_edges)
   || did_replace)
 update_stmt (gsi_stmt (gsi));

exporting replace_uses_in for this is ok.  I guess I prefer this for
now.




I also added the above.  I noticed that I need
recompute_tree_invariant_for_addr_expr as in ssa_propagate. My initial
implementation also had gimple_purge_all_dead_eh_edges and
fixup_noreturn_call as in ssa_propagat but I thinj that is not needed
as it would be done at the end of the pass.




I don't see this being done at the end of the pass.  So please
re-instantiate
that parts.




I have copied these part as well.


With this I noticed more stmts are folded before vrp1. This required
me to adjust some testcases.



Overall this now looks good apart from the folding and the
VR_INITIALIZER thing.

You can commit the already approved refactoring changes and combine
this
patch with the struct value_range move, this way I can more easily
look
into
issues with the UNDEFINED thing if you can come up with a testcase
that
doesn't work.



I have now committed all the dependent patches.

Attached patch passes regression and bootstrap except pr33738.C. This
is an unrelated issue as discussed in
https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01386.html

Is this OK?




+/* Initialize local data structures for VRP.  If DOM_P is true,
+   we will be calling this from early_vrp where value range propagation
+   is done by visiting stmts in dominator tree.  ssa_propagate engine
+   is not used in this case and that part of the ininitialization will
+   be skipped.  */
+
+static void
+vrp_initialize ()

comment needs updating now.


Done.



 static void
-extract_range_from_phi_node (gphi *phi, value_range *vr_result)
+extract_range_from_phi_node (gphi *phi, value_range *vr_result,
+bool early_vrp_p)
 {


I don't think you need this changes now that you have
stmt_visit_phi_node_in_dom_p
guarding its call.




OK removed it. That also mean I had to put scev_* in the early_vrp.




+static bool
+stmt_visit_phi_node_in_dom_p (gphi *phi)
+{
+  ssa_op_iter iter;
+  use_operand_p oprnd;
+  tree op;
+  value_range *vr;
+  FOR_EACH_PHI_ARG (oprnd, phi, iter, SSA_OP_USE)
+{
+  op = USE_FROM_PTR (oprnd);
+  if (TREE_CODE (op) == SSA_NAME)
+   {
+ vr = get_value_range (op);
+ if (vr->type == VR_UNDEFINED)
+   return false;
+   }
+}

I think this is overly conservative in never allowing UNDEFINED on PHI
node args (even if the def was actually visited).  I think that the most
easy way to improve this bit would be to actually track visited blocks.
You already set the EDGE_EXECUTABLE flag on edges so you could
clear BB_VISITED on all blocks and set it in the before_dom_children
hook (at the end).  Then the above can be folded into the PHI visiting:

bool has_unvisited_pred = false;
FOR_EACH_EDGE (e, ei, bb->preds)
   if (!(e->src->flags & BB_VISITED))
 {
has_unvisited_preds = true;
break;
 }


OK done.

I also had to check for uninitialized variables that will have
VR_UNDEFINED
as range. We do not visit GIMPLE_NOP.



But VR_UNDEFINED of uninitialized variables is fine to use.



Indeed. I was really trying to fix another problem with this.

The real 

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan]

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 then
materializing the associated ops via rewrite_expr_tree.


Here is a draft patch which records the stmt chain when in
zero_one_operation and then fixes it when OP is removed. when we
update *def, that will update the ops vector. Does this looks sane?



Yes.  A few comments below

+  /* PR72835 - Record the stmt chain that has to be updated such that
+ we dont use the same LHS when the values computed are different.  */
+  auto_vec stmts_to_fix;

use auto_vec here so we get stack allocation only most
of the times


Done.


  if (stmt_is_power_of_op (stmt, op))
{
+ make_new_ssa_for_all_defs (def, op, stmts_to_fix);
  if (decrement_power (stmt) == 1)
propagate_op_to_single_use (op, stmt, def);

for the cases you end up with propagate_op_to_single_use its argument
stmt is handled superfluosly in the new SSA making, I suggest to pop it
from the stmts_to_fix vector in that case.  I suggest to break; instead
of return in all cases and do the make_new_ssa_for_all_defs call at
the function end instead.


Done.


@@ 

Re: [PATCH], Use VMRGEW on PowerPC power8/power9 to construct V4SFmode

[Michael Meissner]

On Mon, Sep 19, 2016 at 06:20:59PM -0500, Segher Boessenkool wrote:
> On Mon, Sep 19, 2016 at 06:51:42PM -0400, Michael Meissner wrote:
> > > > However ISA 2.07 (i.e. power8) added the VMRGEW instruction, which can 
> > > > do this
> > > > more simply:
> > > > 
> > > > xxpermdi 34,1,2,0
> > > > xxpermdi 32,3,4,0
> > > > xvcvdpsp 34,34
> > > > xvcvdpsp 32,32
> > > > vmrgew 2,2,0
> > > 
> > > This results in {a,c,b,d} instead?
> > 
> > Yes.
> 
> [ snip ]
> 
> My question was if you typoed/pastoed/thinkoed it here and you meant
> 
> xxpermdi 34,1,3,0
> xxpermdi 32,2,4,0
> xvcvdpsp 34,34
> xvcvdpsp 32,32
> vmrgew 2,2,0
> 
> which a) works, and b) seems to be what the code generates :-)

Yes it works on both big endian and little endian power8's.  And yes, it is the
code generated by the patch.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH 7/8] make next/prev active_insn and active_insn_p take rtx_insn *

[Trevor Saunders]

On Mon, Sep 19, 2016 at 03:17:33PM -0600, Jeff Law wrote:
> On 09/14/2016 01:21 PM, tbsaunde+...@tbsaunde.org wrote:
> > From: Trevor Saunders 
> > 
> > gcc/ChangeLog:
> > 
> > 2016-09-14  Trevor Saunders  
> > 
> > * emit-rtl.c (next_active_insn): Change argument type to
> > rtx_insn *.
> > (prev_active_insn): Likewise.
> > (active_insn_p): Likewise.
> > * rtl.h: Adjust prototypes.
> > * cfgcleanup.c (merge_blocks_move_successor_nojumps): Adjust.
> > * config/arc/arc.md: Likewise.
> > * config/pa/pa.c (branch_to_delay_slot_p): Likewise.
> > (branch_needs_nop_p): Likewise.
> > (use_skip_p): Likewise.
> > * config/sh/sh.c (gen_block_redirect): Likewise.
> > (split_branches): Likewise.
> > * reorg.c (optimize_skip): Likewise.
> > (fill_simple_delay_slots): Likewise.
> > (fill_slots_from_thread): Likewise.
> > (relax_delay_slots): Likewise.
> > * resource.c (mark_target_live_regs): Likewise.
> > ---
> >  gcc/cfgcleanup.c  |  2 +-
> >  gcc/config/arc/arc.md | 33 +++--
> >  gcc/config/pa/pa.c|  6 +++---
> >  gcc/config/sh/sh.c|  8 
> >  gcc/emit-rtl.c| 10 +++---
> >  gcc/reorg.c   | 29 +
> >  gcc/resource.c|  2 +-
> >  gcc/rtl.h |  6 +++---
> >  8 files changed, 55 insertions(+), 41 deletions(-)
> > 
> > diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
> > index 023b9d2..2e2a635 100644
> > --- a/gcc/cfgcleanup.c
> > +++ b/gcc/cfgcleanup.c
> > @@ -708,7 +708,7 @@ merge_blocks_move_successor_nojumps (basic_block a, 
> > basic_block b)
> >/* If there is a jump table following block B temporarily add the jump 
> > table
> >   to block B so that it will also be moved to the correct location.  */
> >if (tablejump_p (BB_END (b), , )
> > -  && prev_active_insn (label) == BB_END (b))
> > +  && prev_active_insn (as_a (label)) == BB_END (b))
> Looking at tablejump_p, there seems to be a belief that JUMP_LABEL could be
> a RETURN (as you mentioned in a latter message).  That seems like something
> we should tackle, but I think that can happen independently.
> 
> Presumably if we get a RETURN or SIMPLE_RETURN for LABEL the as_a will fail
> because it's not something that can be converted into an rtx_insn *.

tablejump_p checks for that and only returns an actually label.  Which
is good because as_a would assert now in the caller, but previously it
would inside prev_active_insn.

> 
> 
>  diff --git a/gcc/config/sh/sh.c b/gcc/config/sh/sh.c
> > index b3e949e..a9b5a14 100644
> > --- a/gcc/config/sh/sh.c
> > +++ b/gcc/config/sh/sh.c
> > @@ -5503,7 +5503,8 @@ gen_block_redirect (rtx_insn *jump, int addr, int 
> > need_block)
> > 
> >else if (optimize && need_block >= 0)
> >  {
> > -  rtx_insn *next = next_active_insn (next_active_insn (dest));
> > +  rtx_insn *next = next_active_insn (as_a (dest));
> It may be better to fix how we initialize "dest" to ensure it's an rtx_insn
> *.  Essentially this:
> 
>   rtx dest = XEXP (SET_SRC (PATTERN (jump)), 0);
> 
> turns into:
> 
>   rtx temp = XEXP (SET_SRC (PATTERN (jump)), 0);
>   rtx_insn *dest = as_a (temp));
> 
> But presumably we can't do this until INSN_UID (and perhaps other stuff) is
> changed to accept an rtx_insn *.

actually that part is fine, rtx_insn * can implicitly convert to rtx,
the problem is a few lines down we assign JUMP_LABEL (next) to
dest, which  could certainly be changed, but it seemed better to do the
simple thing for now.

> So that's future work.
> 
> 
> > @@ -2575,7 +2575,8 @@ fill_slots_from_thread (rtx_jump_insn *insn, rtx 
> > condition,
> >  to call update_block and delete_insn.  */
> >   fix_reg_dead_note (prior_insn, insn);
> >   update_reg_unused_notes (prior_insn, new_thread);
> > - new_thread = next_active_insn (new_thread);
> > + new_thread
> > +   = next_active_insn (as_a (new_thread));
> Probably can't avoid the cast for now without other surgery.  Nothing that
> should be terrible, but I don't think you need to pull on the that thread
> (pun intended) for this patch.

heh, that's my impression too.

> 
> In fact, I'm generally OK with adding the casts in reorg.c until fairly late
> in this process :-)
> 
> diff --git a/gcc/resource.c b/gcc/resource.c
> > index ae2f5d8..1d7ce95 100644
> > --- a/gcc/resource.c
> > +++ b/gcc/resource.c
> > @@ -1122,7 +1122,7 @@ mark_target_live_regs (rtx_insn *insns, rtx 
> > target_maybe_return, struct resource
> >rtx_insn *stop_insn = next_active_insn (jump_insn);
> > 
> >if (!ANY_RETURN_P (jump_target))
> > -   jump_target = next_active_insn (jump_target);
> > +   jump_target = next_active_insn (as_a (jump_target));
> Right.  jump_target here can only be an rtx_insn * because of controlling
> 

Re: [PATCH], Use VMRGEW on PowerPC power8/power9 to construct V4SFmode

[Segher Boessenkool]

On Mon, Sep 19, 2016 at 06:51:42PM -0400, Michael Meissner wrote:
> > > However ISA 2.07 (i.e. power8) added the VMRGEW instruction, which can do 
> > > this
> > > more simply:
> > > 
> > > xxpermdi 34,1,2,0
> > > xxpermdi 32,3,4,0
> > > xvcvdpsp 34,34
> > > xvcvdpsp 32,32
> > > vmrgew 2,2,0
> > 
> > This results in {a,c,b,d} instead?
> 
> Yes.

[ snip ]

My question was if you typoed/pastoed/thinkoed it here and you meant

xxpermdi 34,1,3,0
xxpermdi 32,2,4,0
xvcvdpsp 34,34
xvcvdpsp 32,32
vmrgew 2,2,0

which a) works, and b) seems to be what the code generates :-)


Segher

Re: [PATCH 3/8] make next/prev _nonnote_insn take rtx_insn *

[Trevor Saunders]

On Mon, Sep 19, 2016 at 03:23:58PM -0600, Jeff Law wrote:
> On 09/14/2016 01:21 PM, tbsaunde+...@tbsaunde.org wrote:
> > From: Trevor Saunders 
> > 
> > gcc/ChangeLog:
> > 
> > 2016-09-13  Trevor Saunders  
> > 
> > * emit-rtl.c (next_nonnote_insn): Change argument type to
> > rtx_insn *.
> > (prev_nonnote_insn): Likewise.
> > * jump.c (reversed_comparison_code_parts): Likewise.
> > (reversed_comparison): Likewise.
> > * rtl.h: Adjust prototypes.
> > * config/arc/arc.md: Adjust.
> > * cse.c (find_comparison_args): Likewise.
> > * reorg.c (redundant_insn): Change return type to rtx_insn *.
> > (fix_reg_dead_note): Change argument type to rtx_insn *.
> > (delete_prior_computation): Likewise.
> > (delete_computation): Likewise.
> > (fill_slots_from_thread): Adjust.
> > (relax_delay_slots): Likewise.
> > * simplify-rtx.c (simplify_unary_operation_1): Likewise.
> > (simplify_relational_operation_1): Likewise.
> > (simplify_ternary_operation): Likewise.
> The arc bits are similar in nature to the sh bits in how they match on the
> CODE_LABEL rather than on the enclosing LABEL_REF.  I think cleaning up
> those warts can be deferred.
> 
> 
> OK once all prereqs are approved.
> 
> I don't see any more patches in this series in my inbox.  Is that consistent
> with your tracking?

I think the only thing not approved at this point is 1/8, which needs a
fixup that was discussed a little while ago, and some retesting for
that.

Thanks for the reviews!

Trev

> 
> jeff

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

[Segher Boessenkool]

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}.


Segher

Re: [PATCH], Use VMRGEW on PowerPC power8/power9 to construct V4SFmode

[Michael Meissner]

On Mon, Sep 19, 2016 at 05:43:19PM -0500, Segher Boessenkool wrote:
> On Mon, Sep 19, 2016 at 06:02:08PM -0400, Michael Meissner wrote:
> > vector float combine (float a, float b, float c, float d)
> > {
> >   return (vector float) { a, b, c, d };
> > }
> 
> [ ... ]
> 
> > However ISA 2.07 (i.e. power8) added the VMRGEW instruction, which can do 
> > this
> > more simply:
> > 
> > xxpermdi 34,1,2,0
> > xxpermdi 32,3,4,0
> > xvcvdpsp 34,34
> > xvcvdpsp 32,32
> > vmrgew 2,2,0
> 
> This results in {a,c,b,d} instead?

Yes.

> > --- gcc/config/rs6000/rs6000.c  
> > (.../svn+ssh://meiss...@gcc.gnu.org/svn/gcc/trunk/gcc/config/rs6000)
> > (revision 240142)
> > +++ gcc/config/rs6000/rs6000.c  (.../gcc/config/rs6000) (working copy)
> > @@ -6821,11 +6821,26 @@ rs6000_expand_vector_init (rtx target, r
> >   rtx op2 = force_reg (SFmode, XVECEXP (vals, 0, 2));
> >   rtx op3 = force_reg (SFmode, XVECEXP (vals, 0, 3));
> >  
> > - emit_insn (gen_vsx_concat_v2sf (dbl_even, op0, op1));
> > - emit_insn (gen_vsx_concat_v2sf (dbl_odd, op2, op3));
> > - emit_insn (gen_vsx_xvcvdpsp (flt_even, dbl_even));
> > - emit_insn (gen_vsx_xvcvdpsp (flt_odd, dbl_odd));
> > - rs6000_expand_extract_even (target, flt_even, flt_odd);
> > + /* Use VMRGEW if we can instead of doing a permute.  */
> > + if (TARGET_P8_VECTOR)
> > +   {
> > + emit_insn (gen_vsx_concat_v2sf (dbl_even, op0, op2));
> > + emit_insn (gen_vsx_concat_v2sf (dbl_odd, op1, op3));
> 
> But this looks correct, so just the example is pastoed?

Yes, I pasted the code for -mcpu=power7 and -mcpu=power8.  The original code
puts the elements in a different order, and then fixes it up with a permute.  I
changed the order so that it would match how VMRGEW works, and I tested it on
both big and little endian power8's.

The original puts the values as:

+---+---+---+---+
| a | unsued| b | unused|
+---+---+---+---+

+---+---+---+---+
| c | unsued| d | unused|
+---+---+---+---+

The VMRGEW instruction wants the register as:

+---+---+---+---+
| a | unsued| c | unused|
+---+---+---+---+

+---+---+---+---+
| b | unsued| d | unused|
+---+---+---+---+

> Okay for trunk if you can clear that up.

Did that answer the question?

> Thanks,
> 
> 
> Segher
> 

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH], Use VMRGEW on PowerPC power8/power9 to construct V4SFmode

[Segher Boessenkool]

On Mon, Sep 19, 2016 at 06:02:08PM -0400, Michael Meissner wrote:
> vector float combine (float a, float b, float c, float d)
> {
>   return (vector float) { a, b, c, d };
> }

[ ... ]

> However ISA 2.07 (i.e. power8) added the VMRGEW instruction, which can do this
> more simply:
> 
> xxpermdi 34,1,2,0
> xxpermdi 32,3,4,0
> xvcvdpsp 34,34
> xvcvdpsp 32,32
> vmrgew 2,2,0

This results in {a,c,b,d} instead?

> --- gcc/config/rs6000/rs6000.c
> (.../svn+ssh://meiss...@gcc.gnu.org/svn/gcc/trunk/gcc/config/rs6000)
> (revision 240142)
> +++ gcc/config/rs6000/rs6000.c(.../gcc/config/rs6000) (working copy)
> @@ -6821,11 +6821,26 @@ rs6000_expand_vector_init (rtx target, r
> rtx op2 = force_reg (SFmode, XVECEXP (vals, 0, 2));
> rtx op3 = force_reg (SFmode, XVECEXP (vals, 0, 3));
>  
> -   emit_insn (gen_vsx_concat_v2sf (dbl_even, op0, op1));
> -   emit_insn (gen_vsx_concat_v2sf (dbl_odd, op2, op3));
> -   emit_insn (gen_vsx_xvcvdpsp (flt_even, dbl_even));
> -   emit_insn (gen_vsx_xvcvdpsp (flt_odd, dbl_odd));
> -   rs6000_expand_extract_even (target, flt_even, flt_odd);
> +   /* Use VMRGEW if we can instead of doing a permute.  */
> +   if (TARGET_P8_VECTOR)
> + {
> +   emit_insn (gen_vsx_concat_v2sf (dbl_even, op0, op2));
> +   emit_insn (gen_vsx_concat_v2sf (dbl_odd, op1, op3));

But this looks correct, so just the example is pastoed?

Okay for trunk if you can clear that up.

Thanks,


Segher

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

[Trevor Saunders]

On Mon, Sep 19, 2016 at 02:49:49PM -0600, Jeff Law wrote:
> On 09/16/2016 04:10 AM, Trevor Saunders wrote:
> > On Thu, Sep 15, 2016 at 10:27:56AM -0600, Jeff Law wrote:
> > > On 09/15/2016 10:10 AM, Trevor Saunders wrote:
> > > > On Thu, Sep 15, 2016 at 12:52:59PM +0200, Bernd Schmidt wrote:
> > > > > On 09/14/2016 09:21 PM, tbsaunde+...@tbsaunde.org wrote:
> > > > > 
> > > > > > Basically $subject.  First change variable's type to rtx_insn * 
> > > > > > where possible.
> > > > > > Then change the functions and fixup callers where it is still 
> > > > > > necessary to
> > > > > > cast.
> > > > > 
> > > > > #2, #4 and #8 look good and can be applied if they work independently 
> > > > > of the
> > > > > others.
> > > > 
> > > > at most #2 should depend on #1 so it should be fine and I can check on
> > > > the others.
> > > > 
> > > > > Less certain about some of the others which introduce additional 
> > > > > casts.
> > > > 
> > > > yeah, its somewhat unfortunate, though one way or another we will need
> > > > to add casts I think the question is just how many we will accept and
> > > > where.
> > > In my mind the casts represent the "bounds" of how far we've taken this
> > > work.  They occur at the boundaries where we haven't converted something
> > > from an "rtx" to an "rtx_insn *" and allow us to do the work piecemeal
> > > rather than all-at-once.
> > > 
> > > What I don't have a sense of is when we'll be able to push rtx_insn * far
> > > enough that we're able to start removing casts.
> > > 
> > > And that might be a good way to prioritize the next batch of work.  Pick a
> > > cast, remove it and deal with the fallout.  :-)
> > > 
> > > 
> > > > 
> > > > > Maybe LABEL_REF_LABEL needs converting first, and reorg.c has a few
> > > > > variables that might have to be made rtx_insn * in patch #7 to avoid 
> > > > > casts.
> > > > 
> > > > LABEL_REF_LABEL might be doable, its a good idea I'll look into.  The
> > > > reorg.c stuff around target_label is rather complicated unfortunately.
> > > > In the end I of course agree the variables should be rtx_insn *.
> > > > However currently things are assigned to that variable that are not
> > > > insns.  So we need to break the variable up, but its involved in a lot
> > > > of code I don't think I know well enough to really refactor.  For
> > > > example it looks like target_label can hold a value between iterations
> > > > of the loop, I suspect that would be a bug, but I'm not really sure.
> > > I can probably help with reorg.  Hell, you might even be referring to my
> > > code!
> > 
> > ok, going through all the casts added here I see the following reasons
> > for them.
> > 
> > - in md files operands is a array of rtx, we should probably have a
> >   different way to pass insns to the C code here.  That seems worth
> >   investigating incrementally.
> Agreed.  Presumably the exception is passing around something like a
> CODE_LABEL?   Or maybe some hackery with passing around a jump table?

 I took a quick sample of the casts in .md files.  It looks like in most
 if not all cases we have something like label_ref (match_operand 3 ""
 "") then we use operands[3] somewhere that expects a rtx_insn *.  So I
 guess it would make sense to add a match_label and add some magic to
 genrecog to populate a label_operands with the matched labels.

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

> > - tablejump_p returns a label through a rtx * out argument.  I expect
> >   that isn't hard to fix at this point.
> Right.  Seems like a reasonable follow-up.

yeah, I did it locally, so I just need to split out the parts of this
queue we can all agree should go in.

> > 
> > - sh.c uses XEXP (SET_SRC (PATTERN (jump)) 0) with a comment JUMP_LABEL
> >   might be undefined when not optimizing.  Its not clear to me if that
> >   is still true.
> The code in question is nearly 19 years old.  What I think Joern was
> referring to here was that in the old days jump.c was responsible for
> setting JUMP_LABEL and that would only happen when optimizing.
> 
> JUMP_LABEL was a convenient way to find the jump target of an insn.  It
> didn't matter where the label appeared as an operand.  It was also the case
> that we could derive a label, even though the insn might have been an
> indirect jump.
> 
> Anyway...
> 
> So instead of relying on JUMP_LABEL he extracts the destination out of the
> pattern (JUMP_LABEL is field outside the insn's pattern).  That destination
> should be a 

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

[Bernd Edlinger]

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.

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


Thanks
Bernd.

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

[Michael Meissner]

On Mon, Sep 12, 2016 at 04:19:32PM +, Tamar Christina wrote:
> Hi All,
> 
> This patch adds an optimized route to the fpclassify builtin
> for floating point numbers which are similar to IEEE-754 in format.
> 
> The goal is to make it faster by:
> 1. Trying to determine the most common case first
>(e.g. the float is a Normal number) and then the
>rest. The amount of code generated at -O2 are
>about the same +/- 1 instruction, but the code
>is much better.
> 2. Using integer operation in the optimized path.
> 
> At a high level, the optimized path uses integer operations
> to perform the following:
> 
>   if (exponent bits aren't all set or unset)
>  return Normal;
>   else if (no bits are set on the number after masking out
>  sign bits then)
>  return Zero;
>   else if (exponent has no bits set)
>  return Subnormal;
>   else if (mantissa has no bits set)
>  return Infinite;
>   else
>  return NaN;

I haven't looked at fpclassify.  I assume we can define a backend insn to do
the right thing?  One of the things we've noticed over the years with the
PowerPC is that it can be rather expensive to move things from the floating
point/vector unit to the integer registers and vice versa.  This is
particularly true if you having to do the transfer via the memory unit via
stores and loads of different sizes.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797

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

[Jakub Jelinek]

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.

--- gcc/testsuite/g++.old-deja/g++.jason/init3.C.jj 2008-09-05 
12:54:52.0 +0200
+++ gcc/testsuite/g++.old-deja/g++.jason/init3.C2016-09-19 
16:06:01.328022761 +0200
@@ -10,7 +10,7 @@ class My_string {
 public:
My_string(const char* string);
My_string(const My_string &);
-   ~My_string() { delete str; }
+   ~My_string() { delete [] str; }
char* char_p() { return str; }
 };
 

Jakub

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

[Jakub Jelinek]

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?

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.c  2016-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.c  2016-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.c  2016-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

[PATCH], Use VMRGEW on PowerPC power8/power9 to construct V4SFmode

[Michael Meissner]

The original VSX instruction set did not have a simple way to merge the two
vectors with the upper words set in each double word that hold the float values
in V4SFmode format after the XVCVDPSP instructions.  So, the code used a VPERM
instruction to reorder the parts.

vector float combine (float a, float b, float c, float d)
{
  return (vector float) { a, b, c, d };
}

Would generate:

xxpermdi 34,1,2,0
addis 9,2,.LC0@toc@ha
xxpermdi 32,3,4,0
addi 9,9,.LC0@toc@l
lxvw4x 33,0,9
xvcvdpsp 34,34
xvcvdpsp 32,32
vperm 2,2,0,1

# ...

.LC0:
.byte   0
.byte   1
.byte   2
.byte   3
.byte   8
.byte   9
.byte   10
.byte   11
.byte   16
.byte   17
.byte   18
.byte   19
.byte   24
.byte   25
.byte   26
.byte   27

However ISA 2.07 (i.e. power8) added the VMRGEW instruction, which can do this
more simply:

xxpermdi 34,1,2,0
xxpermdi 32,3,4,0
xvcvdpsp 34,34
xvcvdpsp 32,32
vmrgew 2,2,0

I also built Spec 2006 with the compiler, and 4 benchmarks generate the new
sequences (gromacs, dealII, hmmer, and wrf).  I tested gromacs, dealII, hmmer
and I didn't see any changes in execution time.

This patch adds support to use the VMRGEW instruction on ISA 2.07 and above.  I
did bootstrap builds on both big endian power8 and little endian power8 and
there were no regressions.  Is this patch ok to check into the trunk?

2016-09-19  Michael Meissner  

* config/rs6000/rs6000.c (rs6000_expand_vector_init): For V4SF
inits on power8 and above, use the VMRGEW instruction instead of a
permute.

* config/rs6000/altivec.md (UNSPEC_VMRGEW_DIRECT): New unspec.
(p8_vmrgew_v4sf_direct): New VMRGEW insn for V4SF floating
initialization.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/rs6000.c
===
--- gcc/config/rs6000/rs6000.c  
(.../svn+ssh://meiss...@gcc.gnu.org/svn/gcc/trunk/gcc/config/rs6000)
(revision 240142)
+++ gcc/config/rs6000/rs6000.c  (.../gcc/config/rs6000) (working copy)
@@ -6821,11 +6821,26 @@ rs6000_expand_vector_init (rtx target, r
  rtx op2 = force_reg (SFmode, XVECEXP (vals, 0, 2));
  rtx op3 = force_reg (SFmode, XVECEXP (vals, 0, 3));
 
- emit_insn (gen_vsx_concat_v2sf (dbl_even, op0, op1));
- emit_insn (gen_vsx_concat_v2sf (dbl_odd, op2, op3));
- emit_insn (gen_vsx_xvcvdpsp (flt_even, dbl_even));
- emit_insn (gen_vsx_xvcvdpsp (flt_odd, dbl_odd));
- rs6000_expand_extract_even (target, flt_even, flt_odd);
+ /* Use VMRGEW if we can instead of doing a permute.  */
+ if (TARGET_P8_VECTOR)
+   {
+ emit_insn (gen_vsx_concat_v2sf (dbl_even, op0, op2));
+ emit_insn (gen_vsx_concat_v2sf (dbl_odd, op1, op3));
+ emit_insn (gen_vsx_xvcvdpsp (flt_even, dbl_even));
+ emit_insn (gen_vsx_xvcvdpsp (flt_odd, dbl_odd));
+ if (BYTES_BIG_ENDIAN)
+   emit_insn (gen_p8_vmrgew_v4sf_direct (target, flt_even, 
flt_odd));
+ else
+   emit_insn (gen_p8_vmrgew_v4sf_direct (target, flt_odd, 
flt_even));
+   }
+ else
+   {
+ emit_insn (gen_vsx_concat_v2sf (dbl_even, op0, op1));
+ emit_insn (gen_vsx_concat_v2sf (dbl_odd, op2, op3));
+ emit_insn (gen_vsx_xvcvdpsp (flt_even, dbl_even));
+ emit_insn (gen_vsx_xvcvdpsp (flt_odd, dbl_odd));
+ rs6000_expand_extract_even (target, flt_even, flt_odd);
+   }
}
   return;
 }
Index: gcc/config/rs6000/altivec.md
===
--- gcc/config/rs6000/altivec.md
(.../svn+ssh://meiss...@gcc.gnu.org/svn/gcc/trunk/gcc/config/rs6000)
(revision 240142)
+++ gcc/config/rs6000/altivec.md(.../gcc/config/rs6000) (working copy)
@@ -141,6 +141,7 @@ (define_c_enum "unspec"
UNSPEC_VMRGH_DIRECT
UNSPEC_VMRGL_DIRECT
UNSPEC_VSPLT_DIRECT
+   UNSPEC_VMRGEW_DIRECT
UNSPEC_VSUMSWS_DIRECT
UNSPEC_VADDCUQ
UNSPEC_VADDEUQM
@@ -1340,6 +1341,15 @@ (define_insn "p8_vmrgow"
 }
   [(set_attr "type" "vecperm")])
 
+(define_insn "p8_vmrgew_v4sf_direct"
+  [(set (match_operand:V4SF 0 "register_operand" "=v")
+   (unspec:V4SF [(match_operand:V4SF 1 "register_operand" "v")
+ (match_operand:V4SF 2 "register_operand" "v")]
+UNSPEC_VMRGEW_DIRECT))]
+  "TARGET_P8_VECTOR"
+  "vmrgew %0,%1,%2"
+  [(set_attr "type" "vecperm")])
+
 (define_expand "vec_widen_umult_even_v16qi"
   [(use (match_operand:V8HI 0 "register_operand" ""))
(use 

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

[Jakub Jelinek]

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

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

[Jakub Jelinek]

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: [BUILDROBOT] vax-netbsdelf / vax-linux: ‘ELIMINABLE_REGS’ was not declared in this scope

[Jeff Law]

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

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

[Jakub Jelinek]

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: PR35503 - warn for restrict pointer

[Joseph Myers]

On Mon, 19 Sep 2016, David Malcolm wrote:

> But does the new specifier need to be added to c-family/c-format.c, so
> that stage 2 and 3 compilers do know about it?

Yes.

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

patch to fix PR77416

[Vladimir N Makarov]

  The following patch fixes

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77416

  The patch was tested on ppc64 and bootstrapped on x86-64.

Committed as rev. 240247.
Index: ChangeLog
===
--- ChangeLog	(revision 240246)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2016-09-19  Vladimir Makarov  
+
+	PR rtl-optimization/77416
+	* lra-remat.c (operand_to_remat): Process hard coded insn
+	registers.
+
 2016-09-19  Kyrylo Tkachov  
 
 	* simplify-rtx.c (simplify_relational_operation_1): Add transformation
Index: testsuite/ChangeLog
===
--- testsuite/ChangeLog	(revision 240246)
+++ testsuite/ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2016-09-19  Vladimir Makarov  
+
+	PR rtl-optimization/77416
+	* gcc.target/powerpc/pr77416.c: New.
+
 2016-09-19  Patrick Palka  
 
 	PR c++/77639
Index: lra-remat.c
===
--- lra-remat.c	(revision 240246)
+++ lra-remat.c	(working copy)
@@ -370,6 +370,22 @@ operand_to_remat (rtx_insn *insn)
 		   + hard_regno_nregs[reg->regno][reg->biggest_mode])))
 	  return -1;
   }
+  /* Check hard coded insn registers.  */
+  for (struct lra_insn_reg *reg = static_id->hard_regs;
+   reg != NULL;
+   reg = reg->next)
+if (reg->type == OP_INOUT)
+  return -1;
+else if (reg->type == OP_IN)
+  {
+	/* Check that there is no output hard reg as the input
+	   one.  */
+	  for (struct lra_insn_reg *reg2 = static_id->hard_regs;
+	   reg2 != NULL;
+	   reg2 = reg2->next)
+	if (reg2->type == OP_OUT && reg->regno == reg2->regno)
+		return -1;
+  }
   /* Find the rematerialization operand.  */
   int nop = static_id->n_operands;
   for (int i = 0; i < nop; i++)
Index: testsuite/gcc.target/powerpc/pr77416.c
===
--- testsuite/gcc.target/powerpc/pr77416.c	(revision 0)
+++ testsuite/gcc.target/powerpc/pr77416.c	(working copy)
@@ -0,0 +1,34 @@
+/* { dg-do compile { target { powerpc64*-*-*} } } */
+/* { dg-skip-if "" { powerpc64-*-aix* } { "*" } { "" } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc64*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
+/* { dg-options "-mcpu=power7 -O2 -m32" } */
+/* { dg-final { scan-assembler-times "addze" 1 } } */
+
+extern int fn2 ();
+extern void fn3 ();
+extern void fn4 (int);
+int a, c, d, f, g, h, i, j, k, l, m, n;
+struct
+{
+  int escape;
+} *b;
+int e[8];
+void
+fn1 (int p1, int p2)
+{
+  int o = a;
+  for (; f; f++)
+{
+  int p;
+  if (e[h])
+  continue;
+  if (fn2 (o, d, l, n, p1, i, j, k, 0==0))
+  continue;
+  p = p2;
+  if (b[g].escape)
+  p++;
+  fn3 ("", c, m);
+  if (k)
+  fn4 (p);
+}
+}

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

[Joseph Myers]

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.

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

Re: [PATCH 5/9] Introduce class function_reader

[Jeff Law]

On 09/16/2016 03:42 PM, David Malcolm wrote:

On Fri, 2016-09-16 at 15:28 -0600, Jeff Law wrote:

On 09/08/2016 06:30 PM, David Malcolm wrote:

This patch generalizes the RTL-reading capabilities so that they
can be run on the host as well as the build machine.
The available rtx in rtl.def changes dramatically between these
two configurations, so a fair amount of #ifdef GENERATOR_FILE is
required to express this.

This patch introduces a function_reader subclass of rtx_reader,
capable of reading an RTL function dump (or part of one),
reconstructing a cfun with a CFG and basic blocks containing insns.

gcc/ChangeLog:
* Makefile.in (OBJS): Add errors.o, read-md.o, read-rtl.o,
read-rtl-function.o, and selftest-rtl.o.



* cfgexpand.c (pass_expand::execute): Move stack
initializations
to rtl_data::init_stack_alignment and call it.  Pass "true"
for new "emit_insns" param of expand_function_start.
* emit-rtl.c (gen_reg_rtx): Move regno_pointer_align and
regno_reg_rtx resizing logic to...
(emit_status::ensure_regno_capacity): ...this new method.
(init_emit): Allocate regno_reg_rtx using ggc_cleared_vec_alloc
rather than ggc_vec_alloc.
(rtl_data::init_stack_alignment): New method.
(get_insn_by_uid): New function.
* emit-rtl.h (rtl_data::init_stack_alignment): New method.
* errors.c: Use consistent pattern for bconfig.h vs config.h
includes.
(progname): Wrap with #ifdef GENERATOR_FILE.
(error): Likewise.  Add "error: " to message.
(fatal): Likewise.
(internal_error): Likewise.
(trim_filename): Likewise.
(fancy_abort): Likewise.
* errors.h (struct file_location): Move here from read-md.h.
(file_location::file_location): Likewise.
(error_at): New decl.
* function-tests.c (selftest::verify_three_block_rtl_cfg):
Remove
"static".
* function.c (instantiate_decls): Guard call to
instantiate_decls_1 with if (DECL_INITIAL (fndecl)).
(expand_function_start): Add param "emit_insns", and use it to
guard the various gen/emit calls.
* function.h (emit_status::ensure_regno_capacity): New method.
(expand_function_start): Add bool param to decl.
* gensupport.c (gen_reader::gen_reader): Add NULL for new
policy
param of rtx_reader ctor.
* print-rtl.c (print_rtx): Print "(nil)" rather than an empty
string for NULL strings.  Print "(nil)" for NULL basic blocks.
* read-md.c (read_skip_construct): Provide forward decl.
(read_skip_spaces): Support '/'.
(require_char): New function.
(require_word_ws): New function.
(peek_char): New function.
(read_name): Rename to...
(read_name_1): ...this new static function, adding "out_loc"
param,
and converting "missing name or number" to returning false,
rather
than failing.
(read_name): Reimplement in terms of read_name_1.
(read_name_or_nil): New function.
(read_string): Handle "(nil)" by returning NULL.  */
(rtx_reader::rtx_reader): Add rtl_reader_policy * param, using
it to initialize m_policy.
(rtx_reader::~rtx_reader): Free m_base_dir.  Clean up global
data.
* read-md.h (struct file_location): Move to errors.h.
(file_location::file_location): Likewise.
Include errors.h.
(class regno_remapper): New class.
(struct rtl_reader_policy): New struct.
(rtx_reader::rtx_reader): Add rtl_reader_policy * param.
(rtx_reader::add_fixup_insn_uid): New vfunc.
(rtx_reader::add_fixup_bb): New vfunc.
(rtx_reader::add_fixup_note_insn_basic_block): New vfunc.
(rtx_reader::add_fixup_source_location): New vfunc.
(rtx_reader::add_fixup_jump_label): New vfunc.
(rtx_reader::add_fixup_expr): New vfunc.
(rtx_reader::remap_regno): New method.
(rtx_reader::m_policy): New field.
(noop_reader::noop_reader): Add NULL for new policy param of
rtx_reader ctor.
(peek_char): New decl.
(require_char): New decl.
(require_word_ws): New decl.
(read_name): Convert return type from void to file_location.
(read_name_or_nil): New decl.
* read-rtl-function.c: New file.
* read-rtl-function.h: New file.
* read-rtl.c: Potentially include config.h rather than
bconfig.h.
For host, include function.h and emit-rtl.h.
(apply_subst_iterator): Wrap with #ifdef GENERATOR_FILE.
(bind_subst_iter_and_attr): Likewise.
(add_condition_to_string): Likewise.
(add_condition_to_rtx): Likewise.
(apply_attribute_uses): Likewise.
(add_current_iterators): Likewise.
(apply_iterators): Likewise.
(initialize_iterators): Guard usage of apply_subst_iterator
with
#ifdef GENERATOR_FILE.

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

[Jeff Law]

On 09/19/2016 03:32 PM, 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 ?

Yea, that's fine.

I think Tamar & Wilco's work on builtin_classify_fptype is reasonably 
close, so it shouldn't be long before you can get back to those bits.

jeff

Re: [PATCH 8/9] final.c selftests

[Jeff Law]

On 09/16/2016 03:34 PM, David Malcolm wrote:

On Fri, 2016-09-16 at 14:45 -0600, Jeff Law wrote:

On 09/08/2016 06:30 PM, David Malcolm wrote:

gcc/ChangeLog:
* final.c: Include selftest.h and selftest-rtl.h.
(class selftest::temp_asm_out): New subclass of
selftest::named_temp_file.
(selftest::temp_asm_out::temp_asm_out): New ctor.
(selftest::temp_asm_out::~temp_asm_out): New dtor.
(class selftest::asm_out_test): New subclass of
selftest::rtl_dump_test.
(selftest::asm_out_test::asm_out_test): New ctor.
(selftest::test_jump_insn): New function.
(selftest::test_empty_function): New function.
(selftest::test_asm_for_insn): New function.
(TEST_ASM_FOR_INSN): New macro.
(selftest::test_x86_64_leal): New function.
(selftest::test_x86_64_negl): New function.
(selftest::test_x86_64_cmpl): New function.
(selftest::test_x86_64_cmovge): New function.
(selftest::test_x86_64_ret): New function.
(selftest::final_c_tests): New function.
* selftest-run-tests.c (selftest::run_tests): Call
selftest::final_c_tests.
* selftest.h (selftest::final_c_tests): New decl.

I'm really not sure how useful these tests are going to be and would
question the long term maintenance costs of keeping them up-to-date.

I could see perhaps verifying that when there are multiple
alternatives
that the correct one is selected or somesuch, but these tests really
don't seem to be covering anything particularly useful.


My thinking here was that it might be useful to verify insn recognition
and output when someone is bringing up a new target, or adding new
insns to a .md file; the selftest::test_x86_64_cmpl show the beginnings
of how one might write that in selftest form.
Understood.  I'm just not sure how helpful that will be in practice and 
it seems like a maintenance nightmare.


The case where I think something like this is useful is when there's 
multiple ways to express the same insn and there's a preferred form. 
Say because the preferred form is smaller, faster, whatever.


But even that can be incredibly painful.  Consider zero-ing a register 
on the x86.  There's mov 0,reg; xor reg, reg; sub reg,reg, etc.  Which 
is preferred depends on the micro-architecture and possibly surrounding 
context, whether or not the containing block is hot or not, etc.





But I'm happy to drop it.
I don't think it buys us much right now.  It is archived if we end up 
changing our minds.


jeff

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

[Bernd Edlinger]

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 ?


Thanks
Bernd.

Re: [PATCH] Fix PR64078

[Jeff Law]

On 09/19/2016 03:08 PM, Bernd Edlinger wrote:


Would it work to break this up into distinct tests, exit()-ing from each
function rather than returning back to main?



Yes.  I think how this test is designed, each function must be inlined,
or it will fail anyway.  It was for instance impossible to pass the
ubsan test, if -fno-inline was used as RUNTESTFLAGS.
Presumably the dg-skip-if is ensuring that we're only testing with -O2 
turned on.


Therefore it works as well, if main avoids to return and calls
exit(0) instead, with a specific comment of course.

See https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01985.html

That works for me.

jeff

Re: [PATCH] manual: _Bool has trap representations

[Joseph Myers]

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.

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

Re: [PATCH 3/8] make next/prev _nonnote_insn take rtx_insn *

[Jeff Law]

On 09/14/2016 01:21 PM, tbsaunde+...@tbsaunde.org wrote:

From: Trevor Saunders 

gcc/ChangeLog:

2016-09-13  Trevor Saunders  

* emit-rtl.c (next_nonnote_insn): Change argument type to
rtx_insn *.
(prev_nonnote_insn): Likewise.
* jump.c (reversed_comparison_code_parts): Likewise.
(reversed_comparison): Likewise.
* rtl.h: Adjust prototypes.
* config/arc/arc.md: Adjust.
* cse.c (find_comparison_args): Likewise.
* reorg.c (redundant_insn): Change return type to rtx_insn *.
(fix_reg_dead_note): Change argument type to rtx_insn *.
(delete_prior_computation): Likewise.
(delete_computation): Likewise.
(fill_slots_from_thread): Adjust.
(relax_delay_slots): Likewise.
* simplify-rtx.c (simplify_unary_operation_1): Likewise.
(simplify_relational_operation_1): Likewise.
(simplify_ternary_operation): Likewise.
The arc bits are similar in nature to the sh bits in how they match on 
the CODE_LABEL rather than on the enclosing LABEL_REF.  I think cleaning 
up those warts can be deferred.



OK once all prereqs are approved.

I don't see any more patches in this series in my inbox.  Is that 
consistent with your tracking?


jeff

Re: [PATCH 7/8] make next/prev active_insn and active_insn_p take rtx_insn *

[Jeff Law]

On 09/14/2016 01:21 PM, tbsaunde+...@tbsaunde.org wrote:

From: Trevor Saunders 

gcc/ChangeLog:

2016-09-14  Trevor Saunders  

* emit-rtl.c (next_active_insn): Change argument type to
rtx_insn *.
(prev_active_insn): Likewise.
(active_insn_p): Likewise.
* rtl.h: Adjust prototypes.
* cfgcleanup.c (merge_blocks_move_successor_nojumps): Adjust.
* config/arc/arc.md: Likewise.
* config/pa/pa.c (branch_to_delay_slot_p): Likewise.
(branch_needs_nop_p): Likewise.
(use_skip_p): Likewise.
* config/sh/sh.c (gen_block_redirect): Likewise.
(split_branches): Likewise.
* reorg.c (optimize_skip): Likewise.
(fill_simple_delay_slots): Likewise.
(fill_slots_from_thread): Likewise.
(relax_delay_slots): Likewise.
* resource.c (mark_target_live_regs): Likewise.
---
 gcc/cfgcleanup.c  |  2 +-
 gcc/config/arc/arc.md | 33 +++--
 gcc/config/pa/pa.c|  6 +++---
 gcc/config/sh/sh.c|  8 
 gcc/emit-rtl.c| 10 +++---
 gcc/reorg.c   | 29 +
 gcc/resource.c|  2 +-
 gcc/rtl.h |  6 +++---
 8 files changed, 55 insertions(+), 41 deletions(-)

diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
index 023b9d2..2e2a635 100644
--- a/gcc/cfgcleanup.c
+++ b/gcc/cfgcleanup.c
@@ -708,7 +708,7 @@ merge_blocks_move_successor_nojumps (basic_block a, 
basic_block b)
   /* If there is a jump table following block B temporarily add the jump table
  to block B so that it will also be moved to the correct location.  */
   if (tablejump_p (BB_END (b), , )
-  && prev_active_insn (label) == BB_END (b))
+  && prev_active_insn (as_a (label)) == BB_END (b))
Looking at tablejump_p, there seems to be a belief that JUMP_LABEL could 
be a RETURN (as you mentioned in a latter message).  That seems like 
something we should tackle, but I think that can happen independently.


Presumably if we get a RETURN or SIMPLE_RETURN for LABEL the as_a will 
fail because it's not something that can be converted into an rtx_insn *.



 diff --git a/gcc/config/sh/sh.c b/gcc/config/sh/sh.c

index b3e949e..a9b5a14 100644
--- a/gcc/config/sh/sh.c
+++ b/gcc/config/sh/sh.c
@@ -5503,7 +5503,8 @@ gen_block_redirect (rtx_insn *jump, int addr, int 
need_block)

   else if (optimize && need_block >= 0)
 {
-  rtx_insn *next = next_active_insn (next_active_insn (dest));
+  rtx_insn *next = next_active_insn (as_a (dest));
It may be better to fix how we initialize "dest" to ensure it's an 
rtx_insn *.  Essentially this:


  rtx dest = XEXP (SET_SRC (PATTERN (jump)), 0);

turns into:

  rtx temp = XEXP (SET_SRC (PATTERN (jump)), 0);
  rtx_insn *dest = as_a (temp));

But presumably we can't do this until INSN_UID (and perhaps other stuff) 
is changed to accept an rtx_insn *.


So that's future work.



@@ -2575,7 +2575,8 @@ fill_slots_from_thread (rtx_jump_insn *insn, rtx 
condition,
 to call update_block and delete_insn.  */
  fix_reg_dead_note (prior_insn, insn);
  update_reg_unused_notes (prior_insn, new_thread);
- new_thread = next_active_insn (new_thread);
+ new_thread
+   = next_active_insn (as_a (new_thread));
Probably can't avoid the cast for now without other surgery.  Nothing 
that should be terrible, but I don't think you need to pull on the that 
thread (pun intended) for this patch.


In fact, I'm generally OK with adding the casts in reorg.c until fairly 
late in this process :-)


diff --git a/gcc/resource.c b/gcc/resource.c

index ae2f5d8..1d7ce95 100644
--- a/gcc/resource.c
+++ b/gcc/resource.c
@@ -1122,7 +1122,7 @@ mark_target_live_regs (rtx_insn *insns, rtx 
target_maybe_return, struct resource
   rtx_insn *stop_insn = next_active_insn (jump_insn);

   if (!ANY_RETURN_P (jump_target))
-   jump_target = next_active_insn (jump_target);
+   jump_target = next_active_insn (as_a (jump_target));
Right.  jump_target here can only be an rtx_insn * because of 
controlling condition.  But clearly this is something we'll want to 
clean up later.


I think this is fine once prereqs are approved.

jeff

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Sun, 2016-09-18 at 22:46 +0530, Prathamesh Kulkarni wrote:
> On 2 September 2016 at 23:14, David Malcolm 
> wrote:
> > On Thu, 2016-09-01 at 14:55 +0530, Prathamesh Kulkarni wrote:
> > 
> > [...]
> > 
> > > The attached version passes bootstrap+test on ppc64le-linux-gnu.
> > > Given that it only looks if parameters are restrict qualified and
> > > not
> > > how they're used inside the callee,
> > > this can have false positives as in above test-cases.
> > > Should the warning be put in Wextra rather than Wall (I have left
> > > it
> > > in Wall in the patch)  or only enabled with -Wrestrict ?
> > > 
> > > Thanks,
> > > Prathamesh
> > > > 
> > > > Richard.
> > 
> > 
> > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> > index 3feb910..a3dae34 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.  */
> > 
> > @@ -13057,4 +13058,76 @@ 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);
> > +   }
> > +}
> > +
> > +  if (arg_positions.is_empty ())
> > +return;
> > +
> > +  struct obstack fmt_obstack;
> > +  gcc_obstack_init (_obstack);
> > +  char *fmt = (char *) obstack_alloc (_obstack, 0);
> > +
> > +  char num[32];
> > +  sprintf (num, "%u", param_pos + 1);
> > +
> > +  obstack_grow (_obstack, "passing argument ",
> > +   strlen ("passing argument "));
> > +  obstack_grow (_obstack, num, strlen (num));
> > +  obstack_grow (_obstack,
> > +   " to restrict-qualified parameter aliases with
> > argument",
> > +   strlen (" to restrict-qualified parameter "
> > +   "aliases with argument"));
> > +
> > +  /* make argument plural and append space.  */
> > +  if (arg_positions.length () > 1)
> > +obstack_1grow (_obstack, 's');
> > +  obstack_1grow (_obstack, ' ');
> > +
> > +  unsigned pos;
> > +  FOR_EACH_VEC_ELT (arg_positions, i, pos)
> > +{
> > +  tree arg = (*args)[pos];
> > +  if (EXPR_HAS_LOCATION (arg))
> > +   richloc.add_range (EXPR_LOCATION (arg), false);
> > +
> > +  sprintf (num, "%u", pos + 1);
> > +  obstack_grow (_obstack, num, strlen (num));
> > +
> > +  if (i < arg_positions.length () - 1)
> > +   obstack_grow (_obstack, ", ",  strlen (", "));
> > +}
> > +
> > +  obstack_1grow (_obstack, 0);
> > +  warning_at_rich_loc (, OPT_Wrestrict, fmt);
> > +  obstack_free (_obstack, fmt);
> > +}
> > 
> > Thanks for working on this.
> > 
> > I'm not a fan of how the patch builds "fmt" here.  If nothing else,
> > this perhaps should be:
> > 
> >   warning_at_rich_loc (, OPT_Wrestrict, "%s", fmt);
> > 
> > but building up strings like the patch does makes localization
> > difficult.
> > 
> > Much better would be to have the formatting be done inside the
> > diagnostics subsystem's call into pp_format, with something like
> > this:
> > 
> >   warning_at_rich_loc_n (, OPT_Wrestrict,
> >  arg_positions
> > .length (),
> >  "passing argument %i to restrict
> > -qualified"
> >  " parameter aliases with argument
> > %FIXME",
> >  "passing argument %i to restrict
> > -qualified"
> >  " parameter aliases with arguments
> > %FIXME",
> >  param_pos + 1,
> > 
> >  _positions);
> > 
> > 
> > and have %FIXME (or somesuch) consume _positions in the va_arg,
> > printing the argument numbers there.  Doing it this way also avoids
> > building the string for the case where Wrestrict is disabled, since
> > the
> > pp_format only happens after we know we're going to print the
> > warning.
> > 
> > Assuming that there isn't yet a pre-canned way to print a set of
> > argument numbers that I've missed, the place to add the %FIXME
> > 

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Mon, 2016-09-19 at 14:21 -0400, Jason Merrill wrote:
> On Sun, Sep 18, 2016 at 1:16 PM, Prathamesh Kulkarni
>  wrote:
> > Sorry for late response. In the attached patch, I removed obstack
> > building on fmt, and used pp_format for formatting arg_positions by
> > adding specifier %I (name
> > chosen arbitrarily). Does that look OK ?
> > 
> > However it results in following warning during gcc build and am not
> > sure how to fix this:
> > ../../gcc/gcc/c-family/c-common.c: In function ‘void
> > warn_for_restrict(unsigned int, vec*)’:
> > ../../gcc/gcc/c-family/c-common.c:13132:24: warning: unknown
> > conversion type character ‘I’ in format [-Wformat=]
> 
> This warning (in stage 1) happens whenever we add a new specifier,
> don't worry about it.

Right: there's no way the stage 1 compiler could know about a new
specifier.

But does the new specifier need to be added to c-family/c-format.c, so
that stage 2 and 3 compilers do know about it?

Re: [PATCH] Fix PR64078

[Bernd Edlinger]

On 09/19/16 22:19, Jeff Law wrote:
> On 09/15/2016 04:29 AM, Tom de Vries wrote:
>> On 31/08/16 07:42, Tom de Vries wrote:
>>> On 30/08/16 11:38, Bernd Edlinger wrote:
 On 08/30/16 10:21, Tom de Vries wrote:
> On 29/08/16 18:43, Bernd Edlinger wrote:
>> Thanks!
>>
>> Actually my patch missed to fix one combination: -m32 with -fpic
>>
>> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c
>> --tool_opts
>> '-m32 -fpic'"
>>
>> FAIL: c-c++-common/ubsan/object-size-9.c   -O2  execution test
>> FAIL: c-c++-common/ubsan/object-size-9.c   -O2 -flto
>> -fno-use-linker-plugin -flto-partition=none  execution test
>>
>> The problem here is that the functions f2 and f3 access a stack-
>> based object out of bounds and that is inlined in main and
>> therefore smashes the return address of main in this case.
>>
>> A possible fix could look like follows:
>>
>> Index: object-size-9.c
>> ===
>> --- object-size-9.c(revision 239794)
>> +++ object-size-9.c(working copy)
>> @@ -93,5 +93,9 @@
>>   #endif
>> f4 (12);
>> f5 (12);
>> +#ifdef __cplusplus
>> +  /* Stack may be smashed by f2/f3 above.  */
>> +  __builtin_exit (0);
>> +#endif
>> return 0;
>>   }
>>
>>
>> Do you think that this should be fixed too?
>
> I think it should be fixed. Ideally, we'd prevent the out-of-bounds
> writes to have harmful effects, but I'm not sure how to enforce that.
>
> This works for me:
> ...
> diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c
> b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c
> index 46f1fb9..fec920d 100644
> --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c
> +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c
> @@ -31,6 +31,7 @@ static struct C
>   f2 (int i)
>   {
> struct C x;
> +  struct C x2;
> x.d[i] = 'z';
> return x;
>   }
> @@ -45,6 +46,7 @@ static struct C
>   f3 (int i)
>   {
> struct C x;
> +  struct C x2;
> char *p = x.d;
> p += i;
> *p = 'z';
> ...
>
> But I have no idea how stable this solution is.
>

 At least x2 could not be opimized away, as it is no POD,
 but there is no guarantee, that x2 comes after x on the stack.
 Another possibility, which seems to work as well:


 Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c
 ===
 --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c(revision
 239794)
 +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c(working copy)
 @@ -30,7 +30,7 @@ f1 (struct T x, int i)
   static struct C
   f2 (int i)
   {
 -  struct C x;
 +  struct C x __attribute__ ((aligned(16)));
 x.d[i] = 'z';
 return x;
   }
 @@ -44,7 +44,7 @@ f2 (int i)
   static struct C
   f3 (int i)
   {
 -  struct C x;
 +  struct C x __attribute ((aligned(16)));
 char *p = x.d;
 p += i;
 *p = 'z';

>>>
>>> Works for me.
>>
>> OK for trunk, 5 & 6 branch?
>>
>> Thanks,
>> - Tom
>>
>>
>> 0001-Fix-object-size-9.c-with-fpic.patch
>>
>>
>> Fix object-size-9.c with -fpic
>>
>> 2016-09-15  Bernd Edlinger  
>> Tom de Vries  
>>
>> PR testsuite/77411
>> * c-c++-common/ubsan/object-size-9.c (f2, f3): Declare struct C
>> variable
>> with __attribute__((aligned(16))).
> Just so I'm sure on this stuff.
>
> The tests exist to verify that ubsan detects the out-of-bounds writes.
> ubsan isn't terminating the process, so we end up with a smashed stack?
>
> I fail to see how using aligned like this should consistently work.  It
> feels like a hack that just happens to work now.
>
> Would it work to break this up into distinct tests, exit()-ing from each
> function rather than returning back to main?
>

Yes.  I think how this test is designed, each function must be inlined,
or it will fail anyway.  It was for instance impossible to pass the
ubsan test, if -fno-inline was used as RUNTESTFLAGS.

Therefore it works as well, if main avoids to return and calls
exit(0) instead, with a specific comment of course.

See https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01985.html


Bernd.

Re: [PATCH 6/8] make next/prev nonnote_nondebug_insn take rtx_insn *

[Jeff Law]

On 09/14/2016 01:21 PM, tbsaunde+...@tbsaunde.org wrote:

From: Trevor Saunders 

gcc/ChangeLog:

2016-09-14  Trevor Saunders  

* config/cris/cris.c (cris_asm_output_case_end): Change argument
type to rtx_insn *.
* emit-rtl.c (next_nonnote_nondebug_insn): Likewise.
(prev_nonnote_nondebug_insn): Likewise.
* config/cris/cris-protos.h: Adjust prototype.
* rtl.h: Likewise.
* jump.c (rtx_renumbered_equal_p): Adjust.
This is fine once prereqs are approved.  Particularly since you 
indicated LABEL_REF_LABEL may be fixable without major surgery as a 
follow-up.  That presumably would allow removal of the as_a casts in 
rtx_renumbered_equal_p.


jeff

Re: [PATCH 5/8] make prev_real_insn take rtx_insn *

[Jeff Law]

On 09/14/2016 01:21 PM, tbsaunde+...@tbsaunde.org wrote:

From: Trevor Saunders 

gcc/ChangeLog:

2016-09-13  Trevor Saunders  

* emit-rtl.c (prev_real_insn): Change argument type to rtx_insn *.
* rtl.h: Adjust prototype.
* config/sh/sh.md: Adjust.
* dwarf2out.c (add_var_loc_to_decl): Likewise.
---
 gcc/config/sh/sh.md | 3 ++-
 gcc/dwarf2out.c | 2 +-
 gcc/emit-rtl.c  | 4 +---
 gcc/rtl.h   | 2 +-
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
index edc4d15..25e03ef 100644
--- a/gcc/config/sh/sh.md
+++ b/gcc/config/sh/sh.md
@@ -7178,7 +7178,8 @@
  (label_ref (match_operand 1 "" ""
(use (label_ref (match_operand 2 "" "")))]
   "TARGET_SH2
-   && (! INSN_UID (operands[1]) || prev_real_insn (operands[1]) == insn)"
+   && (! INSN_UID (operands[1])
+   || prev_real_insn (as_a (operands[1])) == insn)"
OK, this is the "we passed an INSN in the operands field" fallout. 
Note how operand 1 is enclosed inside a (label_ref rtx)...


One way to deal with this would be to match the label_ref itself rather 
than the enclosed code_label.  That'd require changing the casesi 
expander, but hopefully not much else.


But that can (IMHO), be a follow-up.


   "braf   %0%#"
   [(set_attr "needs_delay_slot" "yes")
(set_attr "type" "jump_ind")])
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 45eb684..fb8ec7d 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -5408,7 +5408,7 @@ add_var_loc_to_decl (tree decl, rtx loc_note, const char 
*label)
   && NOTE_VAR_LOCATION_LOC (temp->first->loc)
   && GET_CODE (NOTE_VAR_LOCATION_LOC (temp->first->loc))
 == GET_CODE (DECL_INCOMING_RTL (decl))
-  && prev_real_insn (temp->first->loc) == NULL_RTX
+  && prev_real_insn (as_a (temp->first->loc)) == NULL_RTX
Hmm...  Hmmm.  I would think ->loc should always be a 
NOTE_INSN_VAR_LOCATION here.  Ahhh, and indeed look up a couple lines in 
the context, we only get here if NOTE_VAR_LOCATION_LOC 
(temp->first->loc) :-)



So this is OK once the prereqs have gone in.

jeff

Re: [PATCH] Handle VECTOR_CST in integer_nonzerop()

[Patrick Palka]

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.

> 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: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/19/16 21:17, Jason Merrill wrote:
> On 09/15/2016 05:19 AM, Bernd Edlinger wrote:
>> +  if (warn_int_in_bool_context)
>> +{
>> +  tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
>> +  tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
>> +  if (TREE_CODE (val1) == INTEGER_CST
>> +  && TREE_CODE (val2) == INTEGER_CST
>> +  && !integer_zerop (val1)
>> +  && !integer_zerop (val2)
>> +  && (!integer_onep (val1)
>> +  || !integer_onep (val2)))
>> +warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
>> +"?: using integer constants in boolean context");
>
> I think this message could be more helpful; we're warning about code
> that always evaluates to 'true', so we could say something like
> "conditional expression always evaluates to true".  In which case maybe
> your original warning name was better.
>


Yes.  It is difficult to choose a good name.

I would also like to warn on signed_int shift left in boolean context,
as a follow-up, because it found also a wrong code in
gcc/cp/parser.c (cp_parser_condition) where we have:

bool flags = LOOKUP_ONLYCONVERTING;

but LOOKUP_ONLYCONVERTING is (1 << 2)

This has nothing to do with conditions,
but just with undefined integer operations in boolean context.

So I thought, these warnings would have something in common.


> I'm also not sure we need to check integer_onep at all before giving
> that warning.  Were you seeing a lot of false positives without that check?
>


I did not try.  This warning is trying to identify code which is
doing funny things because one of the conditional expressions are not
boolean, thus not 0 or 1.  That can happen because either brackets
are there, but at the wrong place, or brackets are not there and the
precedence of ?: and && or || result in wrong code.  Chances are that
"? 2 : 3" is not right in a boolean context.  The fact that the
result used as a boolean is always true, adds just another point to
the score.  So we have a two reasons why we would warn here,
that justifies -Wall, otherwise only -Wextra I would say.

My reasoning was that if both sides evaluate to 1 or both evaluate to
0, it may well be possible that we have different defines here, maybe
just configure options that are only both 1 by chance.

However there is PR 64279 which should handle exactly this:
[Bug c/64279] Warning missing for "(cond) ? A : A" / if(cond) expr1; 
else expr1; // same expression in if and else branch

I think Marek is already working on it.


Thanks
Bernd.

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

[Jeff Law]

On 09/16/2016 04:10 AM, Trevor Saunders wrote:

On Thu, Sep 15, 2016 at 10:27:56AM -0600, Jeff Law wrote:

On 09/15/2016 10:10 AM, Trevor Saunders wrote:

On Thu, Sep 15, 2016 at 12:52:59PM +0200, Bernd Schmidt wrote:

On 09/14/2016 09:21 PM, tbsaunde+...@tbsaunde.org wrote:


Basically $subject.  First change variable's type to rtx_insn * where possible.
Then change the functions and fixup callers where it is still necessary to
cast.


#2, #4 and #8 look good and can be applied if they work independently of the
others.


at most #2 should depend on #1 so it should be fine and I can check on
the others.


Less certain about some of the others which introduce additional casts.


yeah, its somewhat unfortunate, though one way or another we will need
to add casts I think the question is just how many we will accept and
where.

In my mind the casts represent the "bounds" of how far we've taken this
work.  They occur at the boundaries where we haven't converted something
from an "rtx" to an "rtx_insn *" and allow us to do the work piecemeal
rather than all-at-once.

What I don't have a sense of is when we'll be able to push rtx_insn * far
enough that we're able to start removing casts.

And that might be a good way to prioritize the next batch of work.  Pick a
cast, remove it and deal with the fallout.  :-)





Maybe LABEL_REF_LABEL needs converting first, and reorg.c has a few
variables that might have to be made rtx_insn * in patch #7 to avoid casts.


LABEL_REF_LABEL might be doable, its a good idea I'll look into.  The
reorg.c stuff around target_label is rather complicated unfortunately.
In the end I of course agree the variables should be rtx_insn *.
However currently things are assigned to that variable that are not
insns.  So we need to break the variable up, but its involved in a lot
of code I don't think I know well enough to really refactor.  For
example it looks like target_label can hold a value between iterations
of the loop, I suspect that would be a bug, but I'm not really sure.

I can probably help with reorg.  Hell, you might even be referring to my
code!


ok, going through all the casts added here I see the following reasons
for them.

- in md files operands is a array of rtx, we should probably have a
  different way to pass insns to the C code here.  That seems worth
  investigating incrementally.
Agreed.  Presumably the exception is passing around something like a 
CODE_LABEL?   Or maybe some hackery with passing around a jump table?






- 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.




- tablejump_p returns a label through a rtx * out argument.  I expect
  that isn't hard to fix at this point.

Right.  Seems like a reasonable follow-up.



- sh.c uses XEXP (SET_SRC (PATTERN (jump)) 0) with a comment JUMP_LABEL
  might be undefined when not optimizing.  Its not clear to me if that
  is still true.
The code in question is nearly 19 years old.  What I think Joern was 
referring to here was that in the old days jump.c was responsible for 
setting JUMP_LABEL and that would only happen when optimizing.


JUMP_LABEL was a convenient way to find the jump target of an insn.  It 
didn't matter where the label appeared as an operand.  It was also the 
case that we could derive a label, even though the insn might have been 
an indirect jump.


Anyway...

So instead of relying on JUMP_LABEL he extracts the destination out of 
the pattern (JUMP_LABEL is field outside the insn's pattern).  That 
destination should be a LABEL_REF.  Operand 0 of a LABEL_REF is its 
associated CODE_LABEL.  In the sh.c case, he happens to know precisely 
where the LABEL_REF will be inside the insn's pattern.


This points out one of the little hairballs we're going to want to sort 
out.  Essentially XEXP (X, 0) where X is a LABEL_REF needs to be an 
rtx_insn *.  Right now it's an rtx.




- var_loc_node::loc is either an expr list or a note, off hand I'm not
  sure what to do with this.

Depending on how it's set we may just want to break this into two fields.



- in reorg.c variables refer to both a insn and other things I think
  this is more results of JUMP_LABEL some times being a return.
I'd probably want to sit down with a debugger or with a more detailed 
description of when this happens.  I wouldn't be surprised if this was 
just a convenient way of marking jumps which we were later going to turn 
into simple returns.




I've locally converted LABEL_REF_LABEL, but that only avoids 2 casts.
However it does seem like an improvement so I'll send that shortly.

Sounds good.

jeff

Re: Ping Re: Define TS 18661-1 CR_DECIMAL_DIG in

[Jeff Law]

On 09/15/2016 06:56 AM, Joseph Myers wrote:

Ping.  This patch
 is pending
review.
I suspect you know more about this than anyone else here, so ISTM you're 
the authority on correctness.


Rubber stamp OK. :-)

jeff

Re: [PATCH] Fix PR64078

[Jeff Law]

On 09/15/2016 04:29 AM, Tom de Vries wrote:

On 31/08/16 07:42, Tom de Vries wrote:

On 30/08/16 11:38, Bernd Edlinger wrote:

On 08/30/16 10:21, Tom de Vries wrote:

On 29/08/16 18:43, Bernd Edlinger wrote:

Thanks!

Actually my patch missed to fix one combination: -m32 with -fpic

make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts
'-m32 -fpic'"

FAIL: c-c++-common/ubsan/object-size-9.c   -O2  execution test
FAIL: c-c++-common/ubsan/object-size-9.c   -O2 -flto
-fno-use-linker-plugin -flto-partition=none  execution test

The problem here is that the functions f2 and f3 access a stack-
based object out of bounds and that is inlined in main and
therefore smashes the return address of main in this case.

A possible fix could look like follows:

Index: object-size-9.c
===
--- object-size-9.c(revision 239794)
+++ object-size-9.c(working copy)
@@ -93,5 +93,9 @@
  #endif
f4 (12);
f5 (12);
+#ifdef __cplusplus
+  /* Stack may be smashed by f2/f3 above.  */
+  __builtin_exit (0);
+#endif
return 0;
  }


Do you think that this should be fixed too?


I think it should be fixed. Ideally, we'd prevent the out-of-bounds
writes to have harmful effects, but I'm not sure how to enforce that.

This works for me:
...
diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c
b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c
index 46f1fb9..fec920d 100644
--- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c
+++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c
@@ -31,6 +31,7 @@ static struct C
  f2 (int i)
  {
struct C x;
+  struct C x2;
x.d[i] = 'z';
return x;
  }
@@ -45,6 +46,7 @@ static struct C
  f3 (int i)
  {
struct C x;
+  struct C x2;
char *p = x.d;
p += i;
*p = 'z';
...

But I have no idea how stable this solution is.



At least x2 could not be opimized away, as it is no POD,
but there is no guarantee, that x2 comes after x on the stack.
Another possibility, which seems to work as well:


Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c
===
--- gcc/testsuite/c-c++-common/ubsan/object-size-9.c(revision
239794)
+++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c(working copy)
@@ -30,7 +30,7 @@ f1 (struct T x, int i)
  static struct C
  f2 (int i)
  {
-  struct C x;
+  struct C x __attribute__ ((aligned(16)));
x.d[i] = 'z';
return x;
  }
@@ -44,7 +44,7 @@ f2 (int i)
  static struct C
  f3 (int i)
  {
-  struct C x;
+  struct C x __attribute ((aligned(16)));
char *p = x.d;
p += i;
*p = 'z';



Works for me.


OK for trunk, 5 & 6 branch?

Thanks,
- Tom


0001-Fix-object-size-9.c-with-fpic.patch


Fix object-size-9.c with -fpic

2016-09-15  Bernd Edlinger  
Tom de Vries  

PR testsuite/77411
* c-c++-common/ubsan/object-size-9.c (f2, f3): Declare struct C variable
with __attribute__((aligned(16))).

Just so I'm sure on this stuff.

The tests exist to verify that ubsan detects the out-of-bounds writes. 
ubsan isn't terminating the process, so we end up with a smashed stack?


I fail to see how using aligned like this should consistently work.  It 
feels like a hack that just happens to work now.


Would it work to break this up into distinct tests, exit()-ing from each 
function rather than returning back to main?


jeff

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

[Tim Shen]

On Mon, Sep 19, 2016 at 5:34 AM, Jonathan Wakely  wrote:
> As noted in bugzilla PR 77641 I don't think is_literal_type is the
> right condition for _Uninitialized, because a type can have a
> non-trivial default constructor but still be literal, but that means
> it can't be used in the union in _Variant_storage.
>
> I'm not sure if the right condition is is_literal &&
> is_trivially_default_constructible, or if this is enough:

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.

>
> PR libstdc++/77641
> * include/std/variant (__detail::__variant::_Uninitialized): Check
> is_trivially_default_constructible_v instead of is_literal_type_v.
> * testsuite/20_util/variant/compile.cc: Test literal type with
> non-trivial default constructor.
>
> Tim, are there case that this doesn't handle, where we need is_literal
> as well? (bear in mind that is_trivially_default_constructible also
> depends on trivially destructible).

I checked for is_default_constructible, and all other sites are appropriate.


-- 
Regards,
Tim Shen

Re: debug container mutex association

[François Dumont]

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 ?

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.


François

diff --git a/libstdc++-v3/include/debug/bitset b/libstdc++-v3/include/debug/bitset
index 55d3281..b7bada3 100644
--- a/libstdc++-v3/include/debug/bitset
+++ b/libstdc++-v3/include/debug/bitset
@@ -66,8 +66,7 @@ namespace __debug
 	friend class bitset;
 	reference();
 
-	reference(const _Base_ref& __base,
-		  bitset* __seq __attribute__((__unused__))) _GLIBCXX_NOEXCEPT
+	reference(const _Base_ref& __base, bitset* __seq) _GLIBCXX_NOEXCEPT
 	: _Base_ref(__base)
 	, _Safe_iterator_base(__seq, false)
 	{ }
@@ -81,7 +80,7 @@ namespace __debug
 	reference&
 	operator=(bool __x) _GLIBCXX_NOEXCEPT
 	{
-	  _GLIBCXX_DEBUG_VERIFY(! this->_M_singular(),
+	  _GLIBCXX_DEBUG_VERIFY(!this->_M_singular(),
 			  _M_message(__gnu_debug::__msg_bad_bitset_write)
 ._M_iterator(*this));
 	  *static_cast<_Base_ref*>(this) = __x;
@@ -91,10 +90,10 @@ namespace __debug
 	reference&
 	operator=(const reference& __x) _GLIBCXX_NOEXCEPT
 	{
-	  _GLIBCXX_DEBUG_VERIFY(! __x._M_singular(),
+	  _GLIBCXX_DEBUG_VERIFY(!__x._M_singular(),
 			   _M_message(__gnu_debug::__msg_bad_bitset_read)
 ._M_iterator(__x));
-	  _GLIBCXX_DEBUG_VERIFY(! this->_M_singular(),
+	  _GLIBCXX_DEBUG_VERIFY(!this->_M_singular(),
 			  _M_message(__gnu_debug::__msg_bad_bitset_write)
 ._M_iterator(*this));
 	  *static_cast<_Base_ref*>(this) = __x;
@@ -104,7 +103,7 @@ namespace __debug
 	bool
 	operator~() const _GLIBCXX_NOEXCEPT
 	{
-	  

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

[Jeff Law]

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?


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



jeff

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

[Jason Merrill]

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

+  if (warn_int_in_bool_context)
+   {
+ tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
+ tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
+ if (TREE_CODE (val1) == INTEGER_CST
+ && TREE_CODE (val2) == INTEGER_CST
+ && !integer_zerop (val1)
+ && !integer_zerop (val2)
+ && (!integer_onep (val1)
+ || !integer_onep (val2)))
+   warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+   "?: using integer constants in boolean context");


I think this message could be more helpful; we're warning about code 
that always evaluates to 'true', so we could say something like 
"conditional expression always evaluates to true".  In which case maybe 
your original warning name was better.


I'm also not sure we need to check integer_onep at all before giving 
that warning.  Were you seeing a lot of false positives without that check?


Jason

Re: C/C++ PATCH to implement -Wpointer-compare warning (PR c++/64767)

[Jason Merrill]

On Thu, Sep 15, 2016 at 8:16 AM, Marek Polacek  wrote:
> On Wed, Sep 14, 2016 at 12:07:49AM -0400, Jason Merrill wrote:
>> On Sat, Sep 10, 2016 at 10:58 AM, Marek Polacek  wrote:
>> > Spurred by the recent  findings, I 
>> > decided to
>> > implement a warning that warns when a pointer is compared with a zero 
>> > character
>> > literal (constant), because this isn't likely to be intended.  So e.g.
>> >
>> >   ptr == L'\0'
>> >
>> > is probably wrong and should've been written as
>> >
>> >   ptr[0] == L'\0'
>> >
>> > Jonathan pointed out that this would actually be invalid C++11 since 
>> > pointer
>> > conversions are only allowed for integer literals, not char literals.
>>
>> Ah, indeed.  And if we fix that, we get an error rather than a
>> warning.  Maybe let's handle this by wrapping character literals in a
>> redundant NOP_EXPR?
>
> So I've tried.  Wrapping character literals in a NOP_EXPR would make us
> error on that comparison (good), but it breaks -Wchar-subscripts, which
> could be solved by adding STRIP_NOPS, but unfortunately it breaks even
> -Wmemset-transposed-args: '\0' would become (char) '\0', which is not a
> literal zero anymore.  And if I do sth like
> +   if (TREE_CODE (arg2) == NOP_EXPR
> +   && TREE_TYPE (arg2) == TREE_TYPE (TREE_OPERAND (arg2, 0)))
> + arg2 = TREE_OPERAND (arg2, 0);
> then (int) 0 would became a literal zero and we'd warn when not appropriate.
>
> We should also error for e.g. void *p = '\0'; but the problem with the
> NOP_EXPR cast is that convert_for_init strips nops, so we wouldn't reject
> this code.

Good point.

> So it seems we may need some CHARACTER_CST or somesuch?

I suppose that an INTEGER_CST of character type is necessarily a
character constant, so adding a check for !char_type_p ought to do the
trick.

> Note that we should also take boolean-literals into account.

We already check for INTEGER_TYPE, so boolean literals should already
be excluded.

Jason

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

[Jason Merrill]

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.



+ 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.



+ && warning_at (location, OPT_Wbool_operation,
+"%<~%> on a boolean expression"))


And let's refer to "expression of type bool" rather than "boolean 
expression".


Jason

Re: [C++ PATCH] Fix constexpr switch handling (PR c++/77467)

[Jason Merrill]

On Fri, Sep 16, 2016 at 4:44 PM, Jakub Jelinek  wrote:
> On Fri, Sep 16, 2016 at 03:51:11PM -0400, Jason Merrill wrote:
>> On Mon, Sep 5, 2016 at 1:11 PM, Jakub Jelinek  wrote:
>> > +  /* If body is a statement other than STATEMENT_LIST or BIND_EXPR,
>> > + it should be skipped.  E.g. switch (a) b = a;  */
>> > +  if (TREE_CODE (body) == STATEMENT_LIST
>> > +  || TREE_CODE (body) == BIND_EXPR)
>>
>> I'm nervous about this optimization for useless code breaking other
>> things that might (one day) wrap a case label; I think I'd prefer to
>> drop the condition.
>
> By droping the condition you mean unconditionally call
>   cxx_eval_constant_expression (ctx, body, false,
> non_constant_p, overflow_p, jump_target);
> ?  That is known not to work, that breaks the
> +constexpr int
> +bar (int x)
> +{
> +  int a = x;
> +  switch (x)
> +a = x + 1;
> +  return a;
> +}
> handling in the testcase, where body is the MODIFY_EXPR which doesn't have
> the label and thus needs to be skipped.  The problem is that all the logic for
> skipping statements until the label is found is in cxx_eval_statement_list
> only.

Ah, right.

> 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 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?

Jason

Re: [PATCH, wwwdocs, committed] Update gcc-7/changes.html for LRA

[Segher Boessenkool]

On Mon, Sep 19, 2016 at 01:49:57PM -0400, Eric Gallager wrote:
> Perhaps add a link to the wiki page you created about the transition
> to LRA, for easy access?

Like this.  Thanks for the suggestion, committed.


Segher


Index: htdocs/gcc-7/changes.html
===
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-7/changes.html,v
retrieving revision 1.13
diff -U 3 -r1.13 changes.html
--- htdocs/gcc-7/changes.html   19 Sep 2016 16:27:03 -  1.13
+++ htdocs/gcc-7/changes.html   19 Sep 2016 18:26:18 -
@@ -21,7 +21,8 @@
 
 Caveats
 
-  GCC now uses LRA by default for new targets.
+  GCC now uses https://gcc.gnu.org/wiki/LRAIsDefault;>LRA by
+  default for new targets.
 
   The non-standard C++0x type traits
   has_trivial_default_constructor,

Re: PR35503 - warn for restrict pointer

[Jason Merrill]

On Sun, Sep 18, 2016 at 1:16 PM, Prathamesh Kulkarni
 wrote:
> Sorry for late response. In the attached patch, I removed obstack
> building on fmt, and used pp_format for formatting arg_positions by adding 
> specifier %I (name
> chosen arbitrarily). Does that look OK ?
>
> However it results in following warning during gcc build and am not
> sure how to fix this:
> ../../gcc/gcc/c-family/c-common.c: In function ‘void
> warn_for_restrict(unsigned int, vec*)’:
> ../../gcc/gcc/c-family/c-common.c:13132:24: warning: unknown
> conversion type character ‘I’ in format [-Wformat=]

This warning (in stage 1) happens whenever we add a new specifier,
don't worry about it.

+ FOR_EACH_VEC_SAFE_ELT (args, i, arg)
+  TREE_VISITED (arg) = 0;

Hmm, so you're clearing TREE_VISITED before you check the arguments
and leaving it set afterward?  That seems like a problem, a lot of
code assumes that TREE_VISITED has been cleared already and clears it
after.

Jason

Re: [PATCH, wwwdocs, committed] Update gcc-7/changes.html for LRA

[Eric Gallager]

On 9/19/16, Segher Boessenkool  wrote:
> Hi!
>
> I'm committing the following for changes-7.html (and actually ran the
> w3c validator thing this time ;-) )
>
>
> Segher
>
>
> Index: htdocs/gcc-7/changes.html
> ===
> RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-7/changes.html,v
> retrieving revision 1.12
> diff -U 3 -r1.12 changes.html
> --- htdocs/gcc-7/changes.html 26 Aug 2016 18:08:49 -  1.12
> +++ htdocs/gcc-7/changes.html 19 Sep 2016 16:25:14 -
> @@ -21,6 +21,8 @@
>  
>  Caveats
>  
> +  GCC now uses LRA by default for new targets.
> +
>The non-standard C++0x type traits
>has_trivial_default_constructor,
>has_trivial_copy_constructor and
> @@ -265,7 +267,10 @@
>
>  
>
> -
> +PowerPC / PowerPC64 / RS6000
> +
> +  The PowerPC port now uses LRA by default.
> +
>
>  
>
>


Perhaps add a link to the wiki page you created about the transition
to LRA, for easy access?


Eric

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

[Jonathan Wakely]

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.

Using a cast is clear and (AFAIK) works with all versions.

Re: [PATCH 0/9] RFC: selftests based on RTL dumps

[Jeff Law]

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.




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: [PATCH] More trivial bits from early LTO debug merge

[Jason Merrill]

OK.

On Mon, Sep 19, 2016 at 3:53 AM, Richard Biener  wrote:
>
> This merges moving filename and related CU annotation from late finish
> to early finish.  With this all changes to cgraphunit.c have been merged.
>
> [LTO] Bootstrap and test running on x86_64-unknown-linux-gnu.
>
> Richard.
>
> 2016-09-19  Richard Biener  
>
> * debug.h (gcc_debug_hooks): Add filename parameter to early_finish
> hook.
> * debug.c (do_nothing_debug_hooks): Adjust.
> * dbxout.c (dbx_debug_hooks): Likewise.
> * sdbout.c (sdb_debug_hooks): Likewise.
> * dwarf2out.c (dwarf2_lineno_debug_hooks): Likewise.
> (dwarf2out_finish): Move producer, filename and
> path annotation ...
> (dwarf2out_early_finish): ... here.  Remove in_lto_p special-casing.
> * cgraphunit.c (symbol_table::finalize_compilation_unit): Adjust.
>
> lto/
> * lto.c (lto_main): Call early_finish with "" as
> filename.
>
> Index: gcc/debug.h
> ===
> --- gcc/debug.h (revision 240228)
> +++ gcc/debug.h (working copy)
> @@ -31,7 +31,7 @@ struct gcc_debug_hooks
>void (* finish) (const char *main_filename);
>
>/* Run cleanups necessary after early debug generation.  */
> -  void (* early_finish) (void);
> +  void (* early_finish) (const char *main_filename);
>
>/* Called from cgraph_optimize before starting to assemble
>   functions/variables/toplevel asms.  */
> Index: gcc/debug.c
> ===
> --- gcc/debug.c (revision 240228)
> +++ gcc/debug.c (working copy)
> @@ -26,7 +26,7 @@ const struct gcc_debug_hooks do_nothing_
>  {
>debug_nothing_charstar,
>debug_nothing_charstar,
> -  debug_nothing_void,  /* early_finish */
> +  debug_nothing_charstar,  /* early_finish */
>debug_nothing_void,
>debug_nothing_int_charstar,
>debug_nothing_int_charstar,
> Index: gcc/dbxout.c
> ===
> --- gcc/dbxout.c(revision 240228)
> +++ gcc/dbxout.c(working copy)
> @@ -344,7 +344,7 @@ const struct gcc_debug_hooks dbx_debug_h
>  {
>dbxout_init,
>dbxout_finish,
> -  debug_nothing_void,
> +  debug_nothing_charstar,
>debug_nothing_void,
>debug_nothing_int_charstar,
>debug_nothing_int_charstar,
> Index: gcc/sdbout.c
> ===
> --- gcc/sdbout.c(revision 240228)
> +++ gcc/sdbout.c(working copy)
> @@ -277,7 +277,7 @@ const struct gcc_debug_hooks sdb_debug_h
>  {
>sdbout_init,  /* init */
>sdbout_finish,/* finish */
> -  debug_nothing_void,   /* early_finish */
> +  debug_nothing_charstar,   /* early_finish */
>debug_nothing_void,   /* assembly_start */
>debug_nothing_int_charstar,   /* define */
>debug_nothing_int_charstar,   /* undef */
> Index: gcc/cgraphunit.c
> ===
> --- gcc/cgraphunit.c(revision 240228)
> +++ gcc/cgraphunit.c(working copy)
> @@ -2561,7 +2564,7 @@ symbol_table::finalize_compilation_unit
>
>/* Clean up anything that needs cleaning up after initial debug
>  generation.  */
> -  (*debug_hooks->early_finish) ();
> +  (*debug_hooks->early_finish) (main_input_filename);
>  }
>
>/* Finally drive the pass manager.  */
> Index: gcc/lto/lto.c
> ===
> --- gcc/lto/lto.c   (revision 240228)
> +++ gcc/lto/lto.c   (working copy)
> @@ -3316,7 +3316,7 @@ lto_main (void)
> lto_promote_statics_nonwpa ();
>
>   /* Annotate the CU DIE and mark the early debug phase as finished.  
> */
> - debug_hooks->early_finish ();
> + debug_hooks->early_finish ("");
>
>   /* Let the middle end know that we have read and merged all of
>  the input files.  */
> Index: gcc/dwarf2out.c
> ===
> --- gcc/dwarf2out.c (revision 240228)
> +++ gcc/dwarf2out.c (working copy)
> @@ -2480,7 +2480,7 @@ build_cfa_aligned_loc (dw_cfa_location *
>
>  static void dwarf2out_init (const char *);
>  static void dwarf2out_finish (const char *);
> -static void dwarf2out_early_finish (void);
> +static void dwarf2out_early_finish (const char *);
>  static void dwarf2out_assembly_start (void);
>  static void dwarf2out_define (unsigned int, const char *);
>  static void dwarf2out_undef (unsigned int, const char *);
> @@ -2556,7 +2556,7 @@ const struct gcc_debug_hooks dwarf2_line
>  {
>dwarf2out_init,
>debug_nothing_charstar,
> -  debug_nothing_void,
> +  debug_nothing_charstar,
>debug_nothing_void,
>

Re: [PATCH 9/9] cse.c selftests

[Jeff Law]

On 09/16/2016 03:19 PM, David Malcolm wrote:



When possible I don't think we want the tests to be target specific.
Hmm, I'm probably about to argue for Bernd's work...  The 71779
testcase
is a great example -- it uses high/lo_sum.  Not all targets support
that
-- as long as we don't try to recognize those insns we're likely OK,
but
that seems fragile long term.  Having an idealized target means we
can
ignore much of these issues.


An alternative would be to pick a specific target for each test.
It's an alternative, but not one I particularly like since those tests 
won't be consistently run.  With an abstracted target like Bernd 
suggests we ought to be able to make most tests work with the abstracted 
target and minimize the number of truely target specific tests.




So I'm real curious, what happens if you run this RTL selftest under
valgrind?  I have the sneaking suspicion that we'll start doing some
uninitialized memory reads.


I'm seeing various leaks (an htab within linemap_init for all of the
line_table fixtures), but no uninitialized reads.

Wow.  I must say I'm surprised.  Good news though.



  +

+  /* Dump taken from comment 2 of PR 71779, of
+ "...the relevant memory access coming out of expand"
+ with basic block IDs added, and prev/next insns set to
+ 0 at ends.  */
+  const char *input_dump
+= (";; MEM[(struct isl_obj *)] = _obj_map_vtable;\n"
+   "(insn 1045 0 1046 2 (set (reg:SI 480)\n"
+   "(high:SI (symbol_ref:SI (\"isl_obj_map_vtable\")
[flags 0xc0] )))
y.c:12702 -1\n"
+   " (nil))\n"
+   "(insn 1046 1045 1047 2 (set (reg/f:SI 479)\n"
+   "(lo_sum:SI (reg:SI 480)\n"
+   "(symbol_ref:SI (\"isl_obj_map_vtable\") [flags
0xc0] ))) y.c:12702
-1\n"
+   " (expr_list:REG_EQUAL (symbol_ref:SI
(\"isl_obj_map_vtable\") [flags 0xc0] )\n"
+   "(nil)))\n"
+   "(insn 1047 1046 1048 2 (set (reg:DI 481)\n"
+   "(subreg:DI (reg/f:SI 479) 0)) y.c:12702 -1\n"
+   " (nil))\n"
+   "(insn 1048 1047 1049 2 (set (zero_extract:DI (reg/v:DI 191
[ obj1D.17368 ])\n"
+   "(const_int 32 [0x20])\n"
+   "(const_int 0 [0]))\n"
+   "(reg:DI 481)) y.c:12702 -1\n"
+   " (nil))\n"

So looking at this just makes my head hurt.  Escaping, lots of
quotes,
unnecessary things in the dump, etc.  The question I would have is
why
not have a file with the dump and read the file?


(nods)

Seems like I need to add a mechanism for telling the selftests which
directory to load the tests relative to.
What about putting them inside the appropriate gcc.target directories? 
We could have one for the "generic" target assuming we build something 
like that, the others could live in their target specific directory.



jeff

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

[Joe Buck]

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

-Original Message-
From: libstdc++-ow...@gcc.gnu.org [mailto:libstdc++-ow...@gcc.gnu.org] On 
Behalf Of Jonathan Wakely
Sent: Monday, September 19, 2016 10:11 AM
To: libstd...@gcc.gnu.org; gcc-patches@gcc.gnu.org
Subject: [PATCH] libstdc++/77645 fix deque and vector xmethods for Python 3

The problem for these xmethods is that in Python 3 division of two integers 
produces a float, and GDB then doesn't allow that value to be used for pointer 
arithmetic (https://sourceware.org/PR20622).

PR libstdc++/77645
* python/libstdcxx/v6/xmethods.py (DequeWorkerBase.__init__)
(DequeWorkerBase.index, VectorWorkerBase.get): Cast results of
division to int to work with Python 3.

Tested x86_64-linux, committed to trunk.

I'll fix it for gcc-5 and gcc-6 too.

Re: Make regcprop to eliminate noop moves better

[Jeff Law]

On 09/18/2016 06:42 AM, Jan Hubicka wrote:

Hi,
while working on the GCN port I ended up with many redundant register copies
of the form
 mov reg, exec
 do something
 mov reg, exec
 do something
 ...
these copies are generated by LRA because exec is small register class and needs
a lot of reloading (it could be improved too, but I do not care because I want
to handle exec specially later anyway).

I was however suprised this garbage survives postreload optimizations.  It is 
easy
to fix in regcprop which already does some noop copy elimination, but only
of the for mov reg, reg after substituting.

This patch implements it and eliminates many movs while running testsuite.
Bootstrapped/regtested x86_64-linux, OK?
Needs a ChangeLog.  Ideally we'd have tests to verify we're catching 
these cases that survive the postreload optimizations.


With those OK.  Possibly OK with just a ChangeLog if it's too damn 
painful to construct a consistent end-to-end testcase (this may be a 
good example where we could probably use David's framework to test this 
directly rather than via an end-to-end test).


Jeff

Re: [PATCH] Fix PR c++/77639 (ICE during error recovery)

[Jason Merrill]

That looks good, OK.

On Mon, Sep 19, 2016 at 11:17 AM, Patrick Palka  wrote:
> On Mon, 19 Sep 2016, Jason Merrill wrote:
>
>> >if (tree t = maybe_new_partial_specialization (type))
>> > {
>> > + if (processing_template_parmlist)
>> > +   {
>> > +/* Some syntactically invalid code can confuse the compiler 
>> > into
>> > +   defining a specialization from inside a template parameter
>> > +   list.  Bail out now so that we won't ICE later.  */
>> > + gcc_assert (seen_error ());
>> > + return error_mark_node;
>> > +   }
>>
>> Rather than decide it's a specialization and then fail, let's avoid
>> treating it as a specialization in the first place.  I think we can
>> add a processing_template_parmlist check at the top where we avoid
>> trying to specialize lambdas.
>>
>> Jason
>>
>
> This works too.  However I was playing around with the parser a bit and
> noticed that the following also fixes the ICE.  It makes no sense to try
> to recover gracefully from a presumed missing "template <>" prefix when
> processing_template_parmlist.  This is what seems to be responsible for
> creating the bogus specialization.  Does this look OK or do you prefer
> adjusting maybe_process_partial_specialization()?
>
> gcc/cp/ChangeLog:
>
> PR c++/77639
> * parser.c (cp_parser_class_head): When
> processing_template_parmlist, don't assume that a
> class-head starts an explicit specialization.
>
> gcc/testsuite/ChangeLog:
>
> PR c++/77639
> * g++.dg/template/error-recovery4.C: New test.
> ---
>  gcc/cp/parser.c | 1 +
>  gcc/testsuite/g++.dg/template/error-recovery4.C | 5 +
>  2 files changed, 6 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/template/error-recovery4.C
>
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index fb88021..c03b9c2 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -22025,6 +22025,7 @@ cp_parser_class_head (cp_parser* parser,
>   it is not, try to recover gracefully.  */
>if (at_namespace_scope_p ()
>&& parser->num_template_parameter_lists == 0
> +  && !processing_template_parmlist
>&& template_id_p)
>  {
>/* Build a location of this form:
> diff --git a/gcc/testsuite/g++.dg/template/error-recovery4.C 
> b/gcc/testsuite/g++.dg/template/error-recovery4.C
> new file mode 100644
> index 000..1e52c6a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/error-recovery4.C
> @@ -0,0 +1,5 @@
> +// PR c++/77639
> +
> +template  struct B {};
> +template  {}; // { dg-error "" }
> +B i;
> --
> 2.10.0.87.g1cd26dd
>

Re: [f...@deneb.enyo.de: [PATCH] ada/77535: GNAT.Perfect_Hash_Generators for non-1-based strings]

[Thomas Quinot]

* Thomas Quinot, 2016-09-18 :

> Sorry for the delay in reviewing your patch. It looks good on the
> surface; we're QAing it internally to make sure it does not break
> PolyORB (which is the primary consumer of g-pehage). I'll let you know
> as soon as I have the results from that run.

QA went fine so OK to commit (please include a regression test).

Thanks!
Thomas.

-- 
Thomas Quinot, Ph.D. ** qui...@adacore.com ** IT Lead Engineer
   AdaCore -- Paris, France -- New York, USA

Re: [build] Fix race condition during libgcc build

[Jeff Law]

On 09/16/2016 03:54 PM, Eric Botcazou wrote:

Hi,

we have some new machines which seem to be very good at stumbling upon race
conditions during bootstrap.  Another example with libgcc:

/azun.a/gnatmail/sandbox/gnatcross-cont/x86-linux/gcc/build/./gcc/xgcc -
B/azun.a/gnatmail/sandbox/gnatcross-cont/x86-linux/gcc/build/./gcc/ -
B/azun.a/gnatmail/sandbox/gnatcross-cont/x86-linux/gcc/pkg/i686-pc-linux-
gnu/bin/ -B/azun.a/gnatmail/sandbox/gnatcross-cont/x86-linux/gcc/pkg/i686-pc-
linux-gnu/lib/ -isystem /azun.a/gnatmail/sandbox/gnatcross-cont/x86-
linux/gcc/pkg/i686-pc-linux-gnu/include -isystem
/azun.a/gnatmail/sandbox/gnatcross-cont/x86-linux/gcc/pkg/i686-pc-linux-
gnu/sys-include-g -O2 -O2  -g -O2 -DIN_GCC-W -Wall -Wno-narrowing -
Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wold-
style-definition  -isystem ./include   -fpic -mlong-double-80 -DUSE_ELF_SYMVER
-g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector   -fpic -mlong-
double-80 -DUSE_ELF_SYMVER -I. -I. -I../.././gcc -I../../../src/libgcc -
I../../../src/libgcc/. -I../../../src/libgcc/../gcc -
I../../../src/libgcc/../include -I../../../src/libgcc/config/libbid -
DENABLE_DECIMAL_BID_FORMAT -DHAVE_CC_TLS  -DUSE_TLS -o _negdi2_s.o -MT
_negdi2_s.o -MD -MP -MF _negdi2_s.dep -DSHARED -DL_negdi2 -c
../../../src/libgcc/libgcc2.c
config.status: linking ../../../src/libgcc/unwind-generic.h to unwind.h
config.status: linking ../../../src/libgcc/config/i386/linux-unwind.h to md-
unwind-support.h
config.status: linking ../../../src/libgcc/config/i386/sfp-machine.h to sfp-
machine.h
config.status: linking ../../../src/libgcc/gthr-posix.h to gthr-default.h
In file included from ../../../src/libgcc/libgcov-interface.c:27:0:
../../../src/libgcc/gthr.h:148:26: fatal error: gthr-default.h: No such file
or directory
 #include "gthr-default.h"
  ^
compilation terminated.
Makefile:894: recipe for target '_gcov_reset.o' failed
make[3]: *** [_gcov_reset.o] Error 1
make[3]: *** Waiting for unfinished jobs

So there is a race condition between the creation of (soft) links by the
libgcc configure script and the use of these links by the compilation of files
launched by the Makefile.

Tentative fix attached, tested on a bunch of machines for several native
platforms, OK for the mainline?


2016-09-16  Eric Botcazou  

* configure.ac: Do not create links, only substitute the filenames.
* configure: Regenerate.
* Makefile.in: Assign the substitution results to variables.
(LIBGCC_LINKS): Define.
(enable-execute-stack.c): New rule.
(unwind.h): Likewise.
(md-unwind-support.h): Likewise.
(sfp-machine.h): Likewise.
(gthr-default.h): Likewise.
Add $(LIBGCC_LINKS) to the prerequisites of all object files and
unwind.h as prerequisite of install-unwind_h-forbuild.

OK.
jeff

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

[Jonathan Wakely]

The problem for these xmethods is that in Python 3 division of two
integers produces a float, and GDB then doesn't allow that value
to be used for pointer arithmetic (https://sourceware.org/PR20622).

PR libstdc++/77645
* python/libstdcxx/v6/xmethods.py (DequeWorkerBase.__init__)
(DequeWorkerBase.index, VectorWorkerBase.get): Cast results of
division to int to work with Python 3.

Tested x86_64-linux, committed to trunk.

I'll fix it for gcc-5 and gcc-6 too.


commit a1eda07dc44e271ea07c266624b600cbae257b58
Author: Jonathan Wakely 
Date:   Mon Sep 19 18:02:36 2016 +0100

libstdc++/77645 fix deque and vector xmethods for Python 3

PR libstdc++/77645
* python/libstdcxx/v6/xmethods.py (DequeWorkerBase.__init__)
(DequeWorkerBase.index, VectorWorkerBase.get): Cast results of
division to int to work with Python 3.

diff --git a/libstdc++-v3/python/libstdcxx/v6/xmethods.py 
b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
index 95f9af9..71a5b75 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 = (512 / val_type.sizeof) or 1
+self._bufsize = int(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 + 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 + index / bit_size
+valp = start + int(index / bit_size)
 offset = index % bit_size
 return (valp.dereference() & (1 << offset)) > 0
 else:

Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Jeff Law]

On 09/16/2016 04:05 PM, Eric Botcazou wrote:

Hi,

for the attached reduced testcase, the ICE is:

eric@polaris:~/gnat/bugs/P901-028> ~/build/gcc/aarch64-linux/gcc/gnat1 -quiet
p.adb -O -I ~/build/gcc/aarch64-linux/gcc/ada/rts
+===GNAT BUG DETECTED==+
| 7.0.0 20160914 (experimental) [trunk revision 240142] (aarch64-linux) GCC
error:|
| in expand_shift_1, at expmed.c:2451  |
| Error detected around p.adb:21:29

#0  internal_error (gmsgid=0x22327b7 "in %s, at %s:%d")
at /home/eric/svn/gcc/gcc/diagnostic.c:1347
#1  0x01e2373b in fancy_abort (
file=0x1f965f8 "/home/eric/svn/gcc/gcc/expmed.c", line=2451,
function=0x1f96ce7  "expand_shift_1")
at /home/eric/svn/gcc/gcc/diagnostic.c:1415
#2  0x00eb435c in expand_shift_1 (code=RSHIFT_EXPR, mode=OImode,
shifted=0x768e02e8, amount=0x768bcef0, target=0x0, unsignedp=1)
at /home/eric/svn/gcc/gcc/expmed.c:2451
#3  0x00eb43bd in expand_shift (code=RSHIFT_EXPR, mode=OImode,
shifted=0x768e02e8, amount=255, target=0x0, unsignedp=1)
at /home/eric/svn/gcc/gcc/expmed.c:2467
#4  0x00ebefe3 in emit_store_flag (target=0x768de780, code=NE,
op0=0x768de798, op1=0x76c3d400, mode=OImode, unsignedp=1,
normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5826
#5  0x00ebe979 in emit_store_flag (target=0x768de780, code=NE,
op0=0x768dc138, op1=0x768dc030, mode=OImode, unsignedp=1,
normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5670
#6  0x00ebf0ab in emit_store_flag_force (target=0x768de780,
code=NE, op0=0x768dc138, op1=0x768dc030, mode=OImode, unsignedp=1,
normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5860
#7  0x00ef0aba in do_store_flag (ops=0x7fffd4d0,
target=0x768de780, mode=QImode) at /home/eric/svn/gcc/gcc/expr.c:11408
#8  0x00ee6873 in expand_expr_real_2 (ops=0x7fffd4d0, target=0x0,
tmode=QImode, modifier=EXPAND_NORMAL) at
/home/eric/svn/gcc/gcc/expr.c:9196

It's an attempt at generating a store-flag sequence with OImode and it fails
because there are no shift operations supported in that mode.  It turns out
that emit_store_flag_force knows how to fall back on a branchy sequence in
that case so the attached patch simply removes the assertion.

Tested on x86-64/Linux, OK for the mainline?


2016-09-16  Eric Botcazou  

* expmed.c (expand_shift_1): Remove assertion and adjust comment.
(expand_shift): Adjust comment.
Is this really safe.  If I look at where we call 
expand_shift/expand_shift_1 I get real worried that we could end up 
using that NULL value in a context where it's not expected.


So while emit_store_flag_force knows what to do upon NULL return, I'm 
not sure the other users of expand_shift/expand_shift_1 do.  For example 
in expand_bit_field_1:


  target = expand_shift (LSHIFT_EXPR, mode, target,
 GET_MODE_BITSIZE (mode) - bitsize, 
NULL_RTX, 0);

  return expand_shift (RSHIFT_EXPR, mode, target,
   GET_MODE_BITSIZE (mode) - bitsize, NULL_RTX, 0);

If the first call returned NULL, but the 2nd didn't, then we end up with 
a NULL argument in the RSHIFT_EXPR node we create.


or extract_fixed_bit_field_1:

 if (bitnum)
{
  /* If the field does not already start at the lsb,
 shift it so it does.  */
  /* Maybe propagate the target for the shift.  */
  rtx subtarget = (target != 0 && REG_P (target) ? target : 0);
  if (tmode != mode)
subtarget = 0;
  op0 = expand_shift (RSHIFT_EXPR, mode, op0, bitnum, 
subtarget, 1);

}
  /* Convert the value to the desired mode.  */
  if (mode != tmode)
op0 = convert_to_mode (tmode, op0, 1);

Can we end up with a NULL op0 which we then pass to convert_to_mode?

Is there any way to address this problem in emit_store_flag_force?

jeff

Re: [PATCH 5/9] regrename: Don't run if function was separately shrink-wrapped

[Segher Boessenkool]

Hi Jeff,

On Wed, Sep 14, 2016 at 01:33:04PM -0600, Jeff Law wrote:
> On 09/14/2016 01:03 PM, Segher Boessenkool wrote:
> >>If you think about it, conceptually we want the return insn to make the
> >>callee saved registers "used" so that DCE, regrename and friends don't
> >>muck with them.  The fact that we don't is as much never having to care
> >>all that much until recently.
> >
> >(There is no return insn at those exits; these are exits *without*
> >successor block, not the exit block).
> Ugh.  Anywhere we could attach this stuff in the insn chain?  If not, 
> the DF side of this problem gets uglier.

> >>I continue to wonder if we could add something similar to
> >>CALL_INSN_FUNCTION_USAGE where we attach uses for all the call-saved
> >>registers onto a return insn.  We would attach them at the end of
> >>prologue/epilogue generation and only attach those where were live
> >>somewhere in the code.
> >
> >Maybe adding the new insns to the {pro,epi}logue_insn_hash will help
> >something.  Or maybe it will blow up spectacularly.  Will know in a
> >bit.
> I think that'll resolve the sel-sched issue, but I doubt the rest.

Adding all the separately shrink-wrapped *logues to the hashes does not
help at all.

Trying to fix DF did not work either.

> >>I pondered just doing it for the separately wrapped components on that
> >>particular path, but I've yet to convince myself that's actually correct.
> >
> >If that is not correct, how is the status quo correct?  That is what
> >puzzles me above, too.
> I couldn't convince myself that we could trivially find all the 
> separately wrapped components on a particular path -- ie, we may have 
> had components saved/restored in some sub-graph.  If an exit point from 
> the function is reachable from that sub-graph, then we need to make sure 
> any components from the subgraph are marked as live in addition to those 
> which are restored on the exit path.
> 
> While it is just a reachability problem, I don't think we need to solve 
> it if we mark anything that was separately shrink wrapped as live at all 
> the exit points.

If I mark every block with no successors as needing all components, various
patches are no longer needed.  It also should not hurt performance much
(with the possible exception of sibling calls, which can be executed often;
still have to do performance runs).

The dce and cprop patches and the first regrename patch are still needed.
Those three are simple and obviously correct patches though.

I'll walk through all your comments again, updating the patches.  Will
send a v3 soon.

Thanks for all the reviews and helpful comments,


Segher

Re: Ping Re: Make max_align_t respect _Float128 [version 3]

[Paul Eggert]

On 09/19/2016 08:58 AM, Joseph Myers wrote:

Ping.  This patch
 is pending
review.

Thanks, the patch looks good to me. It should be safe for the uses of 
max_align_t that I know of (e.g., Emacs).

Re: [PATCH] Allow FP to be used as a call-saved registe

[Jeff Law]

On 09/19/2016 03:45 AM, Richard Earnshaw (lists) wrote:

So those don't seem to me to imply that the frame pointer needs to be a
fixed register.   So the first thing I'd do is fix the aarch64 port to
not do that and see what fallout there is and how to fix it.

Most ports simply don't mark the frame pointer as fixed.



The AArch64 ABI specification strongly recommends that, when a frame
record is not created, the frame pointer register is left unused so that
the frame chain, while not complete, is still valid (a chain of valid
records but ending in a NULL pointer).  That strongly suggests that FP
should remain a fixed register.
So essentially you're asking that even with -fomit-frame-pointer that FP 
still not be used and that the user should specify an additional 
-fcall-saved- parameter?


I can see your point -- lots of things use -fomit-frame-pointer and you 
may not necessarily want to make FP available to the register allocator.


But by requiring -fcall-saved-whatever, aren't you forcing folks to 
encode aarch64 specific flags into their build systems?


Neither seems like a good position to be in.


I guess we could push all of this into a new back-end option to permit
the 'I really want to use FP for general purposes', but it seems to be
just duplicating the existing use of -fcall-saved; so would be yet
another flag in the compiler that needs documenting.

Yea, and more aarch64 specific bits in the build system.



It seems much more sensible to me to just make a slight relaxation of
the fixed-register code and then re-use the existing options.
Maybe.   I understand the situation much better now, but I don't see a 
particularly good solution.

jeff

[PATCH, wwwdocs, committed] Update gcc-7/changes.html for LRA

[Segher Boessenkool]

Hi!

I'm committing the following for changes-7.html (and actually ran the
w3c validator thing this time ;-) )


Segher


Index: htdocs/gcc-7/changes.html
===
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-7/changes.html,v
retrieving revision 1.12
diff -U 3 -r1.12 changes.html
--- htdocs/gcc-7/changes.html   26 Aug 2016 18:08:49 -  1.12
+++ htdocs/gcc-7/changes.html   19 Sep 2016 16:25:14 -
@@ -21,6 +21,8 @@
 
 Caveats
 
+  GCC now uses LRA by default for new targets.
+
   The non-standard C++0x type traits
   has_trivial_default_constructor,
   has_trivial_copy_constructor and
@@ -265,7 +267,10 @@
 
 
 
-
+PowerPC / PowerPC64 / RS6000
+
+  The PowerPC port now uses LRA by default.
+
 
 
 

Re: [PATCH test]Revise gcc.dg/vect/pr57558-1.c by using int instead of long

[Jeff Law]

On 09/19/2016 08:54 AM, Bin Cheng wrote:

Hi,
Case gcc.dg/vect/pr57558-1.c uses unsigned long at the moment which can't be 
vectorized on Sparc64.  This patch revises it by using unsigned int.  I think 
it's an obvious change.
Test result checked, is it OK?

Thanks,
bin

gcc/testsuite/ChangeLog
2016-09-15  Bin Cheng  

* gcc.dg/vect/pr57558-1.c: Use unsigned int instead of unsigned long.


OK.
jeff

Re: [PATCH][simplify-rtx] (GTU (PLUS a C) (C - 1)) --> (LTU a -C)

[Bernd Schmidt]

On 09/19/2016 04:43 PM, Kyrill Tkachov wrote:

Ok?


Sure.


Bernd

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

[Bernd Edlinger]

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,
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


Bernd.

Re: [PATCH] Define pretty printers for C++17 library components

[Jonathan Wakely]

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

This tweaks the existing printers for Fundamentals TS components to
work for the C++17 versions, and adds a printer for std::variant.

* python/libstdcxx/v6/printers.py (StdVariantPrinter): Define.
(StdExpAnyPrinter, StdExpOptionalPrinter, StdExpStringViewPrinter):
Register for C++17 components in namespace std. Strip inline namespace
from typename.


This improves the output for the std::variant printer and adds type
printers and tests for the C++17 types.

I am inclined to change the printers for any and optional so they also
use a display_hint of 'array', which means they would show:

 std::optional = {val}

instead of 


 std::optional = {[contained value] = val}

IIRC I only made them show the "[contained value]" part because I
couldn't figure out how to do {val}.

We might also want to consider using 'array' for std::set, because
currently it shows fake indices for the elements, e.g.

 {[0] = x, [1] = y, [2] z}

but that would be better shown as {x, y, z}


commit b971ec023ef92bd5425df62d20af63317bde430f
Author: Jonathan Wakely 
Date:   Mon Sep 19 12:04:29 2016 +0100

Improve pretty printer for std::variant

	* python/libstdcxx/v6/printers.py (SingleObjContainerPrinter): Allow
	display_hint to be set by subclasses.
	(StdVariantPrinter): Use array for display_hint. Adjust output to be
	more similar to std::any and std::optional output.
	(register_type_printers): Add type printers for basic_string_view
	typedefs and experimental::any. Adjust type printers for
	fundamentals_v1 templates to match fundamentals_v2 and later.
	* testsuite/libstdc++-prettyprinters/cxx17.cc: New.

diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index 8c29760..ac529dd 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -879,9 +879,10 @@ class StdForwardListPrinter:
 class SingleObjContainerPrinter(object):
 "Base class for printers of containers of single objects"
 
-def __init__ (self, val, viz):
+def __init__ (self, val, viz, hint = None):
 self.contained_value = val
 self.visualizer = viz
+self.hint = hint
 
 def _recognize(self, type):
 """Return TYPE as a string after applying type printers"""
@@ -916,7 +917,7 @@ class SingleObjContainerPrinter(object):
 # if contained value is a map we want to display in the same way
 if hasattr (self.visualizer, 'children') and hasattr (self.visualizer, 'display_hint'):
 return self.visualizer.display_hint ()
-return None
+return self.hint
 
 
 class StdExpAnyPrinter(SingleObjContainerPrinter):
@@ -985,7 +986,6 @@ class StdVariantPrinter(SingleObjContainerPrinter):
 
 def __init__(self, typename, val):
 alternatives = self._template_args(val)
-self.alts = alternatives
 self.typename = "%s<%s>" % (typename, ', '.join([self._recognize(alt) for alt in alternatives]))
 self.index = val['_M_index']
 if self.index >= len(alternatives):
@@ -997,24 +997,25 @@ class StdVariantPrinter(SingleObjContainerPrinter):
 addr = val['_M_first']['_M_storage'].address
 contained_value = addr.cast(self.contained_type.pointer()).dereference()
 visualizer = gdb.default_visualizer(contained_value)
-super (StdVariantPrinter, self).__init__(contained_value, visualizer)
+super (StdVariantPrinter, self).__init__(contained_value, visualizer, 'array')
 
-def _template_args(self, val):
+@staticmethod
+def _template_args(val):
 n = 0
-args = ()
+args = []
 while True:
 try:
-args += (val.type.template_argument(n),)
+args.append(val.type.template_argument(n))
 except:
 return args
 n += 1
 
 def to_string(self):
 if self.contained_value is None:
-return "%s [no value]" % self.typename
+return "%s [no contained value]" % self.typename
 if hasattr(self.visualizer, 'children'):
-return "%s [alternative %d] %s" % (self.typename, self.index, self.visualizer.to_string())
-return self.typename
+return "%s [index %d] containing %s" % (self.typename, self.index, self.visualizer.to_string())
+return "%s [index %d]" % (self.typename, self.index)
 
 class StdExpStringViewPrinter:
 "Print a std::basic_string_view or std::experimental::basic_string_view"
@@ -1262,6 +1263,7 @@ def register_type_printers(obj):
 
 for pfx in ('', 'w'):
 add_one_type_printer(obj, 'basic_string', pfx + 'string')
+add_one_type_printer(obj, 'basic_string_view', pfx + 'string_view')
 add_one_type_printer(obj, 'basic_ios', pfx + 'ios')
 add_one_type_printer(obj, 

Ping Re: Make max_align_t respect _Float128 [version 3]

[Joseph Myers]

Ping.  This patch 
 is pending 
review.

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

Re: [Fortran, Patch] First patch for coarray FAILED IMAGES (TS 18508)

[Andre Vehreschild]

Hi Alessandro,

there are still some violations of the style guide:

contrib/check_GNU_style.sh first_complete_patch_REV2.diff

emits:

Lines should not exceed 80 characters.
154:+  add_sym_2 ("failed_images", GFC_ISYM_FAILED_IMAGES,
CLASS_TRANSFORMATIONAL, ACTUAL_NO, BT_INTEGER, 155:+ dd,
GFC_STD_F2008_TS, gfc_check_failed_images, gfc_simplify_failed_images,
156:+ gfc_resolve_failed_images, "team", BT_INTEGER, di, OPTIONAL,
"kind", BT_INTEGER, di, OPTIONAL); 165:+  add_sym_2 ("image_status",
GFC_ISYM_IMAGE_STATUS, CLASS_ELEMENTAL, ACTUAL_NO, BT_INTEGER,
166:+ di, GFC_STD_F2008_TS, gfc_check_image_status,
gfc_simplify_image_status, 167:+ gfc_resolve_image_status, "image",
BT_INTEGER, di, REQUIRED, "team", BT_INTEGER, di, OPTIONAL);
247:+gfc_resolve_image_status (gfc_expr *f, gfc_expr *image, gfc_expr *team
ATTRIBUTE_UNUSED) 409:+  result = transformational_result (result, 0,
BT_INTEGER, kind->ts.kind, _current_locus);
420:+gfc_simplify_image_status(gfc_expr *image ATTRIBUTE_UNUSED, gfc_expr *team
ATTRIBUTE_UNUSED) 469:+  gfor_fndecl_caf_failed_images =
gfc_build_library_function_decl_with_spec ( 471:+pvoid_type_node, 3,
pvoid_type_node, integer_type_node, integer_type_node); 

 The remainder of the script output needs no fix, because its in Fortran
code.

You should fix the above, where they are not in a Fortran testcases. This
allows people with 80 column terminals to read the whole line without scrolling.

The if in resolve.c at 8837: resolve_failed_image (... is intentional? It is
doing nothing. So do you plan to add more code, or will there never be
anything. If the later I recommend to just put a comment there and remove the
empty if.

There still is no test when -fcoarray=single is used. This shouldn't be so
hard, should it?

Regards,
Andre

On Mon, 19 Sep 2016 08:30:12 -0700
Alessandro Fanfarillo  wrote:

> * PING *
> 
> On Sep 7, 2016 3:01 PM, "Alessandro Fanfarillo" 
> wrote:
> 
> > Dear all,
> > the attached patch supports failed images also when -fcoarray=single is
> > used.
> >
> > Built and regtested on x86_64-pc-linux-gnu.
> >
> > Cheers,
> > Alessandro
> >
> > 2016-08-09 5:22 GMT-06:00 Paul Richard Thomas <
> > paul.richard.tho...@gmail.com>:
> > > Hi Sandro,
> > >
> > > As far as I can see, this is OK barring a couple of minor wrinkles and
> > > a question:
> > >
> > > For coarray_failed_images_err.f90 and coarray_image_status_err.f90 you
> > > have used the option -fdump-tree-original without making use of the
> > > tree dump.
> > >
> > > Mikael asked you to provide an executable test with -fcoarray=single.
> > > Is this not possible for some reason?
> > >
> > > Otherwise, this is OK for trunk.
> > >
> > > Thanks for the patch.
> > >
> > > Paul
> > >
> > > On 4 August 2016 at 05:07, Alessandro Fanfarillo
> > >  wrote:
> > >> * PING *
> > >>
> > >> 2016-07-21 13:05 GMT-06:00 Alessandro Fanfarillo <
> > fanfarillo@gmail.com>:
> > >>> Dear Mikael and all,
> > >>>
> > >>> in attachment the new patch, built and regtested on
> > x86_64-pc-linux-gnu.
> > >>>
> > >>> Cheers,
> > >>> Alessandro
> > >>>
> > >>> 2016-07-20 13:17 GMT-06:00 Mikael Morin :
> >  Le 20/07/2016 à 11:39, Andre Vehreschild a écrit :
> > >
> > > Hi Mikael,
> > >
> > >
> > >>> +  if(st == ST_FAIL_IMAGE)
> > >>> +new_st.op = EXEC_FAIL_IMAGE;
> > >>> +  else
> > >>> +gcc_unreachable();
> > >>
> > >> You can use
> > >> gcc_assert (st == ST_FAIL_IMAGE);
> > >> foo...;
> > >> instead of
> > >> if (st == ST_FAIL_IMAGE)
> > >> foo...;
> > >> else
> > >> gcc_unreachable ();
> > >
> > >
> > > Be careful, this is not 100% identical in the general case. For older
> > > gcc version (gcc < 4008) gcc_assert() is mapped to nothing, esp. not
> > to
> > > an abort(), so the behavior can change. But in this case everything
> > is
> > > fine, because the patch is most likely not backported.
> > >
> >  Didn't know about this. The difference seems to be very subtle.
> >  I don't mind much anyway. The original version can stay if preferred,
> > this
> >  was just a suggestion.
> > 
> >  By the way, if the function is inlined in its single caller, the
> > assert or
> >  unreachable statement can be removed, which avoids choosing between
> > them.
> >  That's another suggestion.
> > 
> > 
> > >>> +
> > >>> +  return MATCH_YES;
> > >>> +
> > >>> + syntax:
> > >>> +  gfc_syntax_error (st);
> > >>> +
> > >>> +  return MATCH_ERROR;
> > >>> +}
> > >>> +
> > >>> +match
> > >>> +gfc_match_fail_image (void)
> > >>> +{
> > >>> +  /* if (!gfc_notify_std (GFC_STD_F2008_TS, "FAIL IMAGE statement
> > >>> at %C")) */
> > >>> +  

[PATCH] libstdc++/77645 Fix xmethods for std::list

[Jonathan Wakely]

The std::list xmethods have been broken since I added support for
allocators with fancy pointers, and the switch to std::gnu++14 by
default. This makes them work for any -std option.

PR libstdc++/77645
* python/libstdcxx/v6/xmethods.py (DequeWorkerBase.index): Rename
argument.
(ListWorkerBase.get_value_from_node): Define new method.
(ListFrontWorker.__call__, ListBackWorker.__call__): Use it.

Tested x86_64-linux, committed to trunk and gcc-6-branch.

commit e25f0f03f10c4bd72ddeb6e32e51a5c71134455b
Author: Jonathan Wakely 
Date:   Mon Sep 19 15:25:06 2016 +0100

libstdc++/77645 Fix xmethods for std::list

PR libstdc++/77645
* python/libstdcxx/v6/xmethods.py (DequeWorkerBase.index): Rename
argument.
(ListWorkerBase.get_value_from_node): Define new method.
(ListFrontWorker.__call__, ListBackWorker.__call__): Use it.

diff --git a/libstdc++-v3/python/libstdcxx/v6/xmethods.py 
b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
index eccd574..95f9af9 100644
--- a/libstdc++-v3/python/libstdcxx/v6/xmethods.py
+++ b/libstdc++-v3/python/libstdcxx/v6/xmethods.py
@@ -174,10 +174,10 @@ class DequeWorkerBase(gdb.xmethod.XMethodWorker):
 first = obj['_M_impl']['_M_finish']['_M_first']
 return (last_node - first_node) * self._bufsize + (cur - first)
 
-def index(self, obj, index):
+def index(self, obj, idx):
 first_node = obj['_M_impl']['_M_start']['_M_node']
-index_node = first_node + index / self._bufsize
-return index_node[0][index % self._bufsize]
+index_node = first_node + idx / self._bufsize
+return index_node[0][idx % self._bufsize]
 
 class DequeEmptyWorker(DequeWorkerBase):
 def get_arg_types(self):
@@ -328,6 +328,15 @@ class ListWorkerBase(gdb.xmethod.XMethodWorker):
 def get_arg_types(self):
 return None
 
+def get_value_from_node(self, node):
+node = node.dereference()
+if node.type.fields()[1].name == '_M_data':
+# C++03 implementation, node contains the value as a member
+return node['_M_data']
+# C++11 implementation, node stores value in __aligned_membuf
+addr = node['_M_storage'].address
+return addr.cast(self._val_type.pointer()).dereference()
+
 class ListEmptyWorker(ListWorkerBase):
 def get_result_type(self, obj):
 return get_bool_type()
@@ -358,7 +367,7 @@ class ListFrontWorker(ListWorkerBase):
 
 def __call__(self, obj):
 node = obj['_M_impl']['_M_node']['_M_next'].cast(self._node_type)
-return node['_M_data']
+return self.get_value_from_node(node)
 
 class ListBackWorker(ListWorkerBase):
 def get_result_type(self, obj):
@@ -366,7 +375,7 @@ class ListBackWorker(ListWorkerBase):
 
 def __call__(self, obj):
 prev_node = obj['_M_impl']['_M_node']['_M_prev'].cast(self._node_type)
-return prev_node['_M_data']
+return self.get_value_from_node(prev_node)
 
 class ListMethodsMatcher(gdb.xmethod.XMethodMatcher):
 def __init__(self):

Re: [PATCH 1/3] Put a TARGET_LRA_P into every target

[Segher Boessenkool]

On Fri, Sep 16, 2016 at 12:04:23PM -0700, Mike Stump wrote:
> On Sep 16, 2016, at 6:40 AM, Segher Boessenkool  
> wrote:
> > 
> > Does just "The default version of this target hook returns true." sound
> > better?  I.e. delete "always".
> 
> That is fine.

Okay, I'll commit the following (yes I'm slow).

Thanks,


Segher


2016-09-19  Segher Boessenkool  

* target.def (lra_p): Wordsmithing.
* doc/tm.texi: Regenerate.

---
 gcc/doc/tm.texi | 2 +-
 gcc/target.def  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 0ca00d2..ced6274 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -2861,7 +2861,7 @@ A target hook which can change allocno class for given 
pseudo from
 @end deftypefn
 
 @deftypefn {Target Hook} bool TARGET_LRA_P (void)
-A target hook which returns true if we use LRA instead of reload pass.The 
default version of this target hook returns always true.  New ports  should use 
LRA, and existing ports are encouraged to convert.
+A target hook which returns true if we use LRA instead of reload pass.The 
default version of this target hook returns true.  New ports  should use LRA, 
and existing ports are encouraged to convert.
 @end deftypefn
 
 @deftypefn {Target Hook} int TARGET_REGISTER_PRIORITY (int)
diff --git a/gcc/target.def b/gcc/target.def
index a4e4cbb..0e10d6f 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -4929,7 +4929,7 @@ DEFHOOK
 (lra_p,
  "A target hook which returns true if we use LRA instead of reload pass.\
   \
-  The default version of this target hook returns always true.  New ports\
+  The default version of this target hook returns true.  New ports\
   should use LRA, and existing ports are encouraged to convert.",
  bool, (void),
  default_lra_p)
-- 
1.9.3

Re: [PATCH] Fix PR c++/77639 (ICE during error recovery)

[Patrick Palka]

On Mon, 19 Sep 2016, Jason Merrill wrote:

> >if (tree t = maybe_new_partial_specialization (type))
> > {
> > + if (processing_template_parmlist)
> > +   {
> > +/* Some syntactically invalid code can confuse the compiler 
> > into
> > +   defining a specialization from inside a template parameter
> > +   list.  Bail out now so that we won't ICE later.  */
> > + gcc_assert (seen_error ());
> > + return error_mark_node;
> > +   }
> 
> Rather than decide it's a specialization and then fail, let's avoid
> treating it as a specialization in the first place.  I think we can
> add a processing_template_parmlist check at the top where we avoid
> trying to specialize lambdas.
> 
> Jason
> 

This works too.  However I was playing around with the parser a bit and
noticed that the following also fixes the ICE.  It makes no sense to try
to recover gracefully from a presumed missing "template <>" prefix when
processing_template_parmlist.  This is what seems to be responsible for
creating the bogus specialization.  Does this look OK or do you prefer
adjusting maybe_process_partial_specialization()?

gcc/cp/ChangeLog:

PR c++/77639
* parser.c (cp_parser_class_head): When
processing_template_parmlist, don't assume that a
class-head starts an explicit specialization.

gcc/testsuite/ChangeLog:

PR c++/77639
* g++.dg/template/error-recovery4.C: New test.
---
 gcc/cp/parser.c | 1 +
 gcc/testsuite/g++.dg/template/error-recovery4.C | 5 +
 2 files changed, 6 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/template/error-recovery4.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index fb88021..c03b9c2 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -22025,6 +22025,7 @@ cp_parser_class_head (cp_parser* parser,
  it is not, try to recover gracefully.  */
   if (at_namespace_scope_p ()
   && parser->num_template_parameter_lists == 0
+  && !processing_template_parmlist
   && template_id_p)
 {
   /* Build a location of this form:
diff --git a/gcc/testsuite/g++.dg/template/error-recovery4.C 
b/gcc/testsuite/g++.dg/template/error-recovery4.C
new file mode 100644
index 000..1e52c6a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/error-recovery4.C
@@ -0,0 +1,5 @@
+// PR c++/77639
+
+template  struct B {};
+template  {}; // { dg-error "" }
+B i;
-- 
2.10.0.87.g1cd26dd

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

[Vladimir N Makarov]

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.

[PATCH test]Revise gcc.dg/vect/pr57558-1.c by using int instead of long

[Bin Cheng]

Hi,
Case gcc.dg/vect/pr57558-1.c uses unsigned long at the moment which can't be 
vectorized on Sparc64.  This patch revises it by using unsigned int.  I think 
it's an obvious change.
Test result checked, is it OK?

Thanks,
bin

gcc/testsuite/ChangeLog
2016-09-15  Bin Cheng  

* gcc.dg/vect/pr57558-1.c: Use unsigned int instead of unsigned long.Index: gcc/testsuite/gcc.dg/vect/pr57558-1.c
===
--- gcc/testsuite/gcc.dg/vect/pr57558-1.c   (revision 240166)
+++ gcc/testsuite/gcc.dg/vect/pr57558-1.c   (working copy)
@@ -1,10 +1,10 @@
 /* { dg-do compile } */
 /* { dg-require-effective-target vect_int } */
 
-typedef unsigned long ul;
-void foo (ul* __restrict x, ul* __restrict y, ul n)
+typedef unsigned int u_int;
+void foo (u_int* __restrict x, u_int* __restrict y, u_int n)
 {
-  ul i;
+  u_int i;
   for (i=1; i<=n; i++, x++, y++)
 *x += *y;
 }

Re: Do not drom MEM_EXPR when accessing structure fields

[Bernd Edlinger]

On 09/19/16 16:23, Richard Biener wrote:
> On 19/09/16 15:49, Bernd Edlinger wrote:
>> On 09/19/16 11:28, Richard Biener wrote:
>>> On Sun, 18 Sep 2016, Jan Hubicka wrote:
>>>
> Hi,
>
>> when expanding code for
>> struct a {short a,b,c,d;} *a;
>> a->c;
>>
>> we first produce correct BLKmode MEM rtx representing the whole
>> structure *a,
>> then we use adjust_address to turn it into HImode MEM and finally
>> extract_bitfield is used to offset it by 32 bits to get correct 
> value.
>
> I tried to create a test case as follows and stepped thru the code.
>
> cat test.c
> struct a {short a,b,c,d;};
> void foo (struct a *a)
> {
>  a->c = 1;
> }
>
>
> First I get a DImode MEM rtx not BLKmode:
>
> (mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])
>
> and adjust_address does not clear the MEM_EXPR
>>>
>>> it's only cleared when later offsetted
>>>
> thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:
>
> (mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])
>
> and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:
>
> (mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
>
> which looks perfectly OK.
>>>
>>> Sure you quoted the correct RTL?  MEM_SIZE == 6 looks wrong.  Note
>>> we don't do set_mem_attributes_minus_bitpos when we go the bitfield
>>> extraction path.
>>>
>>
>> I think it is correct, to_rtx points 4 bytes before a->c "+-4", and
>> sizeof(a->c) is 2. So 4+2 is 6 there will be no access beyond that.
>
> Hmm, but if the MEM is HImode then doesn't this imply it will access
> 2 bytes starting at to_rtx (aka a->c "+-4").  At this point the offset
> doesn't seem to be accounted for yet (thus the MEM isn't accessing a->c
> yet).
>

Yes here to_rtx points still at a, and the MEM_EXPR means >c - 4b,
when the constant offset of 4 bytes gets added later in store_field,
and also there it looks like everything is working correctly.

6813  && TREE_CODE (exp) == MEM_REF
(gdb)
6770  if (mode == VOIDmode
(gdb)
6939  rtx to_rtx = adjust_address (target, mode, bitpos / 
BITS_PER_UNIT);
(gdb) call debug(target)
(mem:HI (reg/v/f:DI 87 [ a ]) [2 a_2(D)->c+-4 S6 A16])
(gdb) p mode
$1 = HImode
(gdb) p bitpos
$2 = 32
(gdb) n
6941  if (to_rtx == target)
(gdb) call debug(to_rtx)
(mem:HI (plus:DI (reg/v/f:DI 87 [ a ])
 (const_int 4 [0x4])) [2 a_2(D)->c+0 S2 A16])
(gdb)


>> And yes the RTL is correct: this is what I did:
>>
>> (gdb) n
>> 5152   to_rtx = shallow_copy_rtx (to_rtx);
>> (gdb) call debug(to_rtx)
>> (mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
>> (gdb) n
>> 5153   set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
>> (gdb) call debug(to_rtx)
>> (mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
>> (gdb) n
>> 5154   if (volatilep)
>> (gdb) call debug(to_rtx)
>> (mem:HI (reg/v/f:DI 87 [ a ]) [2 a_2(D)->c+-4 S6 A16])
>> (gdb)
>
> So set_mem_attributes_minus_bitpos seems to try making later adjustment
> by bitpos "valid" if we at the same time shrink size to what was
> originally intended.
>
>> It is not about bitfield extraction, as we are in expand_assignment.
>> next is the call to optimize_bitfield_assignment_op and
>> store_field.
>>
>> I believe, that with Jan's patch we have at this point only
>> a different mode on the to_rtx.  And it is possible that
>> store_field or store_bit_field takes a different decision
>> dependent on GET_MODE(to_rtx), which may accidentally delete
>> the MEM_EXPR.
>
> It offsets the MEM which causes it to go "out of bounds" (no such
> "fancy" too large MEM_SIZE is present) which causes us to drop the
> MEM_EXPR.
>
> IMHO this piecewise (re-)construction of MEM_ATTRS is quite fragile...
> But refactoring this area of the compiler is fragile as well...
>

Yes...


Bernd.

RE: [PATCH 4/4] [MIPS] Add tests for MSA

[Matthew Fortune]

Hi Robert,

Sorry for the long delay. Just a couple of cleanup bits in here but
otherwise OK. Unless you want me to read through again please go
ahead and commit once you've addressed the comments.

We could do more in mips.exp for inferring -mfp64 and -mhard-float
but I'm not sure it is worth it currently. Let's leave it for a
round of testsuite changes at a later date.

Thanks,
Matthew

>---
> gcc/testsuite/gcc.dg/vect/slp-26.c   |7 +-
> gcc/testsuite/gcc.dg/vect/tree-vect.h|2 +
> gcc/testsuite/gcc.target/mips/mips.exp   |8 +
> gcc/testsuite/gcc.target/mips/msa-builtins.c | 1086 ++
> gcc/testsuite/gcc.target/mips/msa.c  |  631 +++
> gcc/testsuite/lib/target-supports.exp|  191 -
> 6 files changed, 1895 insertions(+), 30 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/mips/msa-builtins.c
> create mode 100644 gcc/testsuite/gcc.target/mips/msa.c
>
>diff --git a/gcc/testsuite/gcc.dg/vect/slp-26.c 
>b/gcc/testsuite/gcc.dg/vect/slp-26.c
>index 8b224ff..01f4e4e 100644
>--- a/gcc/testsuite/gcc.dg/vect/slp-26.c
>+++ b/gcc/testsuite/gcc.dg/vect/slp-26.c
>@@ -46,6 +46,7 @@ int main (void)
>   return 0;
> }
> 
>-/* { dg-final { scan-tree-dump-times "vectorized 0 loops" 1 "vect"  } } */
>-/* { dg-final { scan-tree-dump-times "vectorizing stmts using SLP" 0 "vect"  
>} } */
>-  
>+/* { dg-final { scan-tree-dump-times "vectorized 0 loops" 1 "vect" { target { 
>! mips_msa } } } } */
>+/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 1 "vect" { target { 
>mips_msa } } } } */
>+/* { dg-final { scan-tree-dump-times "vectorizing stmts using SLP" 0 "vect" { 
>target { ! mips_msa } } } } */
>+/* { dg-final { scan-tree-dump-times "vectorizing stmts using SLP" 1 "vect" { 
>target { mips_msa } } } } */

Why can we vectorise this with MSA but presumably no other arch can?

>diff --git a/gcc/testsuite/gcc.target/mips/mips.exp 
>b/gcc/testsuite/gcc.target/mips/mips.exp
>index 3258105..7c24140 100644
>--- a/gcc/testsuite/gcc.target/mips/mips.exp
>+++ b/gcc/testsuite/gcc.target/mips/mips.exp
>@@ -290,6 +290,7 @@ foreach option {
> relax-pic-calls
> mcount-ra-address
> odd-spreg
>+msa
> } {
> lappend mips_option_groups $option "-m(no-|)$option"
> }
>@@ -820,6 +821,12 @@ proc mips-dg-init {} {
>   "-mno-synci",
>   #endif
> 
>+  #ifdef __mips_msa
>+  "-mmsa"
>+  #else
>+  "-mno-msa"
>+  #endif
>+
>   0
>   };
> }]
>@@ -1368,6 +1375,7 @@ proc mips-dg-options { args } {
> mips_option_dependency options "-mfpxx" "-mno-paired-single"
> mips_option_dependency options "-msoft-float" "-mno-paired-single"
> mips_option_dependency options "-mno-paired-single" "-mno-mips3d"
>+mips_option_dependency options "-mmsa" "-mno-mips16"
> 
> # If the test requires an unsupported option, change run tests
> # to link tests.
>diff --git a/gcc/testsuite/gcc.target/mips/msa-builtins.c 
>b/gcc/testsuite/gcc.target/mips/msa-builtins.c
>new file mode 100644
>index 000..f5e1efa
>--- /dev/null
>+++ b/gcc/testsuite/gcc.target/mips/msa-builtins.c
>@@ -0,0 +1,1086 @@
>+/* Test builtins for MIPS MSA ASE instructions */
>+/* { dg-do compile } */
>+/* { dg-options "-mfp64 -mhard-float -mmsa" } */
>+/* { dg-skip-if "needs asm output" { *-*-* } { "-flto" } { 
>"-ffat-lto-objects" } } */

This should not be necessary. The scan-assembler directive forces 
fat-lto-objects
via some higher level code. I have seen this fail at times but never tracked it 
down
please remove dg-skip-if and see if you still get any -fno-fat-lto-objects 
builds
of this file.

>+
>+/* { dg-final { scan-assembler "msa_addv_b.*:.*addv\\.b.*msa_addv_b" } } */

Should these be seen only once: I.e. scan-assembler-times?

>+/* { dg-final { scan-assembler "msa_addv_h.*:.*addv\\.h.*msa_addv_h" } } */
>+/* { dg-final { scan-assembler "msa_addv_w.*:.*addv\\.w.*msa_addv_w" } } */
>+/* { dg-final { scan-assembler "msa_addv_d.*:.*addv\\.d.*msa_addv_d" } } */
>+/* { dg-final { scan-assembler "msa_addvi_b.*:.*addvi\\.b.*msa_addvi_b" } } */
>+/* { dg-final { scan-assembler "msa_addvi_h.*:.*addvi\\.h.*msa_addvi_h" } } */
>+/* { dg-final { scan-assembler "msa_addvi_w.*:.*addvi\\.w.*msa_addvi_w" } } */
>+/* { dg-final { scan-assembler "msa_addvi_d.*:.*addvi\\.d.*msa_addvi_d" } } */
>+/* { dg-final { scan-assembler "msa_add_a_b.*:.*add_a\\.b.*msa_add_a_b" } } */
>+/* { dg-final { scan-assembler "msa_add_a_h.*:.*add_a\\.h.*msa_add_a_h" } } */
>+/* { dg-final { scan-assembler "msa_add_a_w.*:.*add_a\\.w.*msa_add_a_w" } } */
>+/* { dg-final { scan-assembler "msa_add_a_d.*:.*add_a\\.d.*msa_add_a_d" } } */
>+/* { dg-final { scan-assembler "msa_adds_a_b.*:.*adds_a\\.b.*msa_adds_a_b" } 
>} */
>+/* { dg-final { scan-assembler "msa_adds_a_h.*:.*adds_a\\.h.*msa_adds_a_h" } 
>} */
>+/* { dg-final { scan-assembler "msa_adds_a_w.*:.*adds_a\\.w.*msa_adds_a_w" } 
>} */
>+/* { dg-final { scan-assembler 

Re: [PATCH][simplify-rtx] (GTU (PLUS a C) (C - 1)) --> (LTU a -C)

[Kyrill Tkachov]

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


On 16/09/16 11:45, Bernd Schmidt wrote:

On 09/16/2016 10:40 AM, Kyrill Tkachov wrote:


2016-09-16  Kyrylo Tkachov  

* simplify-rtx.c (simplify_relational_operation_1): Add transformation
(GTU (PLUS a C) (C - 1)) --> (LTU a -C).

2016-09-16  Kyrylo Tkachov  

* gcc.target/aarch64/gtu_to_ltu_cmp_1.c: New test.


Ok. Don't know if you want to add more variants of the input code to the 
testcase to make sure they're all covered.



Thanks.
I'm having trouble writing testcases for variations of the original testcase as 
GCC really really wants to convert
everything to a comparison against 1 at RTL level, so only the x == -2 || x == 
-1 condition seems to trigger this.
However, testcases of the form:
unsigned int
foo (unsigned int a, unsigned int b)
{
  return (a + 10) > 9;
}

seem to trigger it, so I can add some of this form. However, these will be 
optimised by a match.pd version
of this transformation that I'm working on.


Here's the patch with that test added as well.  The simplify-rtx transformation 
catches it, but if we end up
adding the match.pd form, it will get caught earlier at the GIMPLE level. The 
test will pass regardless of
where this transformation is done.

Ok?

Thanks,
Kyrill

2016-09-16  Kyrylo Tkachov  

* simplify-rtx.c (simplify_relational_operation_1): Add transformation
(GTU (PLUS a C) (C - 1)) --> (LTU a -C).

2016-09-16  Kyrylo Tkachov  

* gcc.target/aarch64/gtu_to_ltu_cmp_1.c: New test.
* gcc.target/aarch64/gtu_to_ltu_cmp_2.c: New test.

diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 054c0f9d41664f8a4d11765dfb501647cfbc728f..63e864a237a05d250e3d8a3510775585fe8002db 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -4663,6 +4663,19 @@ simplify_relational_operation_1 (enum rtx_code code, machine_mode mode,
   cmp_mode, XEXP (op0, 0), new_cmp);
 }
 
+  /* (GTU (PLUS a C) (C - 1)) where C is a non-zero constant can be
+ transformed into (LTU a -C).  */
+  if (code == GTU && GET_CODE (op0) == PLUS && CONST_INT_P (op1)
+  && CONST_INT_P (XEXP (op0, 1))
+  && (UINTVAL (op1) == UINTVAL (XEXP (op0, 1)) - 1)
+  && XEXP (op0, 1) != const0_rtx)
+{
+  rtx new_cmp
+	= simplify_gen_unary (NEG, cmp_mode, XEXP (op0, 1), cmp_mode);
+  return simplify_gen_relational (LTU, mode, cmp_mode,
+   XEXP (op0, 0), new_cmp);
+}
+
   /* Canonicalize (LTU/GEU (PLUS a b) b) as (LTU/GEU (PLUS a b) a).  */
   if ((code == LTU || code == GEU)
   && GET_CODE (op0) == PLUS
diff --git a/gcc/testsuite/gcc.target/aarch64/gtu_to_ltu_cmp_1.c b/gcc/testsuite/gcc.target/aarch64/gtu_to_ltu_cmp_1.c
new file mode 100644
index ..81c536c90afe38932c48ed0af24f55e73eeff80e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/gtu_to_ltu_cmp_1.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int
+f1 (int x, int t)
+{
+  if (x == -1 || x == -2)
+t = 1;
+
+  return t;
+}
+
+/* { dg-final { scan-assembler-times "cmn\\tw\[0-9\]+, #2" 1 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/gtu_to_ltu_cmp_2.c b/gcc/testsuite/gcc.target/aarch64/gtu_to_ltu_cmp_2.c
new file mode 100644
index ..e0e999f9df39c29bb79d8a8f7d9a17f213bd115b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/gtu_to_ltu_cmp_2.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+unsigned int
+foo (unsigned int a, unsigned int b)
+{
+  return (a + 10) > 9;
+}
+
+/* { dg-final { scan-assembler-times "cmn\\tw\[0-9\]+" 1 } } */
+/* { dg-final { scan-assembler-not "add\\tw\[0-9\]+" } } */

Re: [PATCH] Fix PR c++/77639 (ICE during error recovery)

[Jason Merrill]

>if (tree t = maybe_new_partial_specialization (type))
> {
> + if (processing_template_parmlist)
> +   {
> +/* Some syntactically invalid code can confuse the compiler into
> +   defining a specialization from inside a template parameter
> +   list.  Bail out now so that we won't ICE later.  */
> + gcc_assert (seen_error ());
> + return error_mark_node;
> +   }

Rather than decide it's a specialization and then fail, let's avoid
treating it as a specialization in the first place.  I think we can
add a processing_template_parmlist check at the top where we avoid
trying to specialize lambdas.

Jason

Re: Do not drom MEM_EXPR when accessing structure fields

[Richard Biener]

On 19/09/16 15:49, Bernd Edlinger wrote:
> On 09/19/16 11:28, Richard Biener wrote:
>> On Sun, 18 Sep 2016, Jan Hubicka wrote:
>>
 Hi,

   > when expanding code for
   > struct a {short a,b,c,d;} *a;
   > a->c;
   >
   > we first produce correct BLKmode MEM rtx representing the whole
   > structure *a,
   > then we use adjust_address to turn it into HImode MEM and finally
   > extract_bitfield is used to offset it by 32 bits to get correct value.

 I tried to create a test case as follows and stepped thru the code.

 cat test.c
 struct a {short a,b,c,d;};
 void foo (struct a *a)
 {
 a->c = 1;
 }


 First I get a DImode MEM rtx not BLKmode:

 (mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])

 and adjust_address does not clear the MEM_EXPR
>>
>> it's only cleared when later offsetted
>>
 thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:

 (mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])

 and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:

 (mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])

 which looks perfectly OK.
>>
>> Sure you quoted the correct RTL?  MEM_SIZE == 6 looks wrong.  Note
>> we don't do set_mem_attributes_minus_bitpos when we go the bitfield
>> extraction path.
>>
> 
> I think it is correct, to_rtx points 4 bytes before a->c "+-4", and
> sizeof(a->c) is 2. So 4+2 is 6 there will be no access beyond that.

Hmm, but if the MEM is HImode then doesn't this imply it will access
2 bytes starting at to_rtx (aka a->c "+-4").  At this point the offset
doesn't seem to be accounted for yet (thus the MEM isn't accessing a->c
yet).

> And yes the RTL is correct: this is what I did:
> 
> (gdb) n
> 5152to_rtx = shallow_copy_rtx (to_rtx);
> (gdb) call debug(to_rtx)
> (mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
> (gdb) n
> 5153set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
> (gdb) call debug(to_rtx)
> (mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
> (gdb) n
> 5154if (volatilep)
> (gdb) call debug(to_rtx)
> (mem:HI (reg/v/f:DI 87 [ a ]) [2 a_2(D)->c+-4 S6 A16])
> (gdb)

So set_mem_attributes_minus_bitpos seems to try making later adjustment
by bitpos "valid" if we at the same time shrink size to what was
originally intended.

> It is not about bitfield extraction, as we are in expand_assignment.
> next is the call to optimize_bitfield_assignment_op and
> store_field.
> 
> I believe, that with Jan's patch we have at this point only
> a different mode on the to_rtx.  And it is possible that
> store_field or store_bit_field takes a different decision
> dependent on GET_MODE(to_rtx), which may accidentally delete
> the MEM_EXPR.

It offsets the MEM which causes it to go "out of bounds" (no such
"fancy" too large MEM_SIZE is present) which causes us to drop the
MEM_EXPR.

IMHO this piecewise (re-)construction of MEM_ATTRS is quite fragile...
But refactoring this area of the compiler is fragile as well...

Richard.

> 
> Bernd.
> 

 But with your patch the RTX looks different:

 (mem:DI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])

 which looks inconsistent, and not like an improvement.
>>>
>>> Hmm,
>>> the memory reference does point to a_2(D)->c+-4 so I would say it is OK.  
>>> The
>>> memory refernece is not adjusted yet to be reg87+4 and this is where memory
>>> attributes got lost for me (because the pointer becames out of range of 
>>> (mem:HI
>>> (reg87))).  I see this does not happen on x86_64, I will try to see why
>>> tomorrow.
>>
>> I think if you don't go the bitfield path nothing ends up chaning the
>> mode.  The inconsistency is for MEM_SIZE vs. GET_MODE of the MEM.
>>
>> mem:DI certainly looks wrong for a HImode access.
>>
>> Richard.
>>
> 

Re: Do not drom MEM_EXPR when accessing structure fields

[Bernd Edlinger]

On 09/19/16 11:28, Richard Biener wrote:
> On Sun, 18 Sep 2016, Jan Hubicka wrote:
>
>>> Hi,
>>>
>>>   > when expanding code for
>>>   > struct a {short a,b,c,d;} *a;
>>>   > a->c;
>>>   >
>>>   > we first produce correct BLKmode MEM rtx representing the whole
>>>   > structure *a,
>>>   > then we use adjust_address to turn it into HImode MEM and finally
>>>   > extract_bitfield is used to offset it by 32 bits to get correct value.
>>>
>>> I tried to create a test case as follows and stepped thru the code.
>>>
>>> cat test.c
>>> struct a {short a,b,c,d;};
>>> void foo (struct a *a)
>>> {
>>> a->c = 1;
>>> }
>>>
>>>
>>> First I get a DImode MEM rtx not BLKmode:
>>>
>>> (mem:DI (reg/f:DI 87) [2 *a_2(D)+0 S8 A16])
>>>
>>> and adjust_address does not clear the MEM_EXPR
>
> it's only cleared when later offsetted
>
>>> thus to_rtx = adjust_address (to_rtx, mode1, 0) returns:
>>>
>>> (mem:HI (reg/f:DI 87) [2 *a_2(D)+0 S2 A16])
>>>
>>> and then set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos) does:
>>>
>>> (mem:HI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
>>>
>>> which looks perfectly OK.
>
> Sure you quoted the correct RTL?  MEM_SIZE == 6 looks wrong.  Note
> we don't do set_mem_attributes_minus_bitpos when we go the bitfield
> extraction path.
>

I think it is correct, to_rtx points 4 bytes before a->c "+-4", and
sizeof(a->c) is 2. So 4+2 is 6 there will be no access beyond that.

And yes the RTL is correct: this is what I did:

(gdb) n
5152  to_rtx = shallow_copy_rtx (to_rtx);
(gdb) call debug(to_rtx)
(mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
(gdb) n
5153  set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
(gdb) call debug(to_rtx)
(mem:HI (reg/v/f:DI 87 [ a ]) [1 *a_2(D)+0 S2 A16])
(gdb) n
5154  if (volatilep)
(gdb) call debug(to_rtx)
(mem:HI (reg/v/f:DI 87 [ a ]) [2 a_2(D)->c+-4 S6 A16])
(gdb)


It is not about bitfield extraction, as we are in expand_assignment.
next is the call to optimize_bitfield_assignment_op and
store_field.

I believe, that with Jan's patch we have at this point only
a different mode on the to_rtx.  And it is possible that
store_field or store_bit_field takes a different decision
dependent on GET_MODE(to_rtx), which may accidentally delete
the MEM_EXPR.


Bernd.

>>>
>>> But with your patch the RTX looks different:
>>>
>>> (mem:DI (reg/f:DI 87) [3 a_2(D)->c+-4 S6 A16])
>>>
>>> which looks inconsistent, and not like an improvement.
>>
>> Hmm,
>> the memory reference does point to a_2(D)->c+-4 so I would say it is OK.  The
>> memory refernece is not adjusted yet to be reg87+4 and this is where memory
>> attributes got lost for me (because the pointer becames out of range of 
>> (mem:HI
>> (reg87))).  I see this does not happen on x86_64, I will try to see why
>> tomorrow.
>
> I think if you don't go the bitfield path nothing ends up chaning the
> mode.  The inconsistency is for MEM_SIZE vs. GET_MODE of the MEM.
>
> mem:DI certainly looks wrong for a HImode access.
>
> Richard.
>

Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[Richard Biener]

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 then
 materializing the associated ops via rewrite_expr_tree.

>>> Here is a draft patch which records the stmt chain when in
>>> zero_one_operation and then fixes it when OP is removed. when we
>>> update *def, that will update the ops vector. Does this looks sane?
>>
>>
>> Yes.  A few comments below
>>
>> +  /* PR72835 - Record the stmt chain that has to be updated such that
>> + we dont 

Re: [PATCH 2/2][AArch64] Add missing support for poly64x1_t

[James Greenhalgh]

On Mon, Sep 19, 2016 at 03:22:39PM +0200, Christophe Lyon wrote:
> Hi,
> 
> 
> On 19 September 2016 at 10:39, Tamar Christina  
> wrote:
> >
> >
> > On 17/09/16 07:20, James Greenhalgh wrote:
> >
> >
> > Hi James,
> >
> >> The convention when fixing a bugzilla is to place a line in your ChangeLog
> >> which looks like:
> >>
> >> PR component/x
> >
> > Oh, I wasn't aware of this, thanks! I'll add it.
> >>
> >>  FAIL: gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c   -O0  (test
> >> for excess errors)
> >>  Excess errors:
> >>
> >> .../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c:171:5:
> >> warning: implicit declaration of function 'vmov_n_p64'; did you mean
> >> 'vmov_n_u64'? [-Wimplicit-function
> >
> >
> > It looks like ARM is missing some of the poly64 functions. I can add these
> > since they don't seem to be too many.
> >
> I had opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71233 to track 
> missing
> poly64 intrinsics for the ARM target. Not sure why vmov_n_p64 isn't in the 
> list?

Now you mention it, I don't see vmov_n_p64 in the list of intrinsics at:

  
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0073a/IHI0073A_arm_neon_intrinsics_ref.pdf

So I wonder whether this patch would actually add some extra intrinsics
that we don't require?

Thanks,
James

Re: [PATCH 2/2][AArch64] Add missing support for poly64x1_t

[Christophe Lyon]

Hi,


On 19 September 2016 at 10:39, Tamar Christina  wrote:
>
>
> On 17/09/16 07:20, James Greenhalgh wrote:
>
>
> Hi James,
>
>> The convention when fixing a bugzilla is to place a line in your ChangeLog
>> which looks like:
>>
>> PR component/x
>
> Oh, I wasn't aware of this, thanks! I'll add it.
>>
>>  FAIL: gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c   -O0  (test
>> for excess errors)
>>  Excess errors:
>>
>> .../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c:171:5:
>> warning: implicit declaration of function 'vmov_n_p64'; did you mean
>> 'vmov_n_u64'? [-Wimplicit-function
>
>
> It looks like ARM is missing some of the poly64 functions. I can add these
> since they don't seem to be too many.
>
I had opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71233 to track missing
poly64 intrinsics for the ARM target. Not sure why vmov_n_p64 isn't in the list?

> Regards,
> Tamar
>

Re: [RFC][IPA-VRP] Early VRP Implementation

[Richard Biener]

On Sun, Sep 18, 2016 at 10:50 PM, kugan
 wrote:
> Hi Richard,
>
>
>
> On 16/09/16 20:21, Richard Biener wrote:
>>
>> On Fri, Sep 16, 2016 at 7:59 AM, kugan
>>  wrote:
>>>
>>> Hi Richard,
>>>
>>> Thanks for the review.
>>>
>>> On 14/09/16 22:04, Richard Biener wrote:


 On Tue, Aug 23, 2016 at 4:11 AM, Kugan Vivekanandarajah
  wrote:
>
>
> Hi,
>
> On 19 August 2016 at 21:41, Richard Biener 
> wrote:
>>
>>
>> On Tue, Aug 16, 2016 at 9:45 AM, kugan
>>  wrote:
>>>
>>>
>>> Hi Richard,
>>>
>>>
>>>
>>> I am now having -ftree-evrp which is enabled all the time. But This
>>> will
>>> only be used for disabling the early-vrp. That is, early-vrp will be
>>> run
>>> when ftree-vrp is enabled and ftree-evrp is not explicitly disabled.
>>> Is
>>> this
>>> OK?
>>
>>
>>
>> Why would one want to disable early-vrp?  I see you do this in the
>> testsuite
>> for non-early VRP unit-tests but using -fdisable-tree-evrp1 there
>> would be ok as well.
>
>
>
> Removed it altogether. I though that you wanted a way to disable
> early-vrp for testing purposes.



 But there is via the generic -fdisable-tree-DUMPFILE way.
>>>
>>>
>>>
>>> OK. I didnt know about that.
>>>
>>>
>> Note that you want to have a custom valueize function instead of just
>> follow_single_use_edges as you want to valueize all SSA names
>> according
>> to their lattice value (if it has a single value).  You can use
>> vrp_valueize
>> for this though that gets you non-single-use edge following as well.
>> Eventually it's going to be cleaner to do what the SSA propagator does
>> and
>> before folding do
>>
>>did_replace = replace_uses_in (stmt, vrp_valueize);
>>if (fold_stmt (, follow_single_use_edges)
>>|| did_replace)
>>  update_stmt (gsi_stmt (gsi));
>>
>> exporting replace_uses_in for this is ok.  I guess I prefer this for
>> now.
>
>
>
> I also added the above.  I noticed that I need
> recompute_tree_invariant_for_addr_expr as in ssa_propagate. My initial
> implementation also had gimple_purge_all_dead_eh_edges and
> fixup_noreturn_call as in ssa_propagat but I thinj that is not needed
> as it would be done at the end of the pass.



 I don't see this being done at the end of the pass.  So please
 re-instantiate
 that parts.
>>>
>>>
>>>
>>> I have copied these part as well.
>>>
> With this I noticed more stmts are folded before vrp1. This required
> me to adjust some testcases.
>
>>
>> Overall this now looks good apart from the folding and the
>> VR_INITIALIZER thing.
>>
>> You can commit the already approved refactoring changes and combine
>> this
>> patch with the struct value_range move, this way I can more easily
>> look
>> into
>> issues with the UNDEFINED thing if you can come up with a testcase
>> that
>> doesn't work.
>>
>
> I have now committed all the dependent patches.
>
> Attached patch passes regression and bootstrap except pr33738.C. This
> is an unrelated issue as discussed in
> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01386.html
>
> Is this OK?



 +/* Initialize local data structures for VRP.  If DOM_P is true,
 +   we will be calling this from early_vrp where value range propagation
 +   is done by visiting stmts in dominator tree.  ssa_propagate engine
 +   is not used in this case and that part of the ininitialization will
 +   be skipped.  */
 +
 +static void
 +vrp_initialize ()

 comment needs updating now.

>>> Done.
>>>

  static void
 -extract_range_from_phi_node (gphi *phi, value_range *vr_result)
 +extract_range_from_phi_node (gphi *phi, value_range *vr_result,
 +bool early_vrp_p)
  {


 I don't think you need this changes now that you have
 stmt_visit_phi_node_in_dom_p
 guarding its call.
>>>
>>>
>>>
>>> OK removed it. That also mean I had to put scev_* in the early_vrp.
>>>
>>>
>>>
 +static bool
 +stmt_visit_phi_node_in_dom_p (gphi *phi)
 +{
 +  ssa_op_iter iter;
 +  use_operand_p oprnd;
 +  tree op;
 +  value_range *vr;
 +  FOR_EACH_PHI_ARG (oprnd, phi, iter, SSA_OP_USE)
 +{
 +  op = USE_FROM_PTR (oprnd);
 +  if (TREE_CODE (op) == SSA_NAME)
 +   {
 + vr = get_value_range (op);
 + if (vr->type == VR_UNDEFINED)
 +   return false;
 +   }
 +}

 I think this is overly conservative in never allowing UNDEFINED on PHI
 

Re: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types (was: Advice sought for debugging a lto1 ICE (was: Implement C _FloatN, _FloatNx types [version 6]))

[Joseph Myers]

On Mon, 19 Sep 2016, Thomas Schwinge wrote:

> > The question is whether such a complex type could be a global tree which I
> > don't think it could.
> 
> Specifically, my question was whether for every complex type that is part
> of the global trees, it holds that the complex type's component type also
> is part of the global trees?

That should definitely be the case (but there might be (a) complex types 
that aren't part of those trees, whose components aren't part either, and 
(b) non-complex types that are part of the global trees, whose 
corresponding complex types exist but aren't part of the global trees).

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

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

[Jonathan Wakely]

As noted in bugzilla PR 77641 I don't think is_literal_type is the
right condition for _Uninitialized, because a type can have a
non-trivial default constructor but still be literal, but that means
it can't be used in the union in _Variant_storage.

I'm not sure if the right condition is is_literal &&
is_trivially_default_constructible, or if this is enough:

PR libstdc++/77641
* include/std/variant (__detail::__variant::_Uninitialized): Check
is_trivially_default_constructible_v instead of is_literal_type_v.
* testsuite/20_util/variant/compile.cc: Test literal type with
non-trivial default constructor.

Tim, are there case that this doesn't handle, where we need is_literal
as well? (bear in mind that is_trivially_default_constructible also
depends on trivially destructible).

Tested powerpc64le-linux with no failures.

commit 0d5f60751145f81d914d7411e0f4d79546e06fed
Author: Jonathan Wakely 
Date:   Mon Sep 19 13:18:07 2016 +0100

libstdc++/77641 fix std::variant for literal types

PR libstdc++/77641
* include/std/variant (__detail::__variant::_Uninitialized): Check
is_trivially_default_constructible_v instead of is_literal_type_v.
* testsuite/20_util/variant/compile.cc: Test literal type with
non-trivial default constructor.

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 7dbb533..6c090f6 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -168,7 +168,7 @@ namespace __variant
   template
 using __storage = typename __storage_type<_Type>::type;
 
-  template>
+  template>
 struct _Uninitialized;
 
   template
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc 
b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index b57d356..3042a2e 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -403,3 +403,12 @@ void test_void()
   v = 3;
   v = "asdf";
 }
+
+void test_pr77641()
+{
+  struct X {
+constexpr X() { }
+  };
+
+  std::variant v1 = X{};
+}

Re: Do not drom MEM_EXPR when accessing structure fields

[Bernd Schmidt]

On 09/18/2016 02:51 PM, Jan Hubicka wrote:


   to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
-
-  /* If the field has a mode, we want to access it in the
-field's mode, not the computed mode.
-If a MEM has VOIDmode (external with incomplete type),
-use BLKmode for it instead.  */
-  if (MEM_P (to_rtx))
-   {
- if (mode1 != VOIDmode)
-   to_rtx = adjust_address (to_rtx, mode1, 0);
- else if (GET_MODE (to_rtx) == VOIDmode)
-   to_rtx = adjust_address (to_rtx, BLKmode, 0);
-   }

   if (offset != 0)
{


This was added by Jakub in r166603, referencing PR46388. Have you looked 
at that?



Bernd

[PATCH] Fix PR c++/77639 (ICE during error recovery)

[Patrick Palka]

Some syntactically invalid code can confuse the compiler into trying to
define a specialization while processing_template_parmlist != 0, during
which there is a dummy parameter level (a TREE_VEC of length 0) on
current_template_parms which the TEMPLATE_DECL of the specialization
then inherits.  This dummy parameter level is expected to appear only on
the template parms of a TEMPLATE_TEMPLATE_PARM so its appearance on a
TEMPLATE_DECL (of the bogus specialization) eventually causes an ICE.
>From what I understand, a template specialization can not possibly be
defined from inside a template parameter list so this patch avoids the
ICE by avoiding to generate the bogus specialization in the first place.
Does this look OK to commit after bootstrap + regtesting?

gcc/cp/ChangeLog:

PR c++/77639
* pt.c (maybe_process_partial_specialization): Don't try
defining a specialization when processing_template_parmlist.

gcc/testsuite/ChangeLog:

PR c++/77639
* g++.dg/template/error-recovery4.C: New test.
---
 gcc/cp/pt.c | 8 
 gcc/testsuite/g++.dg/template/error-recovery4.C | 5 +
 2 files changed, 13 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/template/error-recovery4.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 29d8beb..c1b3d5b 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -962,6 +962,14 @@ maybe_process_partial_specialization (tree type)
 
   if (tree t = maybe_new_partial_specialization (type))
{
+ if (processing_template_parmlist)
+   {
+/* Some syntactically invalid code can confuse the compiler into
+   defining a specialization from inside a template parameter
+   list.  Bail out now so that we won't ICE later.  */
+ gcc_assert (seen_error ());
+ return error_mark_node;
+   }
  if (!check_specialization_namespace (CLASSTYPE_TI_TEMPLATE (t))
  && !at_namespace_scope_p ())
return error_mark_node;
diff --git a/gcc/testsuite/g++.dg/template/error-recovery4.C 
b/gcc/testsuite/g++.dg/template/error-recovery4.C
new file mode 100644
index 000..1725d87
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/error-recovery4.C
@@ -0,0 +1,5 @@
+// PR c++/77639
+
+template  struct B {};
+template  {}; // { dg-error 
"" }
+B i;
-- 
2.10.0.87.g1cd26dd

Re: [PATCH 0/9] RFC: selftests based on RTL dumps

[Bernd Schmidt]

On 09/16/2016 11:12 PM, David Malcolm wrote:

There are some interrelated questions here:
(a) where do the dumps live? (string fragments embedded in the source
file vs external files)
(b) -fself-test vs DejaGnu tests that use a real frontend.  In the
latter case, is the frontend "rtl1", or an extension of "cc1" with an
"__RTL" marker?


I think a rtl1 frontend that gets run from a specific subdirectory in 
testsuite/ is probably the best option.



For (a), I'd like to do support both (in that it's clear we need
support for external files, but it seems trivial to support embedding).


If we're dealing with small snippets, I'm not sure embedding them as 
strings is really that much better than building them up with gen_ 
functions. I'm sure there's room for rtl selftests living inside the 
compiler like that, but anything larger probably ought to live outside.



Bernd

Re: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types (was: Advice sought for debugging a lto1 ICE (was: Implement C _FloatN, _FloatNx types [version 6]))

[Richard Biener]

On Mon, Sep 19, 2016 at 1:19 PM, Thomas Schwinge
 wrote:
> Hi!
>
> On Mon, 19 Sep 2016 10:18:35 +0200, Richard Biener 
>  wrote:
>> On Fri, Sep 16, 2016 at 3:32 PM, Thomas Schwinge
>>  wrote:
>> > --- gcc/tree-core.h
>> > +++ gcc/tree-core.h
>> > @@ -553,20 +553,6 @@ enum tree_index {
>> >TI_BOOLEAN_FALSE,
>> >TI_BOOLEAN_TRUE,
>> >
>> > -  TI_COMPLEX_INTEGER_TYPE,
>> > -[...]
>> > -  TI_COMPLEX_FLOAT128X_TYPE,
>> > -
>> >TI_FLOAT_TYPE,
>> >TI_DOUBLE_TYPE,
>> >TI_LONG_DOUBLE_TYPE,
>> > @@ -596,6 +582,23 @@ enum tree_index {
>> >  - TI_FLOATN_NX_TYPE_FIRST  \
>> >  + 1)
>> >
>> > +  /* Put the complex types after their component types, so that in 
>> > (sequential)
>> > + tree streaming we can assert that their component types have already 
>> > been
>> > + handled (see tree-streamer.c:record_common_node).  */
>> > +  TI_COMPLEX_INTEGER_TYPE,
>> > +  TI_COMPLEX_FLOAT_TYPE,
>> > +  TI_COMPLEX_DOUBLE_TYPE,
>> > +  TI_COMPLEX_LONG_DOUBLE_TYPE,
>> > +
>> > +  TI_COMPLEX_FLOAT16_TYPE,
>> > +  TI_COMPLEX_FLOATN_NX_TYPE_FIRST = TI_COMPLEX_FLOAT16_TYPE,
>> > +  TI_COMPLEX_FLOAT32_TYPE,
>> > +  TI_COMPLEX_FLOAT64_TYPE,
>> > +  TI_COMPLEX_FLOAT128_TYPE,
>> > +  TI_COMPLEX_FLOAT32X_TYPE,
>> > +  TI_COMPLEX_FLOAT64X_TYPE,
>> > +  TI_COMPLEX_FLOAT128X_TYPE,
>> > +
>> >TI_FLOAT_PTR_TYPE,
>> >TI_DOUBLE_PTR_TYPE,
>> >TI_LONG_DOUBLE_PTR_TYPE,
>>
>> If the above change alone fixes your issues then it is fine to commit.
>
> That alone won't fix the problem, because we'd still have the recursion
> in gcc/tree-streamer.c:record_common_node done differently for x86_64
> target and nvptx offload target.

Doh - obviously.

>> > --- gcc/tree-streamer.c
>> > +++ gcc/tree-streamer.c
>> > @@ -278,9 +278,23 @@ record_common_node (struct streamer_tree_cache_d 
>> > *cache, tree node)
>> >streamer_tree_cache_append (cache, node, cache->nodes.length ());
>> >
>> >if (POINTER_TYPE_P (node)
>> > -  || TREE_CODE (node) == COMPLEX_TYPE
>> >|| TREE_CODE (node) == ARRAY_TYPE)
>> >  record_common_node (cache, TREE_TYPE (node));
>> > +  else if (TREE_CODE (node) == COMPLEX_TYPE)
>> > +{
>> > +  /* Assert that complex types' component types have already been 
>> > handled
>> > +(and we thus don't need to recurse here).  See PR lto/77458.  */
>> > +  if (cache->node_map)
>> > +   gcc_assert (streamer_tree_cache_lookup (cache, TREE_TYPE (node), 
>> > NULL));
>> > +  else
>> > +   {
>> > + gcc_assert (cache->nodes.exists ());
>> > + bool found = false;
>> > + for (unsigned i = 0; !found && i < cache->nodes.length (); ++i)
>> > +   found = true;
>>
>> hmm, this doesn't actually test anything? ;)
>
> ;-) Haha, hooray for patch review!
>
>> > + gcc_assert (found);
>> > +   }
>> > +}
>> >else if (TREE_CODE (node) == RECORD_TYPE)
>> >  {
>> >/* The FIELD_DECLs of structures should be shared, so that every
>
>> So I very much like to go forward with this kind of change as well
>
> OK, good.  So, in plain text, we'll make it a requirement that:
> integer_types trees must only refer to earlier integer_types trees;
> sizetype_tab trees must only refer to integer_types trees, and earlier
> sizetype_tab trees; and global_trees must only refer to integer_types
> trees, sizetype_tab trees, and earlier global_trees.

Yeah, though I'd put sizetypes first.

>> (the assert code
>> should go to a separate helper function).
>
> Should this checking be done only in
> gcc/tree-streamer.c:record_common_node, or should generally

Yes.

> gcc/tree-streamer.c:streamer_tree_cache_append check/assert that such
> recursive trees are already present in the cache?  And generally do that,
> or "if (flag_checking)" only?

I think we should restrict it to flag_checking only because in general violating
it is harmless plus we never know what happens on all targets/frontend/flag(!)
combinations.

>
>> Did you try it on more than just
>> the complex type case?
>
> Not yet, but now that you have approved the general concept, I'll look
> into that.
>
> Here's the current patch with the assertion condition fixed, but still
> for complex types only.  OK for trunk already?

Ok with the checking blob moved to a helper function,

  bool common_node_recorded_p (cache, node)

and its body guarded with if (flag_checking).

[looks to me we miss handling of vector type components alltogether,
maybe there are no global vector type trees ...]

Thanks,
Richard.

> --- gcc/tree-core.h
> +++ gcc/tree-core.h
> @@ -553,20 +553,6 @@ enum tree_index {
>TI_BOOLEAN_FALSE,
>TI_BOOLEAN_TRUE,
>
> -  TI_COMPLEX_INTEGER_TYPE,
> -  TI_COMPLEX_FLOAT_TYPE,
> -  TI_COMPLEX_DOUBLE_TYPE,
> -  TI_COMPLEX_LONG_DOUBLE_TYPE,
> -
> -  TI_COMPLEX_FLOAT16_TYPE,
> -  TI_COMPLEX_FLOATN_NX_TYPE_FIRST = 

Re: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types (was: Advice sought for debugging a lto1 ICE (was: Implement C _FloatN, _FloatNx types [version 6]))

[Thomas Schwinge]

Hi!

On Mon, 19 Sep 2016 10:18:35 +0200, Richard Biener  
wrote:
> On Fri, Sep 16, 2016 at 3:32 PM, Thomas Schwinge
>  wrote:
> > --- gcc/tree-core.h
> > +++ gcc/tree-core.h
> > @@ -553,20 +553,6 @@ enum tree_index {
> >TI_BOOLEAN_FALSE,
> >TI_BOOLEAN_TRUE,
> >
> > -  TI_COMPLEX_INTEGER_TYPE,
> > -[...]
> > -  TI_COMPLEX_FLOAT128X_TYPE,
> > -
> >TI_FLOAT_TYPE,
> >TI_DOUBLE_TYPE,
> >TI_LONG_DOUBLE_TYPE,
> > @@ -596,6 +582,23 @@ enum tree_index {
> >  - TI_FLOATN_NX_TYPE_FIRST  \
> >  + 1)
> >
> > +  /* Put the complex types after their component types, so that in 
> > (sequential)
> > + tree streaming we can assert that their component types have already 
> > been
> > + handled (see tree-streamer.c:record_common_node).  */
> > +  TI_COMPLEX_INTEGER_TYPE,
> > +  TI_COMPLEX_FLOAT_TYPE,
> > +  TI_COMPLEX_DOUBLE_TYPE,
> > +  TI_COMPLEX_LONG_DOUBLE_TYPE,
> > +
> > +  TI_COMPLEX_FLOAT16_TYPE,
> > +  TI_COMPLEX_FLOATN_NX_TYPE_FIRST = TI_COMPLEX_FLOAT16_TYPE,
> > +  TI_COMPLEX_FLOAT32_TYPE,
> > +  TI_COMPLEX_FLOAT64_TYPE,
> > +  TI_COMPLEX_FLOAT128_TYPE,
> > +  TI_COMPLEX_FLOAT32X_TYPE,
> > +  TI_COMPLEX_FLOAT64X_TYPE,
> > +  TI_COMPLEX_FLOAT128X_TYPE,
> > +
> >TI_FLOAT_PTR_TYPE,
> >TI_DOUBLE_PTR_TYPE,
> >TI_LONG_DOUBLE_PTR_TYPE,
> 
> If the above change alone fixes your issues then it is fine to commit.

That alone won't fix the problem, because we'd still have the recursion
in gcc/tree-streamer.c:record_common_node done differently for x86_64
target and nvptx offload target.

> > --- gcc/tree-streamer.c
> > +++ gcc/tree-streamer.c
> > @@ -278,9 +278,23 @@ record_common_node (struct streamer_tree_cache_d 
> > *cache, tree node)
> >streamer_tree_cache_append (cache, node, cache->nodes.length ());
> >
> >if (POINTER_TYPE_P (node)
> > -  || TREE_CODE (node) == COMPLEX_TYPE
> >|| TREE_CODE (node) == ARRAY_TYPE)
> >  record_common_node (cache, TREE_TYPE (node));
> > +  else if (TREE_CODE (node) == COMPLEX_TYPE)
> > +{
> > +  /* Assert that complex types' component types have already been 
> > handled
> > +(and we thus don't need to recurse here).  See PR lto/77458.  */
> > +  if (cache->node_map)
> > +   gcc_assert (streamer_tree_cache_lookup (cache, TREE_TYPE (node), 
> > NULL));
> > +  else
> > +   {
> > + gcc_assert (cache->nodes.exists ());
> > + bool found = false;
> > + for (unsigned i = 0; !found && i < cache->nodes.length (); ++i)
> > +   found = true;
> 
> hmm, this doesn't actually test anything? ;)

;-) Haha, hooray for patch review!

> > + gcc_assert (found);
> > +   }
> > +}
> >else if (TREE_CODE (node) == RECORD_TYPE)
> >  {
> >/* The FIELD_DECLs of structures should be shared, so that every

> So I very much like to go forward with this kind of change as well

OK, good.  So, in plain text, we'll make it a requirement that:
integer_types trees must only refer to earlier integer_types trees;
sizetype_tab trees must only refer to integer_types trees, and earlier
sizetype_tab trees; and global_trees must only refer to integer_types
trees, sizetype_tab trees, and earlier global_trees.

> (the assert code
> should go to a separate helper function).

Should this checking be done only in
gcc/tree-streamer.c:record_common_node, or should generally
gcc/tree-streamer.c:streamer_tree_cache_append check/assert that such
recursive trees are already present in the cache?  And generally do that,
or "if (flag_checking)" only?

> Did you try it on more than just
> the complex type case?

Not yet, but now that you have approved the general concept, I'll look
into that.

Here's the current patch with the assertion condition fixed, but still
for complex types only.  OK for trunk already?

--- gcc/tree-core.h
+++ gcc/tree-core.h
@@ -553,20 +553,6 @@ enum tree_index {
   TI_BOOLEAN_FALSE,
   TI_BOOLEAN_TRUE,
 
-  TI_COMPLEX_INTEGER_TYPE,
-  TI_COMPLEX_FLOAT_TYPE,
-  TI_COMPLEX_DOUBLE_TYPE,
-  TI_COMPLEX_LONG_DOUBLE_TYPE,
-
-  TI_COMPLEX_FLOAT16_TYPE,
-  TI_COMPLEX_FLOATN_NX_TYPE_FIRST = TI_COMPLEX_FLOAT16_TYPE,
-  TI_COMPLEX_FLOAT32_TYPE,
-  TI_COMPLEX_FLOAT64_TYPE,
-  TI_COMPLEX_FLOAT128_TYPE,
-  TI_COMPLEX_FLOAT32X_TYPE,
-  TI_COMPLEX_FLOAT64X_TYPE,
-  TI_COMPLEX_FLOAT128X_TYPE,
-
   TI_FLOAT_TYPE,
   TI_DOUBLE_TYPE,
   TI_LONG_DOUBLE_TYPE,
@@ -596,6 +582,23 @@ enum tree_index {
 - TI_FLOATN_NX_TYPE_FIRST  \
 + 1)
 
+  /* Put the complex types after their component types, so that in (sequential)
+ tree streaming we can assert that their component types have already been
+ handled (see tree-streamer.c:record_common_node).  */
+  TI_COMPLEX_INTEGER_TYPE,
+  TI_COMPLEX_FLOAT_TYPE,
+  TI_COMPLEX_DOUBLE_TYPE,
+  TI_COMPLEX_LONG_DOUBLE_TYPE,
+
+  TI_COMPLEX_FLOAT16_TYPE,
+  

Re: [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict

[Tom de Vries]

On 19/09/16 13:02, Szabolcs Nagy wrote:

On 29/08/16 17:12, Tom de Vries wrote:

> On 29/08/16 17:51, Joseph Myers wrote:

>> On Wed, 24 Aug 2016, Tom de Vries wrote:
>>

>>> This patch fixes PR71602 by making canonical_va_list_type more strict.

> 2016-08-22  Tom de Vries  
>
>PR C/71602
>* builtins.c (std_canonical_va_list_type): Strictly return non-null for
>va_list type only.

this does not seem to be true.


> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index abc934b..101b1e3 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -4089,10 +4089,6 @@ std_canonical_va_list_type (tree type)
>  {
>tree wtype, htype;
>
> -  if (INDIRECT_REF_P (type))
> -type = TREE_TYPE (type);
> -  else if (POINTER_TYPE_P (type) && POINTER_TYPE_P (TREE_TYPE (type)))
> -type = TREE_TYPE (type);
>wtype = va_list_type_node;
>htype = type;
>/* Treat structure va_list types.  */

here the code follows as

if (TREE_CODE (wtype) == RECORD_TYPE && POINTER_TYPE_P (htype))
  htype = TREE_TYPE (htype);
else if (TREE_CODE (wtype) == ARRAY_TYPE)
  [...]
if (TYPE_MAIN_VARIANT (wtype) == TYPE_MAIN_VARIANT (htype))
  return va_list_type_node;

return NULL_TREE;

i think va_list* is accepted as va_list in the
first check if it is a struct type.

i think this check is not needed and causes
problems on arm and aarch64 (where va_list is
a struct type).



Yes, that's a known issue: PR77558 - [6/7 regression] 
c-c++-common/va-arg-va-list-type.c fails for arm/aarch64.


Thanks,
- Tom

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77558

Re: [PATCH, PR71602, 4/4] Make canonical_va_list_type more strict

[Szabolcs Nagy]

On 29/08/16 17:12, Tom de Vries wrote:
> On 29/08/16 17:51, Joseph Myers wrote:
>> On Wed, 24 Aug 2016, Tom de Vries wrote:
>>
>>> This patch fixes PR71602 by making canonical_va_list_type more strict.

> 2016-08-22  Tom de Vries  
> 
>   PR C/71602
>   * builtins.c (std_canonical_va_list_type): Strictly return non-null for
>   va_list type only.

this does not seem to be true.

> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index abc934b..101b1e3 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -4089,10 +4089,6 @@ std_canonical_va_list_type (tree type)
>  {
>tree wtype, htype;
>  
> -  if (INDIRECT_REF_P (type))
> -type = TREE_TYPE (type);
> -  else if (POINTER_TYPE_P (type) && POINTER_TYPE_P (TREE_TYPE (type)))
> -type = TREE_TYPE (type);
>wtype = va_list_type_node;
>htype = type;
>/* Treat structure va_list types.  */

here the code follows as

if (TREE_CODE (wtype) == RECORD_TYPE && POINTER_TYPE_P (htype))
  htype = TREE_TYPE (htype);
else if (TREE_CODE (wtype) == ARRAY_TYPE)
  [...]
if (TYPE_MAIN_VARIANT (wtype) == TYPE_MAIN_VARIANT (htype))
  return va_list_type_node;

return NULL_TREE;

i think va_list* is accepted as va_list in the
first check if it is a struct type.

i think this check is not needed and causes
problems on arm and aarch64 (where va_list is
a struct type).

Re: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types (was: Advice sought for debugging a lto1 ICE (was: Implement C _FloatN, _FloatNx types [version 6]))

[Thomas Schwinge]

Hi!

On Mon, 19 Sep 2016 10:12:48 +0200, Richard Biener  
wrote:
> On Fri, Sep 16, 2016 at 7:07 PM, Joseph Myers  wrote:
> > On Fri, 16 Sep 2016, Thomas Schwinge wrote:
> >
> >> That's what I was afraid of: for example, I can't tell if it holds for
> >> all GCC configurations (back ends), that complex types' component types
> >> will always match one of the already existing global trees?  (I can
> >
> > Well, a component type could certainly match a target-specific type
> > instead (e.g. __ibm128 on powerpc, which if it's not long double won't be
> > any of the other types either).  That's a type registered with
> > lang_hooks.types.register_builtin_type, not one of the global trees.
> > (You can't write _Complex __ibm128, but can get such a type with _Complex
> > float __attribute__ ((__mode__ (__IC__))).  Or similarly, with ARM __fp16,
> > the complex type _Complex float __attribute__ ((__mode__ (__HC__))).)
> 
> The question is whether such a complex type could be a global tree which I
> don't think it could.

Specifically, my question was whether for every complex type that is part
of the global trees, it holds that the complex type's component type also
is part of the global trees?


Grüße
 Thomas

Re: [PATCH][Libiberty] Support empty arguments in pex-win32

[Andrew Stubbs]

On 16/09/16 19:12, DJ Delorie wrote:

This is OK.  Thanks!


Thanks, committed.

Andrew

Re: [PATCH] Allow FP to be used as a call-saved registe

[Richard Earnshaw (lists)]

On 15/09/16 17:36, Jeff Law wrote:
> On 09/13/2016 05:10 AM, Tamar Christina wrote:
>> Hi Jeff,
>>
>>
>> On 12/09/16 18:16, Jeff Law wrote:
>>> On 09/05/2016 08:59 AM, Tamar Christina wrote:
 Hi All,

 This patch allows the FP register to be used as a call-saved
 register when -fomit-frame-pointer is used.

 The change is done in such a way that the defaults do not change.
 To use the FP register both -fomit-frame-pointer and
 -fcall-saved- need to be used.

 Regression ran on aarch64-none-linux-gnu and no regressions.
 Bootstrapped and ran regressions on `x86_64` and no regressions.

 A new test fp_free_1 was added to test functionality.

 Ok for trunk?

 Thanks,
 Tamar

 PS. I don't have commit rights so if OK can someone apply the patch
 for me.

 gcc/
 2016-09-01  Tamar Christina  

 * gcc/reginfo.c (fix_register): Allow FP to be set if
 -fomit-frame-pointer.
>>> I'm a little surprised you need this.  Most ports allow use of FP as a
>>> call-saved register with -fomit-frame-pointer.
>> I think this is because on most architectures the FP is not in the fixed
>> registers list. But the AArch64 ABI (I believe) currently
>> mandates that it is. With the option of:
>>
>> - It may permit the frame pointer register to be used as a
>> general-purpose callee-saved register, but provide a platform-specific
>> mechanism for external agents to reliably detect this condition
>>
>> - It may elect not to maintain a frame chain and to use the frame
>> pointer register as a general-purpose callee-saved register.
> So those don't seem to me to imply that the frame pointer needs to be a
> fixed register.   So the first thing I'd do is fix the aarch64 port to
> not do that and see what fallout there is and how to fix it.
> 
> Most ports simply don't mark the frame pointer as fixed.
> 

The AArch64 ABI specification strongly recommends that, when a frame
record is not created, the frame pointer register is left unused so that
the frame chain, while not complete, is still valid (a chain of valid
records but ending in a NULL pointer).  That strongly suggests that FP
should remain a fixed register.

I guess we could push all of this into a new back-end option to permit
the 'I really want to use FP for general purposes', but it seems to be
just duplicating the existing use of -fcall-saved; so would be yet
another flag in the compiler that needs documenting.

It seems much more sensible to me to just make a slight relaxation of
the fixed-register code and then re-use the existing options.

> I am a bit curious about how you're going to solve the "external agents
> to reliably detect this condition" :-)
> 

:-)  It's a get-out clause to pacify folk who want a completely
different ABI variant.  Reliable detection would probably mean 'a
platform convention' that all code had to conform to.

R.

>>
>>> Also note the documentation explicitly forbids using -fcall-saved for
>>> the stack or frame pointer.
>>>
>> Ah, yes, hadn't noticed that before. Isn't it a bit too strict a
>> restriction? In general if you have -fomit-frame-pointer then shouldn't
>> the it be safe for the FP to be used
>> with -fcall-saved? Since it's probably a no-op on most ports that
>> support -fomit-frame-pointer anyway?
> It might be.  In general I don't think the -fcall-whatever options are
> used that much anymore and I don't think anyone has seriously looked at
> their documentation in a long time.
> 
> Regardless, I still think the first step is to "unfix" the frame pointer
> hard register on the aarch64 port.
> 
> jeff
>