Re: Record equivalences for spill registers

2017-05-07 Thread Andrew Pinski 
On Sun, May 7, 2017 at 9:30 PM, Jim Wilson  wrote:
> On 05/05/2017 12:23 AM, Richard Sandiford wrote:
>>
>> 2017-05-05  Richard Sandiford  
>>
>> gcc/
>> * lra-constraints.c (lra_copy_reg_equiv): New function.
>> (split_reg): Use it to copy equivalence information from the
>> original register to the spill register.
>
>
> This patch breaks aarch64 bootstrap.  I get a link error for lto1 and f951
>
> godump.o: In function `go_define(unsigned int, char const*)':
> godump.c:(.text+0x36c): relocation truncated to fit:
> R_AARCH64_ADR_PREL_LO21 against `.rodata'
> godump.c:(.text+0x4b4): relocation truncated to fit: R_AARCH64_ADR_PREL_LO21
> against `.rodata'
>
> The godump.c.271r.ira file looks OK, I see
>
> (insn 237 223 225 10 (set (reg/f:DI 489)
> (high:DI (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 49
> {*movdi_aarch64}
>  (expr_list:REG_EQUIV (high:DI (label_ref 240))
> (insn_list:REG_LABEL_OPERAND 240 (nil
> ...
> (insn 238 115 1157 10 (set (reg/f:DI 490)
> (lo_sum:DI (reg/f:DI 489)
> (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 929
> {add_losym_di}
>  (expr_list:REG_DEAD (reg/f:DI 489)
> (expr_list:REG_EQUIV (label_ref 240)
> (insn_list:REG_LABEL_OPERAND 240 (nil)
>
> But in the godump.c.272r.reload file I see in a different basic block
>
> (insn 1244 76 1161 22 (set (reg/f:DI 7 x7 [490])
> (label_ref 240)) "../../gcc-svn/gcc/godump.c":221 49
> {*movdi_aarch64}
>  (nil))
>
> which is not OK.  This label ref is the address of a jumptable in the rodata
> section, and can't be loaded with a single instruction.  It looks like there
> needs to be some extra work when rematerializing, to handle equiv values
> that can't just be copied to a register.

Hmm, shouldn't have the mov rejected as being invalid?  I think that
is one issue with the aarch64 backend there.

See https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01800.html


I can't remember if the following patch was ever submitted or committed.

Here are my notes about this patch from the internal bug report we got
here at Cavium (back in 2013):

Switch tables are implemented using the tiny model but they are stored
in the rodata section which means they could overflow the address.

Note this patch most likely won't apply directly either:

From: Andrew Pinski 
Date: Thu, 15 Aug 2013 20:42:12 + (-0700)
Subject: 2013-08-11  Andrew Pinski  
X-Git-Tag: tc47_SDK_3_1_0_build_28~9
X-Git-Url: 
http://cagit1.caveonetworks.com/git/?p=toolchain%2Fgcc.git;a=commitdiff_plain;h=1714f167b575956801264db5cd2300203a58e3e9

2013-08-11  Andrew Pinski  

Bug #7079
* config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.
(*movdi_aarch64): Likewise.
(ldr_got_small_sidi): Add type attribute.
* config/aarch64/constraints.md (Ust): New constraint like S but only
if the symbol is SYMBOL_TINY_ABSOLUTE.
---

diff --git a/gcc/ChangeLog.aarch64 b/gcc/ChangeLog.aarch64
index 3c8ff13..f4e1e91 100644
--- a/gcc/ChangeLog.aarch64
+++ b/gcc/ChangeLog.aarch64
@@ -1,3 +1,12 @@
+2013-08-11  Andrew Pinski  
+
+ Bug #7079
+ * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.
+ (*movdi_aarch64): Likewise.
+ (ldr_got_small_sidi): Add type attribute.
+ * config/aarch64/constraints.md (Ust): New constraint like S but only
+ if the symbol is SYMBOL_TINY_ABSOLUTE.
+
 2013-08-14  Yufeng Zhang  

  * function.c (assign_parm_find_data_types): Set passed_mode and
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 485bd59..0cd6a41 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -810,7 +810,7 @@

 (define_insn "*movsi_aarch64"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,
m,r,r  ,*w, r,*w")
- (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,
m,rZ,*w,S,Ush,rZ,*w,*w"))]
+ (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,
m,rZ,*w,Ust,Ush,rZ,*w,*w"))]
   "(register_operand (operands[0], SImode)
 || aarch64_reg_or_zero (operands[1], SImode))"
   "@
@@ -836,7 +836,7 @@

 (define_insn "*movdi_aarch64"
   [(set (match_operand:DI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,
m,r,r,  *w, r,*w,w")
- (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,
m,rZ,*w,S,Ush,rZ,*w,*w,Dd"))]
+ (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,
m,rZ,*w,Ust,Ush,rZ,*w,*w,Dd"))]
   "(register_operand (operands[0], DImode)
 || aarch64_reg_or_zero (operands[1], DImode))"
   "@
@@ -3978,6 +3978,7 @@
   "TARGET_ILP32"
   "ldr\\t%w0, [%1, #:got_lo12:%a2]"
   [(set_attr "v8type" "load1")
+   (set_attr "type" "load1")
(set_attr "mode" "DI")]
 )

diff --git a/gcc/config/aarch64/constraints.md
b/gcc/config/aarch64/constraints.md
index 2570400..a36bdaf 100644
--- a/gcc/config/aarch64/constraints.md
+++

Re: Record equivalences for spill registers

2017-05-07 Thread Andrew Pinski 
On Sun, May 7, 2017 at 10:26 PM, Andrew Pinski  wrote:
> On Sun, May 7, 2017 at 9:30 PM, Jim Wilson  wrote:
>> On 05/05/2017 12:23 AM, Richard Sandiford wrote:
>>>
>>> 2017-05-05  Richard Sandiford  
>>>
>>> gcc/
>>> * lra-constraints.c (lra_copy_reg_equiv): New function.
>>> (split_reg): Use it to copy equivalence information from the
>>> original register to the spill register.
>>
>>
>> This patch breaks aarch64 bootstrap.  I get a link error for lto1 and f951
>>
>> godump.o: In function `go_define(unsigned int, char const*)':
>> godump.c:(.text+0x36c): relocation truncated to fit:
>> R_AARCH64_ADR_PREL_LO21 against `.rodata'
>> godump.c:(.text+0x4b4): relocation truncated to fit: R_AARCH64_ADR_PREL_LO21
>> against `.rodata'
>>
>> The godump.c.271r.ira file looks OK, I see
>>
>> (insn 237 223 225 10 (set (reg/f:DI 489)
>> (high:DI (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 49
>> {*movdi_aarch64}
>>  (expr_list:REG_EQUIV (high:DI (label_ref 240))
>> (insn_list:REG_LABEL_OPERAND 240 (nil
>> ...
>> (insn 238 115 1157 10 (set (reg/f:DI 490)
>> (lo_sum:DI (reg/f:DI 489)
>> (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 929
>> {add_losym_di}
>>  (expr_list:REG_DEAD (reg/f:DI 489)
>> (expr_list:REG_EQUIV (label_ref 240)
>> (insn_list:REG_LABEL_OPERAND 240 (nil)
>>
>> But in the godump.c.272r.reload file I see in a different basic block
>>
>> (insn 1244 76 1161 22 (set (reg/f:DI 7 x7 [490])
>> (label_ref 240)) "../../gcc-svn/gcc/godump.c":221 49
>> {*movdi_aarch64}
>>  (nil))
>>
>> which is not OK.  This label ref is the address of a jumptable in the rodata
>> section, and can't be loaded with a single instruction.  It looks like there
>> needs to be some extra work when rematerializing, to handle equiv values
>> that can't just be copied to a register.
>
> Hmm, shouldn't have the mov rejected as being invalid?  I think that
> is one issue with the aarch64 backend there.
>
> See https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01800.html
>
>
> I can't remember if the following patch was ever submitted or committed.
>
> Here are my notes about this patch from the internal bug report we got
> here at Cavium (back in 2013):
>
> Switch tables are implemented using the tiny model but they are stored
> in the rodata section which means they could overflow the address.


Some more notes:

(In reply to comment #15)
> (In reply to comment #14)
> > //(insn 793 35 795 (set (reg/f:DI 2 x2 [450])
> > //(label_ref 456)) 32 {*movdi_aarch64}
> > // (insn_list:REG_LABEL_OPERAND 456 (expr_list:REG_EQUAL (label_ref 456)
> > //(nil
> > adr x2, .L806 // 793 *movdi_aarch64/9 [length = 4]
>
> Which is generated by the register allocator and we never split it into the
> adrp/add again.

The register allocator is doing an ok job as the backend said this is
a valid pattern.  We need a constraint for
*movdi_aarch64/*movsi_aarch64 which says this is not a valid pattern
and to go through the standard gen_movinsn path.


Thanks,
Andrew Pinski

>
> Note this patch most likely won't apply directly either:
>
> From: Andrew Pinski 
> Date: Thu, 15 Aug 2013 20:42:12 + (-0700)
> Subject: 2013-08-11  Andrew Pinski  
> X-Git-Tag: tc47_SDK_3_1_0_build_28~9
> X-Git-Url: 
> http://cagit1.caveonetworks.com/git/?p=toolchain%2Fgcc.git;a=commitdiff_plain;h=1714f167b575956801264db5cd2300203a58e3e9
>
> 2013-08-11  Andrew Pinski  
>
> Bug #7079
> * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.
> (*movdi_aarch64): Likewise.
> (ldr_got_small_sidi): Add type attribute.
> * config/aarch64/constraints.md (Ust): New constraint like S but only
> if the symbol is SYMBOL_TINY_ABSOLUTE.
> ---
>
> diff --git a/gcc/ChangeLog.aarch64 b/gcc/ChangeLog.aarch64
> index 3c8ff13..f4e1e91 100644
> --- a/gcc/ChangeLog.aarch64
> +++ b/gcc/ChangeLog.aarch64
> @@ -1,3 +1,12 @@
> +2013-08-11  Andrew Pinski  
> +
> + Bug #7079
> + * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S 
> constrain.
> + (*movdi_aarch64): Likewise.
> + (ldr_got_small_sidi): Add type attribute.
> + * config/aarch64/constraints.md (Ust): New constraint like S but only
> + if the symbol is SYMBOL_TINY_ABSOLUTE.
> +
>  2013-08-14  Yufeng Zhang  
>
>   * function.c (assign_parm_find_data_types): Set passed_mode and
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 485bd59..0cd6a41 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -810,7 +810,7 @@
>
>  (define_insn "*movsi_aarch64"
>[(set (match_operand:SI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,
> m,r,r  ,*w, r,*w")
> - (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,
> m,rZ,*w,S,Ush,rZ,*w,*w"))]
> +

Re: [PATCH] Pretty-printing of some unsupported expressions (PR c/35441)

2017-05-07 Thread Volker Reichelt 
On  2 May, Joseph Myers wrote:
> On Fri, 10 Mar 2017, Volker Reichelt wrote:
> 
>> a) This part (with foo1 and foo2 from the testcase) is straightforward.
> 
> That part is OK.
> 
>> b) I chose the shift operators 'a << b' and 'a >> b' for the rotate
>>expressions, which is not 100% correct. Would it be better to use
>>something like 'lrotate(a, b)', '__lrotate__(a, b)' or 'a lrotate b'
>>instead? Or is there something like an '__builtin_lrotate' that I misseed?
> 
> I'd be inclined to use the notation <<< and >>> for rotation, cf. 
> .

Nice idea. It would be nice to see these operators in some future C/C++
versions. Is the nested ternary operator used in the updated patch below
OK, or would you prefer a switch?

>> c) I chose 'max(q, b)' and 'min(q, b)'.
> 
> I think that's fine.
> 
>> In addition I found some more division operators in gcc/tree.def that
>> aren't handled by the pretty-printer:
>> 
>>   CEIL_DIV_EXPR
>>   FLOOR_DIV_EXPR
>>   ROUND_DIV_EXPR
>>   CEIL_MOD_EXPR
>>   FLOOR_MOD_EXPR
>>   ROUND_MOD_EXPR
>> 
>> Alas I don't have testcases for them. Nevertheless, I could handle them
>> like the other MOD and DIV operators just to be safe.
> 
> These can probably appear from Ada code, but maybe not from C.

OK, I'll ignore them.

> Now we have caret diagnostics and location ranges I think we should be 
> moving away from printing complicated expressions from trees anyway.  So 
> for the diagnostics about calling non-functions, it would be best to make 
> a location range for the called expression available if it isn't already, 
> then do a diagnostic with a location that underlines that text rather than 
> trying to reproduce an expression text from trees.

Indeed, we can do better with caret diagnostics. But non-caret mode is
still there (and has its uses because of its usually more consise form)
and there are probably more places where this expression is printed.
Therefore, I'd like to fix this regardless of a better caret solution
for the diagnostics about calling non-functions.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2017-05-07  Volker Reichelt  

PR c/35441
* c-pretty-print.c (c_pretty_printer::expression): Handle MAX_EXPR,
MIN_EXPR, EXACT_DIV_EXPR, RDIV_EXPR, LROTATE_EXPR, RROTATE_EXPR.
(c_pretty_printer::postfix_expression): Handle MAX_EXPR, MIN_EXPR.
(c_pretty_printer::multiplicative_expression): Handle EXACT_DIV_EXPR,
RDIV_EXPR.
(pp_c_shift_expression): Handle LROTATE_EXPR, RROTATE_EXPR.

Index: gcc/c-family/c-pretty-print.c
===
--- gcc/c-family/c-pretty-print.c   (revision 247727)
+++ gcc/c-family/c-pretty-print.c   (working copy)
@@ -1551,6 +1551,14 @@
   : "__builtin_islessgreater");
   goto two_args_fun;
 
+case MAX_EXPR:
+  pp_c_ws_string (this, "max");
+  goto two_args_fun;
+
+case MIN_EXPR:
+  pp_c_ws_string (this, "min");
+  goto two_args_fun;
+
 two_args_fun:
   pp_c_left_paren (this);
   expression (TREE_OPERAND (e, 0));
@@ -1829,6 +1837,8 @@
 case MULT_EXPR:
 case TRUNC_DIV_EXPR:
 case TRUNC_MOD_EXPR:
+case EXACT_DIV_EXPR:
+case RDIV_EXPR:
   multiplicative_expression (TREE_OPERAND (e, 0));
   pp_c_whitespace (this);
   if (code == MULT_EXPR)
@@ -1890,9 +1900,13 @@
 {
 case LSHIFT_EXPR:
 case RSHIFT_EXPR:
+case LROTATE_EXPR:
+case RROTATE_EXPR:
   pp_c_shift_expression (pp, TREE_OPERAND (e, 0));
   pp_c_whitespace (pp);
-  pp_string (pp, code == LSHIFT_EXPR ? "<<" : ">>");
+  pp_string (pp, code == LSHIFT_EXPR ? "<<" :
+code == RSHIFT_EXPR ? ">>" :
+code == LROTATE_EXPR ? "<<<" : ">>>");
   pp_c_whitespace (pp);
   pp_c_additive_expression (pp, TREE_OPERAND (e, 1));
   break;
@@ -2186,6 +2200,8 @@
 case UNLT_EXPR:
 case UNGE_EXPR:
 case UNGT_EXPR:
+case MAX_EXPR:
+case MIN_EXPR:
 case ABS_EXPR:
 case CONSTRUCTOR:
 case COMPOUND_LITERAL_EXPR:
@@ -2217,11 +2233,15 @@
 case MULT_EXPR:
 case TRUNC_MOD_EXPR:
 case TRUNC_DIV_EXPR:
+case EXACT_DIV_EXPR:
+case RDIV_EXPR:
   multiplicative_expression (e);
   break;
 
 case LSHIFT_EXPR:
 case RSHIFT_EXPR:
+case LROTATE_EXPR:
+case RROTATE_EXPR:
   pp_c_shift_expression (this, e);
   break;
 
===

2017-05-07  Volker Reichelt  

PR c/35441
* gcc.dg/pr35441.c: New test.

Index: gcc/testsuite/gcc.dg/pr35441.c
===
--- gcc/testsuite/gcc.dg/pr35441.c  2017-03-08 18:38:39
+++ gcc/testsuite/gcc.dg/pr35441.c  2017-03-08

Re: Record equivalences for spill registers

2017-05-07 Thread Jim Wilson 

On 05/05/2017 12:23 AM, Richard Sandiford wrote:

2017-05-05  Richard Sandiford  

gcc/
* lra-constraints.c (lra_copy_reg_equiv): New function.
(split_reg): Use it to copy equivalence information from the
original register to the spill register.


This patch breaks aarch64 bootstrap.  I get a link error for lto1 and f951

godump.o: In function `go_define(unsigned int, char const*)':
godump.c:(.text+0x36c): relocation truncated to fit:
R_AARCH64_ADR_PREL_LO21 against `.rodata'
godump.c:(.text+0x4b4): relocation truncated to fit: 
R_AARCH64_ADR_PREL_LO21 against `.rodata'


The godump.c.271r.ira file looks OK, I see

(insn 237 223 225 10 (set (reg/f:DI 489)
(high:DI (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 49 
{*movdi_aarch64}

 (expr_list:REG_EQUIV (high:DI (label_ref 240))
(insn_list:REG_LABEL_OPERAND 240 (nil
...
(insn 238 115 1157 10 (set (reg/f:DI 490)
(lo_sum:DI (reg/f:DI 489)
(label_ref 240))) "../../gcc-svn/gcc/godump.c":174 929 
{add_losym_di}

 (expr_list:REG_DEAD (reg/f:DI 489)
(expr_list:REG_EQUIV (label_ref 240)
(insn_list:REG_LABEL_OPERAND 240 (nil)

But in the godump.c.272r.reload file I see in a different basic block

(insn 1244 76 1161 22 (set (reg/f:DI 7 x7 [490])
(label_ref 240)) "../../gcc-svn/gcc/godump.c":221 49 
{*movdi_aarch64}

 (nil))

which is not OK.  This label ref is the address of a jumptable in the 
rodata section, and can't be loaded with a single instruction.  It looks 
like there needs to be some extra work when rematerializing, to handle 
equiv values that can't just be copied to a register.


I haven't had a chance to step through this in a debugger to see what is 
going on yet.


Jim

Re: [PATCH 2/n] [PR tree-optimization/78496] Simplify ASSERT_EXPRs to facilitate jump threading

2017-05-07 Thread Jeff Law 

On 05/07/2017 09:18 PM, Andrew Pinski wrote:

On Sun, May 7, 2017 at 8:06 AM, Jeff Law  wrote:

On 05/06/2017 05:56 PM, Andrew Pinski wrote:


On Sat, May 6, 2017 at 4:55 PM, Andrew Pinski  wrote:


On Sat, May 6, 2017 at 8:03 AM, Jeff Law  wrote:


This is the 2nd of 3-5 patches to address pr78496.

Jump threading will examine ASSERT_EXPRs as it walks the IL and record
information from those ASSERT_EXPRs into the available expression and
const/copies tables where they're later used to simplify conditionals.

We're missing a couple things though.

First an ASSERT_EXPR with an EQ test creates an equality we can enter
into
the const/copy tables.  We were failing to do that.  This is most
interesting when the RHS of the condition in the ASSERT_EXPR is a
constant,
obviously.  This has a secondary benefit of doing less work to get
better
optimization.

Second, some ASSERT_EXPRs may start off as a relational test such as x
<= 0,
but after range extraction and propagation they could be simplified into
an
equality comparison.  We already do this with conditionals and
generalizing
that code to handle ASSERT_EXPRs is pretty easy.  This gives us more
chances
to extract simple equivalences from the condition in ASSERT_EXPRs.

This patch fixes those 2 problems.  I don't think it directly helps
pr78496
right now as we're unable to pick up the newly exposed jump threads
until
VRP2.   But that's a short term limitation that I've already addressed
locally :-)

Bootstrapped, regression tested and installed on the trunk.




After this patch on aarch64-linux-gnu I get a bootstrap failure while
linking lto1/cc1/cc1plus/go1 in stage2:
godump.o: In function `go_define(unsigned int, char const*)':
godump.c:(.text+0x36c): relocation truncated to fit:
R_AARCH64_ADR_PREL_LO21 against `.rodata'
godump.c:(.text+0x4b4): relocation truncated to fit:
R_AARCH64_ADR_PREL_LO21 against `.rodata'
collect2: error: ld returned 1 exit status
../../gcc/gcc/lto/Make-lang.in:81: recipe for target 'lto1' failed
make[3]: *** [lto1] Error 1



I should mention this is even after the patch to
simplify_assert_expr_using_ranges .


Just spun up an aarch64 machine for testing and it's failing for me too.
I'll revert the patch and dig in.


Looks like I misread my regression hunter output.  This patch was not
the cause.  I will write to the person who actually caused it.

Thanks,
Funny, I literally just came to the conclusion it was Richard S's LRA 
patch :-)


jeff

Re: [PATCH 2/n] [PR tree-optimization/78496] Simplify ASSERT_EXPRs to facilitate jump threading

2017-05-07 Thread Andrew Pinski 
On Sun, May 7, 2017 at 8:06 AM, Jeff Law  wrote:
> On 05/06/2017 05:56 PM, Andrew Pinski wrote:
>>
>> On Sat, May 6, 2017 at 4:55 PM, Andrew Pinski  wrote:
>>>
>>> On Sat, May 6, 2017 at 8:03 AM, Jeff Law  wrote:

 This is the 2nd of 3-5 patches to address pr78496.

 Jump threading will examine ASSERT_EXPRs as it walks the IL and record
 information from those ASSERT_EXPRs into the available expression and
 const/copies tables where they're later used to simplify conditionals.

 We're missing a couple things though.

 First an ASSERT_EXPR with an EQ test creates an equality we can enter
 into
 the const/copy tables.  We were failing to do that.  This is most
 interesting when the RHS of the condition in the ASSERT_EXPR is a
 constant,
 obviously.  This has a secondary benefit of doing less work to get
 better
 optimization.

 Second, some ASSERT_EXPRs may start off as a relational test such as x
 <= 0,
 but after range extraction and propagation they could be simplified into
 an
 equality comparison.  We already do this with conditionals and
 generalizing
 that code to handle ASSERT_EXPRs is pretty easy.  This gives us more
 chances
 to extract simple equivalences from the condition in ASSERT_EXPRs.

 This patch fixes those 2 problems.  I don't think it directly helps
 pr78496
 right now as we're unable to pick up the newly exposed jump threads
 until
 VRP2.   But that's a short term limitation that I've already addressed
 locally :-)

 Bootstrapped, regression tested and installed on the trunk.
>>>
>>>
>>>
>>> After this patch on aarch64-linux-gnu I get a bootstrap failure while
>>> linking lto1/cc1/cc1plus/go1 in stage2:
>>> godump.o: In function `go_define(unsigned int, char const*)':
>>> godump.c:(.text+0x36c): relocation truncated to fit:
>>> R_AARCH64_ADR_PREL_LO21 against `.rodata'
>>> godump.c:(.text+0x4b4): relocation truncated to fit:
>>> R_AARCH64_ADR_PREL_LO21 against `.rodata'
>>> collect2: error: ld returned 1 exit status
>>> ../../gcc/gcc/lto/Make-lang.in:81: recipe for target 'lto1' failed
>>> make[3]: *** [lto1] Error 1
>>
>>
>> I should mention this is even after the patch to
>> simplify_assert_expr_using_ranges .
>
> Just spun up an aarch64 machine for testing and it's failing for me too.
> I'll revert the patch and dig in.

Looks like I misread my regression hunter output.  This patch was not
the cause.  I will write to the person who actually caused it.

Thanks,
Andrew


>
> jeff

Re: {PATCH] New C++ warning -Wcatch-value

2017-05-07 Thread Martin Sebor 

On 05/07/2017 02:03 PM, Volker Reichelt wrote:

On  2 May, Martin Sebor wrote:

On 05/01/2017 02:38 AM, Volker Reichelt wrote:

Hi,

catching exceptions by value is a bad thing, as it may cause slicing, i.e.
a) a superfluous copy
b) which is only partial.
See also 
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#e15-catch-exceptions-from-a-hierarchy-by-reference

To warn the user about catch handlers of non-reference type,
the following patch adds a new C++/ObjC++ warning option "-Wcatch-value".


I think the problems related to catching exceptions by value
apply to (a subset of) class types but not so much to fundamental
types.  I would expect indiscriminately warning on every type to
be overly restrictive.

The Enforcement section of the C++ guideline suggests to

   Flag by-value exceptions if their types are part of a hierarchy
   (could require whole-program analysis to be perfect).

The corresponding CERT C++ Coding Standard guideline offers
a similar suggestion here:

   https://www.securecoding.cert.org/confluence/x/TAD5CQ

so I would suggest to model the warning on that approach (within
limits of a single translation unit, of course).   I.e., warn only
for catching by value objects of non-trivial types, or perhaps even
only polymorphic types?

Martin


I've never seen anybody throw integers in real-world code, so I didn't
want to complicate things for this case. But maybe I should only warn
about class-types.

IMHO it makes sense to warn about non-polymorphic class types
(although slicing is not a problem there), because you still have to
deal with redundant copies.

Another thing would be pointers. I've never seen pointers in catch
handlers (except some 'catch (const char*)' which I would consider
bad practice). Therefore I'd like to warn about 'catch (const A*)'
which might be a typo that should read 'catch (const A&)' instead.

Would that be OK?


To my knowledge, catch by value of non-polymorphic types (and
certainly fundamental types) is not a cause of common bugs.
It's part of the recommended practice to throw by value, catch
by reference, which is grounded in avoiding the slicing problem.
It's also sometimes recommended for non-trivial class types to
avoid creating a copy of the object (which, for non-trivial types,
may need to allocate resource and could throw).  Otherwise, it's
not dissimilar to pass-by value vs pass-by-reference (or even
pass-by-pointer).  Both may be good practices for some types or
in some situations but neither is necessary to avoid bugs or
universally applicable to achieve superior performance.

The pointer case is interesting.  In C++ Coding Standards,
Sutter and Alexandrescu recommend to throw (and catch) smart
pointers over plain pointers because it obviates having to deal
with memory management issues.  That's sound advice but it seems
more like a design guideline than a coding rule aimed at directly
preventing bugs.  I also think that the memory management bugs
that it might find might be more easily detected at the throw
site instead.  E.g., warning on the throw expression below:

  {
Exception e;
throw 
  }

or perhaps even on

  {
throw *new Exception ();
  }

A more sophisticated (and less restrictive) checker could detect
and warn on "throw " if it found a catch (T) or catch (T&)
in the same file and no catch (T*) (but not warn otherwise).

Martin

PS After re-reading some of the coding guidelines on this topic
it occurs to me that (if your patch doesn't handle this case yet)
it might be worth considering to enhance it to also warn on
rethrowing caught polymorphic objects (i.e., warn on

  catch (E ) { throw e; }

and suggest to use "throw;" instead, for the same reason: to
help avoid slicing.

PPS  It may be a useful feature to implement some of other ideas
you mentioned (e.g., throw by value rather than pointer) but it
feels like a separate and more ambitious project than detecting
the relatively common and narrow slicing problem.



Regards,
Volker


Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2017-05-01  Volker Reichelt  

* doc/invoke.texi (-Wcatch-value): Document new warning option.

Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 247416)
+++ gcc/doc/invoke.texi (working copy)
@@ -265,7 +265,7 @@
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  -Wcast-align  -Wcast-qual  @gol
--Wchar-subscripts -Wchkp  -Wclobbered  -Wcomment  @gol
+-Wchar-subscripts  -Wchkp  -Wcatch-value  -Wclobbered  -Wcomment  @gol
 -Wconditionally-supported  @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
 -Wdelete-incomplete @gol
@@ -5827,6 +5827,11 @@
 literals to @code{char *}.  This warning is enabled by default for C++
 programs.

Re: [PATCH, RFC] warn about raw pointers that can be unique_ptr

2017-05-07 Thread Trevor Saunders 
On Sun, May 07, 2017 at 07:32:48PM +0200, Marc Glisse wrote:
> On Sun, 7 May 2017, tbsaunde+...@tbsaunde.org wrote:
> 
> > This is a start at warning about various resource allocation issues that 
> > can be
> > improved.  Currently it only warns about functions that call malloc and then
> > always pass the resulting pointer to free().
> > 
> > It should be pretty simple to extend this to new/delete and new[]/delete[], 
> > as
> > well as checking that in simple cases the pairing is correct.  However it
> > wasn't obvious to me how to tell if a function call is to a allocating 
> > operator
> > new.  We probably don't want to warn about placement new since complicated
> > things can be happening there.  There is the DECL_IS_OPERATOR_NEW() but that
> > doesn't seem to cover class specific operator new overloads, and maybe even
> > custom ones at global scope?
> > 
> > Other things that may be reasonable to try and include would be open / close
> > and locks.
> > 
> > It might also be good to warn about functions that take or return unique
> > ownership of resources, but I'm not quiet sure how to handle functions that
> > allocate or deallocate shared resources.
> 
> Interesting.
> 
> 1) How do you avoid warning for code that already uses unique_ptr? After
> inlining, it will look just like code calling new, and delete on all paths.

Currently by running early enough that no inline has happened.  I'm not
sure how valuable it would be to try and find things post inlining
instead.

> Well, currently you only handle malloc/free, but if you provide an inline
> implementation of new/delete that forwards to malloc/free, the issue should
> already appear. Or maybe the pass is very early? But then the compiler will
> likely seldom notice that the argument to free is actually the return of
> malloc.

I wonder how true this last bit is, sure we could find some more things
after more optimization, but its not obvious to me that it would be so
much to really be worth it.  Looking at gcc itself there's plenty of
places where this should already be enough with tracking places where
the pointer is coppied from one ssa name to another.  I feel like the
big areas of improvement are warning about functions that take or return
ownership of things.

> 2) In some sense, we would want to warn when some path is missing the call
> to free, but it seems very hard to distinguish the cases where it is
> accidentally missing from those where it is done on purpose. Maybe detect
> cases where the only paths missing free are through exceptions?

I didn't think about exceptions, but I'd been thinking of trying to warn
about variables we prove don't escape in alias analysis, but that would
probably be a small set of all allocations.

> 3) This seems very similar to the conditions needed to transform a call to
> malloc into a stack allocation. I was experimenting with this some years
> ago, see for instance
> https://gcc.gnu.org/ml/gcc-patches/2013-11/msg01049.html (it is probably not
> the most recent message on the subject, since the main code moved to CCP
> afterwards http://geometrica.saclay.inria.fr/team/Marc.Glisse/tmp/heapstack,
> but I can't locate more recent messages at the moment, and it was more a
> prototype than a ready patch). Do you think the infrastructure could be
> shared between your warning and such an optimization? Or are the goals
> likely to diverge too soon for sharing to be practical?

yeah, some of the issues are certainly similar.  I'm not sure if there
would be things worth sharing probably easiest to find out by just
trying it.

Trev

> 
> -- 
> Marc Glisse

Re: [PATCH/AARCH64] Improve/correct ThunderX 1 cost model for Arith_shift

2017-05-07 Thread Andrew Pinski 
On Fri, Dec 30, 2016 at 10:05 PM, Andrew Pinski  wrote:
> Hi,
>   Currently for the following function:
> int f(int a, int b)
> {
>   return a + (b <<7);
> }
>
> GCC produces:
> add w0, w0, w1, lsl 7
> But for ThunderX 1, it is better if the instruction was split allowing
> better scheduling to happen in most cases, the latency is the same.  I
> get a small improvement in coremarks, ~1%.
>
> Currently the code does not take into account Arith_shift even though
> the comment:
>   /* Strip any extend, leave shifts behind as we will
> cost them through mult_cost.  */
> Say it does not strip out the shift, aarch64_strip_extend does and has
> always has since the back-end was added to GCC.
>
> Once I fixed the code around aarch64_strip_extend, I got a regression
> for ThunderX 1 as some shifts/extends (left shifts <=4 and/or zero
> extends) are considered free so I needed to add a new tuning flag.
>
> Note I will get an even more improvement for ThunderX 2 CN99XX, but I
> have not measured it yet as I have not made the change to
> aarch64-cost-tables.h yet as I am waiting for approval of the renaming
> patch first before submitting any of the cost table changes.  Also I
> noticed this problem with this tuning first and then looked back at
> what I needed to do for ThunderX 1.
>
> OK?  Bootstrapped and tested on aarch64-linux-gnu without any
> regressions (both with and without --with-cpu=thunderx).

Ping?  This has been not reviewed for over 5 months now :(.

Thanks,
Andrew

>
> Thanks,
> Andrew
>
> ChangeLog:
> * config/aarch64/aarch64-cost-tables.h (thunderx_extra_costs):
> Increment Arith_shift and Arith_shift_reg by 1.
> * config/aarch64/aarch64-tuning-flags.def (easy_shift_extend): New tuning 
> flag.
> * config/aarch64/aarch64.c (thunderx_tunings): Enable
> AARCH64_EXTRA_TUNE_EASY_SHIFT_EXTEND.
> (aarch64_strip_extend): Add new argument and test for it.
> (aarch64_easy_mult_shift_p): New function.
> (aarch64_rtx_mult_cost): Call aarch64_easy_mult_shift_p and don't add
> a cost if it is true.
> Update calls to aarch64_strip_extend.
> (aarch64_rtx_costs): Update calls to aarch64_strip_extend.

Re: {PATCH] New C++ warning -Wcatch-value

2017-05-07 Thread Volker Reichelt 
On  2 May, Martin Sebor wrote:
> On 05/01/2017 02:38 AM, Volker Reichelt wrote:
>> Hi,
>>
>> catching exceptions by value is a bad thing, as it may cause slicing, i.e.
>> a) a superfluous copy
>> b) which is only partial.
>> See also 
>> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#e15-catch-exceptions-from-a-hierarchy-by-reference
>>
>> To warn the user about catch handlers of non-reference type,
>> the following patch adds a new C++/ObjC++ warning option "-Wcatch-value".
> 
> I think the problems related to catching exceptions by value
> apply to (a subset of) class types but not so much to fundamental
> types.  I would expect indiscriminately warning on every type to
> be overly restrictive.
> 
> The Enforcement section of the C++ guideline suggests to
> 
>Flag by-value exceptions if their types are part of a hierarchy
>(could require whole-program analysis to be perfect).
> 
> The corresponding CERT C++ Coding Standard guideline offers
> a similar suggestion here:
> 
>https://www.securecoding.cert.org/confluence/x/TAD5CQ
> 
> so I would suggest to model the warning on that approach (within
> limits of a single translation unit, of course).   I.e., warn only
> for catching by value objects of non-trivial types, or perhaps even
> only polymorphic types?
> 
> Martin

I've never seen anybody throw integers in real-world code, so I didn't
want to complicate things for this case. But maybe I should only warn
about class-types.

IMHO it makes sense to warn about non-polymorphic class types
(although slicing is not a problem there), because you still have to
deal with redundant copies.

Another thing would be pointers. I've never seen pointers in catch
handlers (except some 'catch (const char*)' which I would consider
bad practice). Therefore I'd like to warn about 'catch (const A*)'
which might be a typo that should read 'catch (const A&)' instead.

Would that be OK?

Regards,
Volker

>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>> OK for trunk?
>>
>> Regards,
>> Volker
>>
>>
>> 2017-05-01  Volker Reichelt  
>>
>>  * doc/invoke.texi (-Wcatch-value): Document new warning option.
>>
>> Index: gcc/doc/invoke.texi
>> ===
>> --- gcc/doc/invoke.texi  (revision 247416)
>> +++ gcc/doc/invoke.texi  (working copy)
>> @@ -265,7 +265,7 @@
>>  -Wno-builtin-declaration-mismatch @gol
>>  -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
>>  -Wc++-compat  -Wc++11-compat  -Wc++14-compat  -Wcast-align  -Wcast-qual  
>> @gol
>> --Wchar-subscripts -Wchkp  -Wclobbered  -Wcomment  @gol
>> +-Wchar-subscripts  -Wchkp  -Wcatch-value  -Wclobbered  -Wcomment  @gol
>>  -Wconditionally-supported  @gol
>>  -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time 
>> @gol
>>  -Wdelete-incomplete @gol
>> @@ -5827,6 +5827,11 @@
>>  literals to @code{char *}.  This warning is enabled by default for C++
>>  programs.
>>
>> +@item -Wcatch-value @r{(C++ and Objective-C++ only)}
>> +@opindex Wcatch-value
>> +@opindex Wno-catch-value
>> +Warn about catch handler of non-reference type.
>> +
>>  @item -Wclobbered
>>  @opindex Wclobbered
>>  @opindex Wno-clobbered
>> ===
>>
>> 2017-05-01  Volker Reichelt  
>>
>>  * c.opt (Wcatch-value): New C++ warning flag.
>>
>> Index: gcc/c-family/c.opt
>> ===
>> --- gcc/c-family/c.opt   (revision 247416)
>> +++ gcc/c-family/c.opt   (working copy)
>> @@ -388,6 +388,10 @@
>>  C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
>>  Warn about casts which discard qualifiers.
>>
>> +Wcatch-value
>> +C++ ObjC++ Var(warn_catch_value) Warning
>> +Warn about catch handlers of non-reference type.
>> +
>>  Wchar-subscripts
>>  C ObjC C++ ObjC++ Var(warn_char_subscripts) Warning LangEnabledBy(C ObjC 
>> C++ ObjC++,Wall)
>>  Warn about subscripts whose type is \"char\".
>> ===
>>
>> 2017-05-01  Volker Reichelt  
>>
>>  * semantics.c (finish_handler_parms): Warn about non-reference type
>>  catch handlers.
>>
>> Index: gcc/cp/semantics.c
>> ===
>> --- gcc/cp/semantics.c   (revision 247416)
>> +++ gcc/cp/semantics.c   (working copy)
>> @@ -1321,7 +1321,15 @@
>>  }
>>  }
>>else
>> -type = expand_start_catch_block (decl);
>> +{
>> +  type = expand_start_catch_block (decl);
>> +  if (warn_catch_value
>> +  && type != NULL_TREE
>> +  && type != error_mark_node
>> +  && TREE_CODE (TREE_TYPE (decl)) != REFERENCE_TYPE)
>> +warning (OPT_Wcatch_value,
>> + "catching non-reference type %qT", TREE_TYPE (decl));
>> +}
>>HANDLER_TYPE (handler) = type;
>>  }
>>
>>

Re: [PATCH] PR80280: Fix quoting of candidate functions

2017-05-07 Thread Volker Reichelt 
OK, committed as obvious (r247728).

Regards,
Volker

On  7 May, Martin Sebor wrote:
> On 05/06/2017 02:41 PM, Volker Reichelt wrote:
>> Hi,
>>
>> the following patch fixes some wrong quoting that was introduced by
>> Martin's patch for PR translation/80280, see
>> https://gcc.gnu.org/viewcvs/gcc?view=revision=247607
>>
>> Consider the following testcase:
>>  struct A {};
>>  bool b = !A();
>>
>> On trunk we currently get the following diagnostic:
>>  bug.cc:2:10: error: no match for 'operator!' (operand type is 'A')
>>   bool b = !A();
>>^~~~
>>  bug.cc:2:10: note: 'candidate: operator!(bool) '
>>  bug.cc:2:10: note:   no known conversion for argument 1 from 'A' to 'bool'
>>
>> Note, that not only the candidate function, but also the surrounding
>> text is quoted in the second-to-last line.
>>
>> With the patch, this line reads:
>>  bug.cc:2:10: note: candidate: 'operator!(bool)' 
> 
> This quoting looks better, thanks for the correction.  I would
> say it falls under the obvious fix category.
> 
> Incidentally, the candidate for the test case (and other Boolean
> expressions involving the struct) doesn't look very helpful or
> even relevant, but that's a separate issue.
> 
> Martin
> 
>>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>
>> OK for trunk?
>>
>> Regards,
>> Volker
>>
>>
>> 2017-05-06  Volker Reichelt  
>>
>>  PR translation/80280
>>  * call.c (print_z_candidate): Fix quoting.
>>
>> Index: gcc/cp/call.c
>> ===
>> --- gcc/cp/call.c(revision 247720)
>> +++ gcc/cp/call.c(working copy)
>> @@ -3457,16 +3457,16 @@
>>  {
>>cloc = loc;
>>if (candidate->num_convs == 3)
>> -inform (cloc, "%<%s%D(%T, %T, %T) %>", msg, fn,
>> +inform (cloc, "%s%<%D(%T, %T, %T)%> ", msg, fn,
>>  candidate->convs[0]->type,
>>  candidate->convs[1]->type,
>>  candidate->convs[2]->type);
>>else if (candidate->num_convs == 2)
>> -inform (cloc, "%<%s%D(%T, %T) %>", msg, fn,
>> +inform (cloc, "%s%<%D(%T, %T)%> ", msg, fn,
>>  candidate->convs[0]->type,
>>  candidate->convs[1]->type);
>>else
>> -inform (cloc, "%<%s%D(%T) %>", msg, fn,
>> +inform (cloc, "%s%<%D(%T)%> ", msg, fn,
>>  candidate->convs[0]->type);
>>  }
>>else if (TYPE_P (fn))
>> ===

[patch] FreeBSD arm libgcc config.host

2017-05-07 Thread Andreas Tobler 

Hi all,

I'm going to commit the below patch to all active branches. (8,7,6,5)
It makes arm*-*-freebsd* use the generic FreeBSD t-slibgcc-elf-ver 
definition. This makes all FreeBSD targets 'consistent' in this area.


If not ok, please speak up soon.

TIA,
Andreas

2017-05-07  Andreas Tobler  

* config.host): Use the generic FreeBSD t-slibgcc-elf-ver for
arm*-*-freebsd instead of the t-slibgcc-libgcc.

Index: config.host
===
--- config.host (revision 247727)
+++ config.host (working copy)
@@ -397,7 +397,7 @@
;;
 arm*-*-freebsd*)# ARM FreeBSD EABI
tmake_file="${tmake_file} arm/t-arm t-fixedpoint-gnu-prefix arm/t-elf"
-   tmake_file="${tmake_file} arm/t-bpabi arm/t-freebsd t-slibgcc-libgcc"
+   tmake_file="${tmake_file} arm/t-bpabi arm/t-freebsd"
tm_file="${tm_file} arm/bpabi-lib.h"
unwind_header=config/arm/unwind-arm.h
 	tmake_file="${tmake_file} t-softfp-sfdf t-softfp-excl arm/t-softfp 
t-softfp"

[patch] FreeBSD arm: make _Unwind_G/SetIP available as function.

2017-05-07 Thread Andreas Tobler 

Hi all,

On FreeBSD we make use of the functions _Unwind_GetIP, _Unwind_GetIPInfo 
and _Unwind_SetIP outside of GCC. All other FreeBSD targets have these 
functions available except arm.


Now since the GCC port for arm*-*-freebsd* is used more often (not only 
by me ;), I was pointed out that these functions are not available.


The below patch tries to fix this.

Is the patch ok for trunk and after a while also for all active 
branches? (7,6,5?)


I am the FreeBSD maintainer, yes, but I prefer to have an ack since the 
affected files are not only used by FreeBSD. And if somebody has better 
idea, I welcome the input.


TIA,
Andreas

2017-05-07  Andreas Tobler  

* config/arm/unwind-arm.h: Make _Unwind_GetIP, _Unwind_GetIPInfo and
_Unwind_SetIP available as functions for arm*-*-freebsd*.
* config/arm/unwind-arm.c: Implement the above.

Index: libgcc/config/arm/unwind-arm.h
===
--- libgcc/config/arm/unwind-arm.h  (revision 247727)
+++ libgcc/config/arm/unwind-arm.h  (working copy)
@@ -72,6 +72,7 @@
 {
   return _URC_FAILURE;
 }
+#ifndef __FreeBSD__
   /* Return the address of the instruction, not the actual IP value.  */
 #define _Unwind_GetIP(context) \
   (_Unwind_GetGR (context, 15) & ~(_Unwind_Word)1)
@@ -78,6 +79,12 @@
 
 #define _Unwind_SetIP(context, val) \
   _Unwind_SetGR (context, 15, val | (_Unwind_GetGR (context, 15) & 1))
+#else
+  #undef _Unwind_GetIPInfo
+  _Unwind_Ptr _Unwind_GetIP (struct _Unwind_Context *);
+  _Unwind_Ptr _Unwind_GetIPInfo (struct _Unwind_Context *, int *);
+  void _Unwind_SetIP (struct _Unwind_Context *, _Unwind_Ptr);
+#endif
 
 #ifdef __cplusplus
 }   /* extern "C" */
Index: libgcc/config/arm/unwind-arm.c
===
--- libgcc/config/arm/unwind-arm.c  (revision 247727)
+++ libgcc/config/arm/unwind-arm.c  (working copy)
@@ -509,3 +509,25 @@
 {
   return __gnu_unwind_pr_common (state, ucbp, context, 2);
 }
+
+#ifdef __FreeBSD__
+/* FreeBSD expects these to be functions */
+inline _Unwind_Ptr
+_Unwind_GetIP (struct _Unwind_Context *context)
+{
+  return _Unwind_GetGR (context, 15) & ~(_Unwind_Word)1;
+}
+
+inline _Unwind_Ptr
+_Unwind_GetIPInfo (struct _Unwind_Context *context, int *ip_before_insn)
+{
+  *ip_before_insn = 0;
+  return _Unwind_GetIP (context);
+}
+
+inline void
+_Unwind_SetIP (struct _Unwind_Context *context, _Unwind_Ptr val)
+{
+  _Unwind_SetGR (context, 15, val | (_Unwind_GetGR (context, 15) & 1));
+}
+#endif

Re: [PATCH] PR80280: Fix quoting of candidate functions

2017-05-07 Thread Martin Sebor 

On 05/06/2017 02:41 PM, Volker Reichelt wrote:

Hi,

the following patch fixes some wrong quoting that was introduced by
Martin's patch for PR translation/80280, see
https://gcc.gnu.org/viewcvs/gcc?view=revision=247607

Consider the following testcase:
 struct A {};
 bool b = !A();

On trunk we currently get the following diagnostic:
 bug.cc:2:10: error: no match for 'operator!' (operand type is 'A')
  bool b = !A();
   ^~~~
 bug.cc:2:10: note: 'candidate: operator!(bool) '
 bug.cc:2:10: note:   no known conversion for argument 1 from 'A' to 'bool'

Note, that not only the candidate function, but also the surrounding
text is quoted in the second-to-last line.

With the patch, this line reads:
 bug.cc:2:10: note: candidate: 'operator!(bool)' 


This quoting looks better, thanks for the correction.  I would
say it falls under the obvious fix category.

Incidentally, the candidate for the test case (and other Boolean
expressions involving the struct) doesn't look very helpful or
even relevant, but that's a separate issue.

Martin



Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk?

Regards,
Volker


2017-05-06  Volker Reichelt  

PR translation/80280
* call.c (print_z_candidate): Fix quoting.

Index: gcc/cp/call.c
===
--- gcc/cp/call.c   (revision 247720)
+++ gcc/cp/call.c   (working copy)
@@ -3457,16 +3457,16 @@
 {
   cloc = loc;
   if (candidate->num_convs == 3)
-   inform (cloc, "%<%s%D(%T, %T, %T) %>", msg, fn,
+   inform (cloc, "%s%<%D(%T, %T, %T)%> ", msg, fn,
candidate->convs[0]->type,
candidate->convs[1]->type,
candidate->convs[2]->type);
   else if (candidate->num_convs == 2)
-   inform (cloc, "%<%s%D(%T, %T) %>", msg, fn,
+   inform (cloc, "%s%<%D(%T, %T)%> ", msg, fn,
candidate->convs[0]->type,
candidate->convs[1]->type);
   else
-   inform (cloc, "%<%s%D(%T) %>", msg, fn,
+   inform (cloc, "%s%<%D(%T)%> ", msg, fn,
candidate->convs[0]->type);
 }
   else if (TYPE_P (fn))
===

Re: [PATCH, RFC] warn about raw pointers that can be unique_ptr

2017-05-07 Thread Marc Glisse 

On Sun, 7 May 2017, tbsaunde+...@tbsaunde.org wrote:


This is a start at warning about various resource allocation issues that can be
improved.  Currently it only warns about functions that call malloc and then
always pass the resulting pointer to free().

It should be pretty simple to extend this to new/delete and new[]/delete[], as
well as checking that in simple cases the pairing is correct.  However it
wasn't obvious to me how to tell if a function call is to a allocating operator
new.  We probably don't want to warn about placement new since complicated
things can be happening there.  There is the DECL_IS_OPERATOR_NEW() but that
doesn't seem to cover class specific operator new overloads, and maybe even
custom ones at global scope?

Other things that may be reasonable to try and include would be open / close
and locks.

It might also be good to warn about functions that take or return unique
ownership of resources, but I'm not quiet sure how to handle functions that
allocate or deallocate shared resources.


Interesting.

1) How do you avoid warning for code that already uses unique_ptr? After 
inlining, it will look just like code calling new, and delete on all 
paths. Well, currently you only handle malloc/free, but if you provide an 
inline implementation of new/delete that forwards to malloc/free, the 
issue should already appear. Or maybe the pass is very early? But then the 
compiler will likely seldom notice that the argument to free is actually 
the return of malloc.


2) In some sense, we would want to warn when some path is missing the call 
to free, but it seems very hard to distinguish the cases where it is 
accidentally missing from those where it is done on purpose. Maybe detect 
cases where the only paths missing free are through exceptions?


3) This seems very similar to the conditions needed to transform a call to 
malloc into a stack allocation. I was experimenting with this some years 
ago, see for instance 
https://gcc.gnu.org/ml/gcc-patches/2013-11/msg01049.html (it is probably 
not the most recent message on the subject, since the main code moved to 
CCP afterwards 
http://geometrica.saclay.inria.fr/team/Marc.Glisse/tmp/heapstack, but I 
can't locate more recent messages at the moment, and it was more a 
prototype than a ready patch). Do you think the infrastructure could be 
shared between your warning and such an optimization? Or are the goals 
likely to diverge too soon for sharing to be practical?


--
Marc Glisse

Re: [PATCH 2/n] [PR tree-optimization/78496] Simplify ASSERT_EXPRs to facilitate jump threading

2017-05-07 Thread Jeff Law 

On 05/06/2017 05:56 PM, Andrew Pinski wrote:

On Sat, May 6, 2017 at 4:55 PM, Andrew Pinski  wrote:

On Sat, May 6, 2017 at 8:03 AM, Jeff Law  wrote:

This is the 2nd of 3-5 patches to address pr78496.

Jump threading will examine ASSERT_EXPRs as it walks the IL and record
information from those ASSERT_EXPRs into the available expression and
const/copies tables where they're later used to simplify conditionals.

We're missing a couple things though.

First an ASSERT_EXPR with an EQ test creates an equality we can enter into
the const/copy tables.  We were failing to do that.  This is most
interesting when the RHS of the condition in the ASSERT_EXPR is a constant,
obviously.  This has a secondary benefit of doing less work to get better
optimization.

Second, some ASSERT_EXPRs may start off as a relational test such as x <= 0,
but after range extraction and propagation they could be simplified into an
equality comparison.  We already do this with conditionals and generalizing
that code to handle ASSERT_EXPRs is pretty easy.  This gives us more chances
to extract simple equivalences from the condition in ASSERT_EXPRs.

This patch fixes those 2 problems.  I don't think it directly helps pr78496
right now as we're unable to pick up the newly exposed jump threads until
VRP2.   But that's a short term limitation that I've already addressed
locally :-)

Bootstrapped, regression tested and installed on the trunk.



After this patch on aarch64-linux-gnu I get a bootstrap failure while
linking lto1/cc1/cc1plus/go1 in stage2:
godump.o: In function `go_define(unsigned int, char const*)':
godump.c:(.text+0x36c): relocation truncated to fit:
R_AARCH64_ADR_PREL_LO21 against `.rodata'
godump.c:(.text+0x4b4): relocation truncated to fit:
R_AARCH64_ADR_PREL_LO21 against `.rodata'
collect2: error: ld returned 1 exit status
../../gcc/gcc/lto/Make-lang.in:81: recipe for target 'lto1' failed
make[3]: *** [lto1] Error 1


I should mention this is even after the patch to
simplify_assert_expr_using_ranges .
Just spun up an aarch64 machine for testing and it's failing for me too. 
  I'll revert the patch and dig in.


jeff

New Swedish PO file for 'gcc' (version 7.1.0)

2017-05-07 Thread Translation Project Robot 
Hello, gentle maintainer.

This is a message from the Translation Project robot.

A revised PO file for textual domain 'gcc' has been submitted
by the Swedish team of translators.  The file is available at:

http://translationproject.org/latest/gcc/sv.po

(This file, 'gcc-7.1.0.sv.po', has just now been sent to you in
a separate email.)

All other PO files for your package are available in:

http://translationproject.org/latest/gcc/

Please consider including all of these in your next release, whether
official or a pretest.

Whenever you have a new distribution with a new version number ready,
containing a newer POT file, please send the URL of that distribution
tarball to the address below.  The tarball may be just a pretest or a
snapshot, it does not even have to compile.  It is just used by the
translators when they need some extra translation context.

The following HTML page has been updated:

http://translationproject.org/domain/gcc.html

If any question arises, please contact the translation coordinator.

Thank you for all your work,

The Translation Project robot, in the
name of your translation coordinator.

[patch, fortran] Create temporary variables for matmul

2017-05-07 Thread Thomas Koenig 

Hello world,

the attached patch goes one step further in matmul inlinding.

It converts statements like

  r = dot_product(matmul(a2,v1),v2)

into

  tmp = matmul(a2,v1)
  r = dot_product(tmp,v2)

to enable inlining of matmul (but only if inlining
is active, of course).

In order to detect multiple uses of matmul, this is run
several times.  I did this because, with the current
implementation, create_var can fail if -fno-realloc-lhs
is specified.  This is also not optimal, but that's a bug
for another day, and I don't see any drawbacks in
code generation for this (the extra basic blocks will
be removed).

The actual overhead in the case of non-constant bounds should
be small to non-existent, this only replaces one type of
temporary with another.

I had to adjust some test cases which counted things to use
the library version.

Regression-tested.  OK for trunk?

Regards

Thomas

P.S: Next on the agenda is to better handle the left-over combinations
for Matmul, and to create temporaries for dependencies and
function evaluations in the arguments that we currently do not
handle.

2017-05-07  Thomas Koenig  

PR fortran/79930
* frontend-passes.c (matmul_to_var_expr): New function,
add prototype.
(matmul_to_var_code):  Likewise.
(optimize_namespace):  Use them from gfc_code_walker.

2017-05-07  Thomas Koenig  

PR fortran/79930
* gfortran.dg/inline_transpose_1.f90:  Add
-finline-matmul-limit=0 to options.
* gfortran.dg/matmul_5.f90:  Likewise.
* gfortran.dg/vect/vect-8.f90: Likewise.
* gfortran.dg/inline_matmul_14.f90:  New test.
* gfortran.dg/inline_matmul_15.f90:  New test.
Index: fortran/frontend-passes.c
===
--- fortran/frontend-passes.c	(Revision 247566)
+++ fortran/frontend-passes.c	(Arbeitskopie)
@@ -43,6 +43,8 @@ static void optimize_reduction (gfc_namespace *);
 static int callback_reduction (gfc_expr **, int *, void *);
 static void realloc_strings (gfc_namespace *);
 static gfc_expr *create_var (gfc_expr *, const char *vname=NULL);
+static int matmul_to_var_expr (gfc_expr **, int *, void *);
+static int matmul_to_var_code (gfc_code **, int *, void *);
 static int inline_matmul_assign (gfc_code **, int *, void *);
 static gfc_code * create_do_loop (gfc_expr *, gfc_expr *, gfc_expr *,
   locus *, gfc_namespace *,
@@ -1076,9 +1078,20 @@ optimize_namespace (gfc_namespace *ns)
   gfc_code_walker (>code, cfe_code, cfe_expr_0, NULL);
   gfc_code_walker (>code, optimize_code, optimize_expr, NULL);
   if (flag_inline_matmul_limit != 0)
-gfc_code_walker (>code, inline_matmul_assign, dummy_expr_callback,
-		 NULL);
-
+{
+  bool found;
+  do
+	{
+	  found = false;
+	  gfc_code_walker (>code, matmul_to_var_code, matmul_to_var_expr,
+			   (void *) );
+	}
+  while (found);
+	
+  gfc_code_walker (>code, inline_matmul_assign, dummy_expr_callback,
+		   NULL);
+}
+  
   /* BLOCKs are handled in the expression walker below.  */
   for (ns = ns->contained; ns; ns = ns->sibling)
 {
@@ -2086,6 +2099,64 @@ doloop_warn (gfc_namespace *ns)
 
 /* This selction deals with inlining calls to MATMUL.  */
 
+/* Replace calls to matmul outside of straight assignments with a temporary
+   variable so that later inlining will work.  */
+
+static int
+matmul_to_var_expr (gfc_expr **ep, int *walk_subtrees ATTRIBUTE_UNUSED,
+		void *data)
+{
+  gfc_expr *e, *n;
+  bool *found = (bool *) data;
+  
+  e = *ep;
+
+  if (e->expr_type != EXPR_FUNCTION
+  || e->value.function.isym == NULL
+  || e->value.function.isym->id != GFC_ISYM_MATMUL)
+return 0;
+
+  if (forall_level > 0 || iterator_level > 0 || in_omp_workshare
+  || in_where)
+return 0;
+
+  /* Check if this is already in the form c = matmul(a,b).  */
+  
+  if ((*current_code)->expr2 == e)
+return 0;
+
+  n = create_var (e, "matmul");
+  
+  /* If create_var is unable to create a variable (for example if
+ -fno-realloc-lhs is in force with a variable that does not have bounds
+ known at compile-time), just return.  */
+
+  if (n == NULL)
+return 0;
+  
+  *ep = n;
+  *found = true;
+  return 0;
+}
+
+/* Set current_code and associated variables so that matmul_to_var_expr can
+   work.  */
+
+static int
+matmul_to_var_code (gfc_code **c, int *walk_subtrees ATTRIBUTE_UNUSED,
+		void *data ATTRIBUTE_UNUSED)
+{
+  if (current_code != c)
+{
+  current_code = c;
+  inserted_block = NULL;
+  changed_statement = NULL;
+}
+  
+  return 0;
+}
+
+
 /* Auxiliary function to build and simplify an array inquiry function.
dim is zero-based.  */
 
Index: testsuite/gfortran.dg/inline_transpose_1.f90
===
--- testsuite/gfortran.dg/inline_transpose_1.f90	(Revision 247566)
+++ testsuite/gfortran.dg/inline_transpose_1.f90

[PATCH, RFC] warn about raw pointers that can be unique_ptr

2017-05-07 Thread tbsaunde+gcc 
From: Trevor Saunders 

Hi,

This is a start at warning about various resource allocation issues that can be
improved.  Currently it only warns about functions that call malloc and then
always pass the resulting pointer to free().

It should be pretty simple to extend this to new/delete and new[]/delete[], as
well as checking that in simple cases the pairing is correct.  However it
wasn't obvious to me how to tell if a function call is to a allocating operator
new.  We probably don't want to warn about placement new since complicated
things can be happening there.  There is the DECL_IS_OPERATOR_NEW() but that
doesn't seem to cover class specific operator new overloads, and maybe even
custom ones at global scope?

Other things that may be reasonable to try and include would be open / close
and locks.

It might also be good to warn about functions that take or return unique
ownership of resources, but I'm not quiet sure how to handle functions that
allocate or deallocate shared resources.

bootstrapped but not yet regtested other than the included test on
x86_64-linux-gnu.  All comments and suggestions welcome.

Trev


  ---
 gcc/Makefile.in  |   1 +
 gcc/c-family/c.opt   |   4 +
 gcc/gimple-owned-ptr.c   | 214 +++
 gcc/passes.def   |   1 +
 gcc/testsuite/g++.dg/warn/Wowning-ptr1.C |  76 +++
 gcc/tree-pass.h  |   1 +
 6 files changed, 297 insertions(+)
 create mode 100644 gcc/gimple-owned-ptr.c
 create mode 100644 gcc/testsuite/g++.dg/warn/Wowning-ptr1.C

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index f675e073ecc..d33e8591b03 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1294,6 +1294,7 @@ OBJS = \
gimple-laddress.o \
gimple-low.o \
gimple-pretty-print.o \
+   gimple-owned-ptr.o \
gimple-ssa-backprop.o \
gimple-ssa-isolate-paths.o \
gimple-ssa-nonnull-compare.o \
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 8697eb13108..6f12c3ac5eb 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -428,6 +428,10 @@ Wctor-dtor-privacy
 C++ ObjC++ Var(warn_ctor_dtor_privacy) Warning
 Warn when all constructors and destructors are private.
 
+Wowning-ptr
+C++ ObjC++ Var(warn_owning_ptr) Warning
+Warn about pointers that can be marked as owning the resource they refer to.
+
 Wdangling-else
 C ObjC C++ ObjC++ Var(warn_dangling_else) Warning LangEnabledBy(C ObjC C++ 
ObjC++,Wparentheses)
 Warn about dangling else.
diff --git a/gcc/gimple-owned-ptr.c b/gcc/gimple-owned-ptr.c
new file mode 100644
index 000..f1283940920
--- /dev/null
+++ b/gcc/gimple-owned-ptr.c
@@ -0,0 +1,214 @@
+/* Detection of allocations that are owned by a single function.
+   Copyright (C) 2015-2017 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "backend.h"
+#include "tree.h"
+#include "gimple.h"
+#include "basic-block.h"
+#include "options.h"
+#include "flags.h"
+#include "stmt.h"
+#include "gimple-iterator.h"
+#include "tree-cfg.h"
+#include "tree-pass.h"
+#include "ssa.h"
+#include "diagnostic-core.h"
+
+/*  Return true if gcall calls a function in the malloc family where the memory
+ *  can be released by calling free().  */
+
+static bool
+is_malloc_alloc (gcall *call)
+{
+  if (!gimple_call_builtin_p (call))
+return false;
+
+  tree callee = gimple_call_fndecl (call);
+  built_in_function builtin = DECL_FUNCTION_CODE (callee);
+  if (builtin != BUILT_IN_MALLOC
+  && builtin != BUILT_IN_CALLOC
+  && builtin != BUILT_IN_ALIGNED_ALLOC)
+return false;
+
+  return true;
+}
+
+/* A free_fn_p implementation for free().  We don't need to check what's being
+   passed to free() because the resource in question is used in the statement,
+   and free() should only use one thing.  */
+
+static bool
+is_malloc_free (gcall *call, tree)
+{
+  return gimple_call_builtin_p (call, BUILT_IN_FREE);
+}
+
+namespace {
+
+class pass_owned_ptrs : public gimple_opt_pass
+{
+public:
+  pass_owned_ptrs (gcc::context *ctxt) : gimple_opt_pass (data, ctxt) {}
+
+  static const pass_data data;
+  virtual bool
+  gate (function *)
+  {
+return true;
+  }
+  virtual unsigned int
+  execute (function *fun);
+

Re: Make tree-ssa-strlen.c handle partial unterminated strings

2017-05-07 Thread Richard Sandiford 
Jakub Jelinek  writes:
> On Fri, May 05, 2017 at 01:01:08PM +0100, Richard Sandiford wrote:
>> tree-ssa-strlen.c looks for cases in which a string is built up using
>> operations like:
>> 
>> memcpy (a, "foo", 4);
>> memcpy (a + 3, "bar", 4);
>> int x = strlen (a);
>> 
>> As a side-effect, it optimises the non-final memcpys so that they don't
>> include the nul terminator.
>> 
>> However, after removing some "& ~0x1"s from tree-ssa-dse.c, the DSE pass
>> does this optimisation itself (because it can tell that later memcpys
>> overwrite the terminators).  The strlen pass wasn't able to handle these
>> pre-optimised calls in the same way as the unoptimised ones.
>> 
>> This patch adds support for tracking unterminated strings.
>
> I'm not sure I like the terminology (terminated vs. !terminated), I wonder
> if it wouldn't be better to add next to length field minimum_length field,
> length would be what it is now, tree representing the string length,
> while minimum_length would be just a guarantee that strlen (ptr) >=
> minimum_length, i.e. that in the first minimum_length bytes (best would be
> to guarantee that it is just a constant if non-NULL) are non-zero.
> It shouldn't be handled just by non-zero terminated memcpy, but e.g. even if
> you e.g. construct it byte by byte, etc.
>   a[0] = 'a';
>   a[1] = 'b';
>   a[2] = 'c';
>   a[3] = 'd';
>   a[4] = '\0';
>   x = strlen (a);
> etc., or
>   strcpy (a, "abcdefg");
>   strcpy (a + 8, "hijk");
>   a[7] = 'q';
>   x = strlen (a);
> or say by storing 4 non-zero bytes at a time...

I've got most of the way through a version that uses min_length instead.
But one thing that the terminated flag allows that a constant min_length
doesn't is:

  size_t
  f1 (char *a1)
  {
size_t x = strlen (a1);
char *a3 = a1 + x;
a3[0] = '1';  // a1 length x + 1, unterminated  (min length x + 1)
a3[1] = '2';  // a1 length x + 2, unterminated  (min length x + 2)
a3[2] = '3';  // a1 length x + 3, unterminated  (min length x + 3)
a3[3] = 0;// a1 length x + 3, terminated
return strlen (a1);
  }

For the a3[3] = 0, we know a3's min_length is 3 and so it's obvious
that we can convert its min_length to a length.  But even if we allow
a1's min_length to be nonconstant, it seems a bit risky to assume that
we can convert its min_length to a length as well.  It would only work
if the min_lengths in related strinfos are kept in sync, whereas it
ought to be safe to say that the minimum length of something is 0.

Having two separate fields could allow us to say that a1's length is x
and its minimum length is 3 in:

  a1[0] = '1';
  a1[1] = '2';
  a1[2] = '3';
  size_t x = strlen (a1);

which in turn would allow the final length of a1 in:

  a1[0] = '1';
  a1[1] = '2';
  a1[2] = '3';
  size_t x = strlen (a1);
  a1[3] = '4';   // drop a1's length here
  a1[4] = 0;

to be optimised to 4.  But we'd then have to drop the constant minimum for:

  a1[0] = '1';
  a1[1] = '2';
  a1[2] = '3';
  size_t x = strlen (a1);
  char *a3 = a1 + x;
  a3[0] = '4';  // a1's min_length is x + 1, not 4

in order to support:

  a3[1] = 0;// a1's length is x + 1

even though the minimum of 4 would be correct.  So there will be very
few cases in which length is not null and not equal to min_length.

Maybe we could keep both pieces of information around if we had both a
terminated flag and a constant mininum length.

So I think that gives four possiblities:

  (1) Keep the terminated flag, but extend the original patch to handle
  strings built up a character at a time.  This would handle f1 above.

  (2) Replace the terminated flag with a constant minimum length, don't
  handle f1 above.

  (3) Replace the terminated flag with an arbitrary minimum length and
  ensure that it's always valid to copy the minimum length to the
  length when we do so for the final strinfo in a chain.

  (4) Keep the terminated flag and also add a constant minimum length.
  This would handle all the cases above.

Thanks,
Richard