Re: Can we vectorize the code below ?
On Wed, Jun 12, 2019 at 5:22 AM Li Jia He wrote:
>
> Hi,
>
> I recently did some analysis on the automatic vectorization of gcc, I
> found that singed char can not be vectorized in the following code.
>
> ---
> #define ITERATIONS 100
>
> #if defined(do_reduce_signed_char)
> #define TYPE signed char
> #elif defined(do_reduce_unsigned_char)
> #define TYPE unsigned char
> #else
> #error bad define
> #endif
>
> #define SIZE (16384/sizeof(TYPE))
>
> static TYPE x[SIZE] __attribute__ ((aligned (16)));
>
> void obfuscate(void *a, ...);
>
> static void __attribute__((noinline)) do_one(void)
> {
> unsigned long i;
> TYPE a = 0;
>
> obfuscate(x);
>
> for (i = 0; i < SIZE; i++)
> a += x[i];
>
> obfuscate(x, a);
> }
>
> int main(void)
> {
> unsigned long i;
>
> for (i = 0; i < ITERATIONS; i++)
> do_one();
>
> return 0;
> }
> ---
> If we use the following command line
>
> gcc reduce.c -Ddo_reduce_unsigned_char -Ofast -c -S -fdump-tree-vect-details
>
> We can see that this code can be vectorized under the unsigned char data
> type.
> If we use the following command
>
> gcc reduce.c -Ddo_reduce_signed_char -Ofast -c -S -fdump-tree-vect-details
>
> We can see that this code cannot be vectorized under the singed char
> data type.
> I found in the below code for singed char
> ---
> a += x[i];
> ---
> Will do something like the following conversion.
> ---
> a = (signed char) ((unsigned char) x[i] + (unsigned char) a);
> ---
> As a result, the reduction in the code cannot be effectively identified.
> Can we vectorize the code like the above when the data type is signed char ?
Probably another case of https://gcc.gnu.org/PR65930
Richard.
> Thanks,
> Lijia He
>
Re: Can we vectorize the code below ?
Hi Lijia,
On 6/12/19 4:22 AM, Li Jia He wrote:
Hi,
I recently did some analysis on the automatic vectorization of gcc, I
found that singed char can not be vectorized in the following code.
---
#define ITERATIONS 100
#if defined(do_reduce_signed_char)
#define TYPE signed char
#elif defined(do_reduce_unsigned_char)
#define TYPE unsigned char
#else
#error bad define
#endif
#define SIZE (16384/sizeof(TYPE))
static TYPE x[SIZE] __attribute__ ((aligned (16)));
void obfuscate(void *a, ...);
static void __attribute__((noinline)) do_one(void)
{
unsigned long i;
TYPE a = 0;
obfuscate(x);
for (i = 0; i < SIZE; i++)
a += x[i];
obfuscate(x, a);
}
int main(void)
{
unsigned long i;
for (i = 0; i < ITERATIONS; i++)
do_one();
return 0;
}
---
If we use the following command line
gcc reduce.c -Ddo_reduce_unsigned_char -Ofast -c -S
-fdump-tree-vect-details
We can see that this code can be vectorized under the unsigned char data
type.
If we use the following command
gcc reduce.c -Ddo_reduce_signed_char -Ofast -c -S -fdump-tree-vect-details
We can see that this code cannot be vectorized under the singed char
data type.
I found in the below code for singed char
---
a += x[i];
---
Will do something like the following conversion.
---
a = (signed char) ((unsigned char) x[i] + (unsigned char) a);
---
As a result, the reduction in the code cannot be effectively identified.
Can we vectorize the code like the above when the data type is signed
char ?
This looks like the known limitation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65930
Thanks,
Kyrill
Thanks,
Lijia He
Re: Git 'gcc-9_1_0-release' tag vs. GCC 9.1 release: 'BASE-VER' difference
Hi! On Tue, 11 Jun 2019 16:35:40 +0100, Jonathan Wakely wrote: > On Tue, 11 Jun 2019 at 16:33, Jonathan Wakely wrote: > > On Tue, 11 Jun 2019 at 16:29, Thomas Schwinge > > wrote: > > > On Tue, 11 Jun 2019 16:18:51 +0100, Jonathan Wakely > > > wrote: > > > > On Tue, 11 Jun 2019 at 16:13, Thomas Schwinge > > > > wrote: > > > > > We have found that the Git 'gcc-9_1_0-release' tag doesn't correspond > > > > > to > > > > > the actual GCC 9.1 release. The GCC 9.1 release (as per > > > > > 'gcc-9.1.0.tar' > > > > > as well as 'svn+ssh://gcc.gnu.org/svn/gcc/tags/gcc_9_1_0_release', > > > > > r272156) > > > > > > (Eh, at the end of that 'svn co [...]', it printed that it "Checked out > > > revision 272156", but the GCC 9.1 release actually is r270840, and > > > r272156 is GCC trunk from a moment ago.) > > > > > > > > would correspond to Git commit > > > > > 3defceaa1a2987fa90296abfbcc85d7e9ad59684 "Update ChangeLog and version > > > > > files for release", but the Git 'gcc-9_1_0-release' tag points one > > > > > commit > > > > > further: Git commit 1f54d412a517f3a4b82f3dd77517842fb4de099a > > > > > "BASE-VER: > > > > > Set to 9.1.1". (That's not a big problem; the 'BASE-VER' update is > > > > > indeed the only difference.) > > > > > > > > That's probably my fault, I think I created the tag. > > > > > > > > > The Git tag can't be corrected now (would it make sense to push a Git > > > > > 'gcc-9_1_0-release-corrected' tag?), but I wanted to post this, to > > > > > get it > > > > > into the mighty Internet archives; may this note help others who > > > > > stumble > > > > > over the same thing. > > > > > > > > Can't we just delete the tag and add it at the right commit? > > > > > > I don't think that'll be useful: as far as I remember (but please correct > > > me if I'm wrong!), a 'git fetch' will not re-fetch changed tags, so Right, see the "DISCUSSION" "On Re-tagging" in 'git tag --help'. > > I think that's right, but 'git fetch --tags' would update it. Sure, but who's running that? ;-) (We shall see if the GitHub etc. mirrors will pick up the updated tag automatically.) > > > different clones might then have different 'gcc-9_1_0-release' tags. > > > > Which doesn't seem like a problem to me. > > > > I could create a local tag with that name for any arbitrary revision. > > It wouldn't match what's in everybody else's clone, but that's fine. > > It seems to me that having the master repo have the correct tag is > more valuable than everybody having the same tag. > > And because, as you say, the difference is just one commit, it's not > like doing diffs or other commands using the old value of the tag > would look at a completely wrong branch or completely different > histories. Note that I'm not objecting to re-tagging. (I had just proposed 'gcc-9_1_0-release-corrected' to make obvious what's going on.) Is there sufficient consensus, or who's going to make a decision? Grüße Thomas
Re: Can we vectorize the code below ?
On 2019/6/12 4:04 PM, Richard Biener wrote:
On Wed, Jun 12, 2019 at 5:22 AM Li Jia He wrote:
Hi,
I recently did some analysis on the automatic vectorization of gcc, I
found that singed char can not be vectorized in the following code.
---
#define ITERATIONS 100
#if defined(do_reduce_signed_char)
#define TYPE signed char
#elif defined(do_reduce_unsigned_char)
#define TYPE unsigned char
#else
#error bad define
#endif
#define SIZE (16384/sizeof(TYPE))
static TYPE x[SIZE] __attribute__ ((aligned (16)));
void obfuscate(void *a, ...);
static void __attribute__((noinline)) do_one(void)
{
unsigned long i;
TYPE a = 0;
obfuscate(x);
for (i = 0; i < SIZE; i++)
a += x[i];
obfuscate(x, a);
}
int main(void)
{
unsigned long i;
for (i = 0; i < ITERATIONS; i++)
do_one();
return 0;
}
---
If we use the following command line
gcc reduce.c -Ddo_reduce_unsigned_char -Ofast -c -S -fdump-tree-vect-details
We can see that this code can be vectorized under the unsigned char data
type.
If we use the following command
gcc reduce.c -Ddo_reduce_signed_char -Ofast -c -S -fdump-tree-vect-details
We can see that this code cannot be vectorized under the singed char
data type.
I found in the below code for singed char
---
a += x[i];
---
Will do something like the following conversion.
---
a = (signed char) ((unsigned char) x[i] + (unsigned char) a);
---
As a result, the reduction in the code cannot be effectively identified.
Can we vectorize the code like the above when the data type is signed char ?
Probably another case of https://gcc.gnu.org/PR65930
Richard.
Thanks to Kyrill and Richard, I have subscribed to this issue.
Thanks,
Lijia He
[Combine] Unusual behaviour in combine
Hi all,
This is a patch to demonstrate some unusual behavior I have encountered in
combine.
A summary of the behaviour is:
when combining A -> B, the register equivalence notes of A are checked, the
register notes of B are not checked.
Is this expected behaviour?
from combine.c:1484 in combine_instructions
/* Try this insn with each REG_EQUAL note it links back to. */
FOR_EACH_LOG_LINK (links, insn)
{
rtx set, note;
rtx_insn *temp = links->insn;
if ((set = single_set (temp)) != 0
&& (note = find_reg_equal_equiv_note (temp)) != 0
&& (note = XEXP (note, 0), GET_CODE (note)) != EXPR_LIST
The register equivalance notes of temp are checked, but the register
equivalence notes of insn are not checked.
To reproduce:
With the patch applied:
Compile the following function
void
bar (float *a, int *b)
{
int i;
for (i = 0; i < 1024; i++)
a[i] = (((float)b[i])/ 4.0f);
}
Combine does not check the REG_EQUAL note on insn 12, and does not try the
equivalent pattern, using a const_vector instead of register 99.
Trying 10 -> 12:
10: r97:V4SF=float(r96:V4SI)
REG_DEAD r96:V4SI
12: r98:V4SF=r97:V4SF*r99:V4SF
REG_DEAD r97:V4SF
REG_EQUAL r97:V4SF*const_vector
Failed to match this instruction:
(set (reg:V4SF 98 [ D.3422 ])
(mult:V4SF (float:V4SF (reg:V4SI 96 [ D.3420 ]))
(reg:V4SF 99)))
For comparison, a similar pattern, in which the REG_EQUAL note is attached to
the first insn, the REG_EQUAL note is checked, and the equivalent constant is
used.
foo (float *a, int *b)
{
int i;
for (i = 0; i < 1024; i++)
b[i] = a[i] * 4.0f;
}
Trying 11 -> 12:
11: r97:V4SF=r96:V4SF*r98:V4SF
REG_DEAD r96:V4SF
REG_EQUAL r96:V4SF*const_vector
12: r99:V4SI=fix(unspec[r97:V4SF] 23)
REG_DEAD r97:V4SF
Failed to match this instruction:
(set (reg:V4SI 99 [ D.3432 ])
(fix:V4SI (unspec:V4SI [
(mult:V4SF (reg:V4SF 96 [ D.3430 ])
(reg:V4SF 98))
] UNSPEC_FRINTZ)))
Trying 11 -> 12:
11: r97:V4SF=r96:V4SF*const_vector
REG_DEAD r96:V4SF
REG_EQUAL r96:V4SF*const_vector
12: r99:V4SI=fix(unspec[r97:V4SF] 23)
REG_DEAD r97:V4SF
Successfully matched this instruction:
(set (reg:V4SI 99 [ D.3432 ])
(fix:V4SI (unspec:V4SI [
(mult:V4SF (reg:V4SF 96 [ D.3430 ])
(const_vector:V4SF [
(const_double:SF 4.0e+0 [0x0.8p+3]) repeated x4
]))
] UNSPEC_FRINTZ)))
Built from current trunk
$gcc -v
COLLECT_GCC=$BUILD/install/bin/aarch64-none-elf-gcc
COLLECT_LTO_WRAPPER=$BUILD/install/libexec/gcc/aarch64-none-elf/10.0.0/lto-wrapper
Target: aarch64-none-elf
Configured with: $SRC/gcc/configure --target=aarch64-none-elf
--prefix=$BUILD/install/ --with-gmp=$BUILD/host-tools
--with-mpfr=$BUILD/host-tools --with-mpc=$BUILD/host-tools
--with-isl=$BUILD/host-tools --disable-shared --disable-nls --disable-threads
--disable-tls --enable-checking=yes --enable-languages=c,c++,fortran
--with-newlib --with-pkgversion=unknown
Thread model: single
gcc version 10.0.0 20190524 (experimental) (unknown)
Test cases compiled with:
aarch64-none-elf-gcc -S -mcpu=cortex-a53 -O2 tmp.c -ftree-vectorize -fno-inline
-fdump-rtl-all -fno-vect-cost-model -dp -fdump-rtl-combine-all
-fdump-tree-optimized -o -
From 7e744509575030ca5b3fa6042d02d27171fbfbfd Mon Sep 17 00:00:00 2001
From: Joel Hutton
Date: Tue, 11 Jun 2019 10:10:07 +0100
Subject: [PATCH] Minimal pattern to demonstrate combine behaviour
---
gcc/config/aarch64/aarch64-protos.h | 1 +
gcc/config/aarch64/aarch64-simd.md | 13 +
gcc/config/aarch64/aarch64.c| 6 ++
gcc/config/aarch64/predicates.md| 3 +++
4 files changed, 23 insertions(+)
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index a0723266f22..ff1787c37ed 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -483,6 +483,7 @@ enum aarch64_symbol_type aarch64_classify_tls_symbol (rtx);
enum reg_class aarch64_regno_regclass (unsigned);
int aarch64_asm_preferred_eh_data_format (int, int);
int aarch64_fpconst_pow_of_2 (rtx);
+int aarch64_fp_const_vec (rtx);
machine_mode aarch64_hard_regno_caller_save_mode (unsigned, unsigned,
machine_mode);
int aarch64_uxt_size (int, HOST_WIDE_INT);
diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index d4c48d2aa61..698b49c006f 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -2133,6 +2133,19 @@
"TARGET_SIMD"
{})
+(define_insn "*aarch64_combine_scvtf"
+ [(set (match_operand 0 "register_operand" "=w")
+ (mult
+ (float
+ (match_operand 1 "" "w"))
+ (match_operand 2 "aarch64_fp_const_vec" ""))
+ )]
+ ""
+ {
+return "test_match";
+ }
+)
+
(define_insn "2"
[(set (match_operand:VHSDF 0 "regi
[PATCH] Do not warn with warn_unused_result for alloca(0).
On 6/11/19 6:03 PM, Jakub Jelinek wrote:
> On Tue, Jun 11, 2019 at 03:58:27PM +, Michael Matz wrote:
>> On Tue, 11 Jun 2019, Martin Liška wrote:
>>
>>> I see 3 occurrences of the alloca (0) in libiberty/regex.c, but there are
>>> properly
>>> guarded within:
>>>
>>> # ifdef C_ALLOCA
>>> alloca (0);
>>> # endif
>>>
>>> and then I noticed 2 more occurrences in gdb that break build right now:
>>>
>>> gdb/regcache.c: alloca (0);
>>> gdb/top.c: alloca (0);
>>>
>>> Is it the right approach to remove these 2 in gdb?
>>
>> It's more an indication that the annotation requesting the warning for
>> unused results is simply overeager (aka wrong) for alloca. (sure, the
>> uses in gdb probably could be cleaned up as well, but that doesn't affect
>> the wrongness of the warning).
>
> Yeah. Either we special-case alloca in the warn_unused_result code
> - if the call flags include ECF_MAY_BE_ALLOCA and argument is 0, don't warn,
> or don't add the attribute to alloca, or add yet another attribute that will
> be used for alloca only.
>
> Jakub
>
Ok, I've got a patch for it.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
Thanks,
Martin
>From b659c00e54ff3bee736f502e7fa4dc233a814b66 Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Wed, 12 Jun 2019 12:22:36 +0200
Subject: [PATCH] Do not warn with warn_unused_result for alloca(0).
gcc/ChangeLog:
2019-06-12 Martin Liska
* calls.c (special_function_p): Make it global.
* calls.h (special_function_p): Declare.
* tree-cfg.c (do_warn_unused_result): Do not
warn for alloca(0).
gcc/testsuite/ChangeLog:
2019-06-12 Martin Liska
* gcc.dg/pr78902.c: Add testing of alloca(0).
* gcc.dg/attr-alloc_size-5.c: Do not warn
about alloca(0).
---
gcc/calls.c | 3 +--
gcc/calls.h | 1 +
gcc/testsuite/gcc.dg/attr-alloc_size-5.c | 2 +-
gcc/testsuite/gcc.dg/pr78902.c | 1 +
gcc/tree-cfg.c | 16
5 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/gcc/calls.c b/gcc/calls.c
index c8a42680041..be9b2ff046a 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -153,7 +153,6 @@ static void compute_argument_addresses (struct arg_data *, rtx, int);
static rtx rtx_for_function_call (tree, tree);
static void load_register_parameters (struct arg_data *, int, rtx *, int,
int, int *);
-static int special_function_p (const_tree, int);
static int check_sibcall_argument_overlap_1 (rtx);
static int check_sibcall_argument_overlap (rtx_insn *, struct arg_data *, int);
@@ -578,7 +577,7 @@ emit_call_1 (rtx funexp, tree fntree ATTRIBUTE_UNUSED, tree fndecl ATTRIBUTE_UNU
Set ECF_MAY_BE_ALLOCA for any memory allocation function that might allocate
space from the stack such as alloca. */
-static int
+int
special_function_p (const_tree fndecl, int flags)
{
tree name_decl = DECL_NAME (fndecl);
diff --git a/gcc/calls.h b/gcc/calls.h
index 128bb513074..0d2bc888b26 100644
--- a/gcc/calls.h
+++ b/gcc/calls.h
@@ -42,5 +42,6 @@ extern tree get_attr_nonstring_decl (tree, tree * = NULL);
extern void maybe_warn_nonstring_arg (tree, tree);
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
+extern int special_function_p (const_tree, int);
#endif // GCC_CALLS_H
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size-5.c b/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
index 7aa7cbf0c72..26ee43f87de 100644
--- a/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
@@ -230,5 +230,5 @@ test_alloca (size_t n)
{
extern void* alloca (size_t);
- alloca (0); /* { dg-warning "ignoring return value of '.*' declared with attribute 'warn_unused_result'" } */
+ alloca (0);
}
diff --git a/gcc/testsuite/gcc.dg/pr78902.c b/gcc/testsuite/gcc.dg/pr78902.c
index 49efc970475..f0f4314d684 100644
--- a/gcc/testsuite/gcc.dg/pr78902.c
+++ b/gcc/testsuite/gcc.dg/pr78902.c
@@ -7,6 +7,7 @@ void foo(void)
__builtin_malloc (1); /* { dg-warning "ignoring return value of '__builtin_malloc' declared with attribute 'warn_unused_result'" } */
__builtin_calloc (10, 20); /* { dg-warning "ignoring return value of '__builtin_calloc' declared with attribute 'warn_unused_result'" } */
__builtin_alloca (10); /* { dg-warning "ignoring return value of '__builtin_alloca' declared with attribute 'warn_unused_result'" } */
+ __builtin_alloca (0);
__builtin_realloc (ptr, 100); /* { dg-warning "ignoring return value of '__builtin_realloc' declared with attribute 'warn_unused_result'" } */
__builtin_aligned_alloc (10, 16); /* { dg-warning "ignoring return value of '__builtin_aligned_alloc' declared with attribute 'warn_unused_result'" } */
__builtin_strdup ("pes"); /* { dg-warning "ignoring return value of '__builtin_strdup' declared with attribute 'warn_unused_result'" } */
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index a585efea3d
Re: [PATCH] Do not warn with warn_unused_result for alloca(0).
On Wed, Jun 12, 2019 at 01:11:09PM +0200, Martin Liška wrote:
> 2019-06-12 Martin Liska
>
> * calls.c (special_function_p): Make it global.
> * calls.h (special_function_p): Declare.
Why?
> * tree-cfg.c (do_warn_unused_result): Do not
> warn for alloca(0).
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -63,6 +63,7 @@ along with GCC; see the file COPYING3. If not see
> #include "opts.h"
> #include "asan.h"
> #include "profile.h"
> +#include "calls.h"
>
> /* This file contains functions for building the Control Flow Graph (CFG)
> for a function tree. */
> @@ -9447,10 +9448,17 @@ do_warn_unused_result (gimple_seq seq)
> location_t loc = gimple_location (g);
>
> if (fdecl)
> - warning_at (loc, OPT_Wunused_result,
> - "ignoring return value of %qD "
> - "declared with attribute %",
> - fdecl);
> + {
> + if ((special_function_p (fdecl, 0) & ECF_MAY_BE_ALLOCA)
Why not instead gimple_maybe_alloca_call_p (g) ?
On the other side, you want && gimple_call_num_args (g) == 1,
if some alloca call had wrong declaration, you might ICE otherwise.
> + && TREE_CODE (gimple_call_arg (g, 0)) == INTEGER_CST
> + && wi::to_wide (gimple_call_arg (g, 0)) == 0)
&& integer_zerop (gimple_call_arg (g, 0))
instead?
Plus you need a comment explaining why we don't warn about alloca with
constant 0 argument.
Jakub
Re: [PATCH] Do not warn with warn_unused_result for alloca(0).
On 6/12/19 1:22 PM, Jakub Jelinek wrote:
> On Wed, Jun 12, 2019 at 01:11:09PM +0200, Martin Liška wrote:
>> 2019-06-12 Martin Liska
>>
>> * calls.c (special_function_p): Make it global.
>> * calls.h (special_function_p): Declare.
>
> Why?
Not needed any longer.
>
>> * tree-cfg.c (do_warn_unused_result): Do not
>> warn for alloca(0).
>> --- a/gcc/tree-cfg.c
>> +++ b/gcc/tree-cfg.c
>> @@ -63,6 +63,7 @@ along with GCC; see the file COPYING3. If not see
>> #include "opts.h"
>> #include "asan.h"
>> #include "profile.h"
>> +#include "calls.h"
>>
>> /* This file contains functions for building the Control Flow Graph (CFG)
>> for a function tree. */
>> @@ -9447,10 +9448,17 @@ do_warn_unused_result (gimple_seq seq)
>>location_t loc = gimple_location (g);
>>
>>if (fdecl)
>> -warning_at (loc, OPT_Wunused_result,
>> -"ignoring return value of %qD "
>> -"declared with attribute %",
>> -fdecl);
>> +{
>> + if ((special_function_p (fdecl, 0) & ECF_MAY_BE_ALLOCA)
>
> Why not instead gimple_maybe_alloca_call_p (g) ?
Because I was not aware of the function ;)
> On the other side, you want && gimple_call_num_args (g) == 1,
Sure.
> if some alloca call had wrong declaration, you might ICE otherwise.
>
>> + && TREE_CODE (gimple_call_arg (g, 0)) == INTEGER_CST
>> + && wi::to_wide (gimple_call_arg (g, 0)) == 0)
>
> && integer_zerop (gimple_call_arg (g, 0))
>
> instead?
Yep.
>
> Plus you need a comment explaining why we don't warn about alloca with
> constant 0 argument.
Also done.
Is it fine now?
Martin
>
> Jakub
>
>From dfdb688206cadd7fb9450ba005e8aa465ae61f7e Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Wed, 12 Jun 2019 12:22:36 +0200
Subject: [PATCH] Do not warn with warn_unused_result for alloca(0).
gcc/ChangeLog:
2019-06-12 Martin Liska
* tree-cfg.c (do_warn_unused_result): Do not
warn for alloca(0) as it's used by some C
libraries to release previously allocated memory
via alloca calls.
gcc/testsuite/ChangeLog:
2019-06-12 Martin Liska
* gcc.dg/pr78902.c: Add testing of alloca(0).
* gcc.dg/attr-alloc_size-5.c: Do not warn
about alloca(0).
---
gcc/testsuite/gcc.dg/attr-alloc_size-5.c | 2 +-
gcc/testsuite/gcc.dg/pr78902.c | 1 +
gcc/tree-cfg.c | 18 ++
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size-5.c b/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
index 7aa7cbf0c72..26ee43f87de 100644
--- a/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size-5.c
@@ -230,5 +230,5 @@ test_alloca (size_t n)
{
extern void* alloca (size_t);
- alloca (0); /* { dg-warning "ignoring return value of '.*' declared with attribute 'warn_unused_result'" } */
+ alloca (0);
}
diff --git a/gcc/testsuite/gcc.dg/pr78902.c b/gcc/testsuite/gcc.dg/pr78902.c
index 49efc970475..f0f4314d684 100644
--- a/gcc/testsuite/gcc.dg/pr78902.c
+++ b/gcc/testsuite/gcc.dg/pr78902.c
@@ -7,6 +7,7 @@ void foo(void)
__builtin_malloc (1); /* { dg-warning "ignoring return value of '__builtin_malloc' declared with attribute 'warn_unused_result'" } */
__builtin_calloc (10, 20); /* { dg-warning "ignoring return value of '__builtin_calloc' declared with attribute 'warn_unused_result'" } */
__builtin_alloca (10); /* { dg-warning "ignoring return value of '__builtin_alloca' declared with attribute 'warn_unused_result'" } */
+ __builtin_alloca (0);
__builtin_realloc (ptr, 100); /* { dg-warning "ignoring return value of '__builtin_realloc' declared with attribute 'warn_unused_result'" } */
__builtin_aligned_alloc (10, 16); /* { dg-warning "ignoring return value of '__builtin_aligned_alloc' declared with attribute 'warn_unused_result'" } */
__builtin_strdup ("pes"); /* { dg-warning "ignoring return value of '__builtin_strdup' declared with attribute 'warn_unused_result'" } */
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index a585efea3d8..0d21f3624d7 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -63,6 +63,7 @@ along with GCC; see the file COPYING3. If not see
#include "opts.h"
#include "asan.h"
#include "profile.h"
+#include "calls.h"
/* This file contains functions for building the Control Flow Graph (CFG)
for a function tree. */
@@ -9447,10 +9448,19 @@ do_warn_unused_result (gimple_seq seq)
location_t loc = gimple_location (g);
if (fdecl)
- warning_at (loc, OPT_Wunused_result,
- "ignoring return value of %qD "
- "declared with attribute %",
- fdecl);
+ {
+ /* Some C libraries use alloca(0) in order to free previously
+ allocated memory by alloca calls. */
+ if (gimple_maybe_alloca_call_p (g)
+ && gimple_call_num_args (g) == 1
+ && integer_zerop (gimple_call_arg (g, 0)))
+ ;
+ else
+ warning
Re: [PATCH] Do not warn with warn_unused_result for alloca(0).
On Wed, Jun 12, 2019 at 01:30:14PM +0200, Martin Liška wrote:
> @@ -9447,10 +9448,19 @@ do_warn_unused_result (gimple_seq seq)
> location_t loc = gimple_location (g);
>
> if (fdecl)
> - warning_at (loc, OPT_Wunused_result,
> - "ignoring return value of %qD "
> - "declared with attribute %",
> - fdecl);
> + {
> + /* Some C libraries use alloca(0) in order to free previously
> + allocated memory by alloca calls. */
> + if (gimple_maybe_alloca_call_p (g)
> + && gimple_call_num_args (g) == 1
> + && integer_zerop (gimple_call_arg (g, 0)))
> + ;
> + else
Wouldn't it be easier to negate the condition and avoid the weird ; else ?
I.e. if (!gimple_maybe... || gimple_call_num != 1 || !integer_zerop?
> + warning_at (loc, OPT_Wunused_result,
> + "ignoring return value of %qD declared "
> + "with attribute %",
> + fdecl);
> + }
> else
> warning_at (loc, OPT_Wunused_result,
> "ignoring return value of function "
Otherwise LGTM as the patch, but I'd like to hear from others whether
it is kosher to add such a special case to the warn_unused_result attribute
warning. And if the agreement is yes, I think it should be documented
somewhere that alloca (0) will not warn even when the call has such an
attribute (probably in the description of warn_unused_result attribute).
Jakub
Re: Committing patches and other conventions (Was: Re: About GSOC)
Hello. Is this the correct sequence for regression test: 1. Revert back all the changes I made and then configure, build along with make bootstrap make -k check collect the *.sum files 2. Apply the patch and do the configuration, build as above 1 and then collect the *.sum files and compare them. How do I collect and inspect these *.sum files? Thanks, -Tejas On Thu, 6 Jun 2019 at 22:26, Martin Jambor wrote: > > Hi, > > On Mon, Jun 03 2019, Tejas Joshi wrote: > > Hello. > > I have already sent a patch for roundeven implementation but I do not > > know how do I commit my changes to GCC. Am I supposed to create a > > branch or anything etc? > > You don't have to create a branch unless you think it would make ease > your own workflow. Once a patch is ready to go and has been explicitely > approved by a corresponding maintainer, you will be expected to commit > it directly to svn (we'll ask for a svn write access for you when we get > to that point). You'll find the list of maintainers in the MAINTAINERS > file of the gcc repository, I believe your patches will need approval > from a global reviewer, most probably Joseph. > > Before that happens, the code must be of course considered correct but > also must adhere to some conventions, please see > https://gcc.gnu.org/codingconventions.html. Your patches so far lacked > a ChangeLog and testcases. Have a look at what other do when they post > patches to gcc-patches: https://gcc.gnu.org/ml/gcc-patches/2019-06/ > > ChangeLog has to have the given, fairly strict format, but should be > very brief. When posting patches, you don't make it part of the patch > even though when committing, you are expected it to prepend the > corresponding ChangeLog file with your bit (see e.g. gcc/ChangeLog and > gcc/testsuite/ChangeLog). > > You have always stated how you tested your patches but you are actually > supposed to add the testsuite and committed along with the functional > patch, so that other can then test they do not regress on the > functionality you have just added. > > That is why everybody including you has to test their patches also by > doing: > > make bootstrap > make -k check > > (with a -j level appropriate for your computer) and then collect *.sum > files from unpatched and patched runs and compare them (see script in > contrib/compare_tests) to make sure they did not introduce any > regressions. > > See section on "Testing patches" at https://gcc.gnu.org/contribute.html > for more details. > > Please ask about these mechanisms and conventions if anything is not > clear. I'll go and find the latest version of your roundeven patch and > see if I can help you a little (but I am likely to finish that only > tomorrow morning). > > Thanks, > > Martin
Re: [PATCH] Do not warn with warn_unused_result for alloca(0).
On 6/12/19 5:37 AM, Jakub Jelinek wrote:
On Wed, Jun 12, 2019 at 01:30:14PM +0200, Martin Liška wrote:
@@ -9447,10 +9448,19 @@ do_warn_unused_result (gimple_seq seq)
location_t loc = gimple_location (g);
if (fdecl)
- warning_at (loc, OPT_Wunused_result,
- "ignoring return value of %qD "
- "declared with attribute %",
- fdecl);
+ {
+ /* Some C libraries use alloca(0) in order to free previously
+allocated memory by alloca calls. */
+ if (gimple_maybe_alloca_call_p (g)
+ && gimple_call_num_args (g) == 1
+ && integer_zerop (gimple_call_arg (g, 0)))
+ ;
+ else
Wouldn't it be easier to negate the condition and avoid the weird ; else ?
I.e. if (!gimple_maybe... || gimple_call_num != 1 || !integer_zerop?
+ warning_at (loc, OPT_Wunused_result,
+ "ignoring return value of %qD declared "
+ "with attribute %",
+ fdecl);
+ }
else
warning_at (loc, OPT_Wunused_result,
"ignoring return value of function "
Otherwise LGTM as the patch, but I'd like to hear from others whether
it is kosher to add such a special case to the warn_unused_result attribute
warning. And if the agreement is yes, I think it should be documented
somewhere that alloca (0) will not warn even when the call has such an
attribute (probably in the description of warn_unused_result attribute).
I'm not very happy about adding another special case to alloca
(on top of not diagnosing zero allocation by -Walloc-zero).
There is no valid use case for the zero argument, whether or not
the return value is used. When it is used it's just as dangerous
as allocating a zero-length VLA. When it isn't used the call is
pointless entirely, and although it might be harmless, it's worth
removing from code just like any other pointless construct GCC
warns about. So I would prefer not to suppress this warning (from
what I've read in the GDB bug it sounds like its calls to alloca(0)
are being removed). I would also prefer to re-enable -Walloc-zero
for alloca(0).
But if there is insufficient support for this I agree that
documenting the built-ins GCC applies attribute warn_unused_result
to is a good idea. (In fact, documenting all the attributes GCC
applies to each built-in would be helpful, but that's a much bigger
project.)
Martin
Re: [PATCH] Do not warn with warn_unused_result for alloca(0).
Hi, On Wed, 12 Jun 2019, Martin Sebor wrote: > > Otherwise LGTM as the patch, but I'd like to hear from others whether > > it is kosher to add such a special case to the warn_unused_result > > attribute warning. And if the agreement is yes, I think it should be > > documented somewhere that alloca (0) will not warn even when the call > > has such an attribute (probably in the description of > > warn_unused_result attribute). > > I'm not very happy about adding another special case to alloca > (on top of not diagnosing zero allocation by -Walloc-zero). > There is no valid use case for the zero argument, whether or not > the return value is used. That's the thing, there _is_ a valid use case for supplying a zero argument and then the returned value should _not_ be used. There are alloca implementations that do something (freeing memory) when called with a zero size, so some (older) programs contain such calls. Warning on those calls for the unused results is exactly the wrong thing to do, if anything if the result is used we'd have to warn. (That's of course non-standard, but so is alloca itself) And just removing these calls isn't correct either except if it's ensured to not use an alloca implementation with that behaviour. (In fact I think our builtin_alloca implementation could benefit when we added that behaviour as well; it's a natural wish to be able to free memory that you allocated). Ciao, Michael.
Re: [PATCH] Do not warn with warn_unused_result for alloca(0).
On 6/12/19 9:25 AM, Michael Matz wrote: Hi, On Wed, 12 Jun 2019, Martin Sebor wrote: Otherwise LGTM as the patch, but I'd like to hear from others whether it is kosher to add such a special case to the warn_unused_result attribute warning. And if the agreement is yes, I think it should be documented somewhere that alloca (0) will not warn even when the call has such an attribute (probably in the description of warn_unused_result attribute). I'm not very happy about adding another special case to alloca (on top of not diagnosing zero allocation by -Walloc-zero). There is no valid use case for the zero argument, whether or not the return value is used. That's the thing, there _is_ a valid use case for supplying a zero argument and then the returned value should _not_ be used. There are alloca implementations that do something (freeing memory) when called with a zero size, so some (older) programs contain such calls. Warning on those calls for the unused results is exactly the wrong thing to do, if anything if the result is used we'd have to warn. (That's of course non-standard, but so is alloca itself) And just removing these calls isn't correct either except if it's ensured to not use an alloca implementation with that behaviour. But GCC doesn't support such an implementation, does it? The only way to use such an alloca is with -fno-builtin-alloca which should suppress the warning. The Linux man page highlights this and the risks of defining one's own alloca function: http://man7.org/linux/man-pages/man3/alloca.3.html In any event, the warning, just like all others, exists to help catch common mistakes: "constructions that are not inherently erroneous but that are risky or suggest there may have been an error". It's not meant to accommodate every conceivable corner case or oddball implementation. Users of those can easily disable the warning #pragma GCC diagnostic. Doing that makes the intent explicit both to the compiler and to other tools and programmers. Martin (In fact I think our builtin_alloca implementation could benefit when we added that behaviour as well; it's a natural wish to be able to free memory that you allocated). Ciao, Michael.
Re: [PATCH] Do not warn with warn_unused_result for alloca(0).
On Wed, Jun 12, 2019 at 10:13:57AM -0600, Martin Sebor wrote: > But GCC doesn't support such an implementation, does it? Why would that be relevant? The warning would cause people to make portable code less portable (by removing the alloca (0) calls that were added there for portability reasons), or add hacks to work around the warning (whether #pragma GCC diagnostic or something else). That is something we do not want people to do. Jakub
Re: Preventing ISO C errors when using macros for builtin types
On Tue, 11 Jun 2019 18:01:55 -0500
Segher Boessenkool wrote:
> On Tue, Jun 11, 2019 at 09:44:30PM +0100, Jozef Lawrynowicz wrote:
> > --- a/gcc/lto/lto-lang.c
> > +++ b/gcc/lto/lto-lang.c
> > @@ -1260,9 +1260,9 @@ lto_build_c_type_nodes (void)
> > if (int_n_enabled_p[i])
> > {
> > char name[50];
> > - sprintf (name, "__int%d unsigned", int_n_data[i].bitsize);
> > + sprintf (name, "int%d", int_n_data[i].bitsize);
> >
> > - if (strcmp (name, SIZE_TYPE) == 0)
> > + if (strstr (SIZE_TYPE, name) != NULL)
> > {
> > intmax_type_node = int_n_trees[i].signed_type;
> > uintmax_type_node = int_n_trees[i].unsigned_type;
>
> I don't think that is correct, strstr allows too much? If you want to
> allow some variants, you should test for all those variants exactly?
Yeah I'll fix this up in my full patch submission.
> It looks great otherwise :-)
>
>
> Segher
Great, thanks for all your help!
Jozef
Re: [PATCH] Do not warn with warn_unused_result for alloca(0).
On 6/12/19 10:40 AM, Jakub Jelinek wrote: On Wed, Jun 12, 2019 at 10:13:57AM -0600, Martin Sebor wrote: But GCC doesn't support such an implementation, does it? Why would that be relevant? Obviously because it makes no sense to cater to all conceivable extensions provided by all sorts of implementations out there. There are libc implementations out there that accept null pointer arguments to functions like memcpy with zero sizes, for example. Or those that accept overlapping objects in calls to strcpy. GCC itself handles those gracefully, yet warns for such constructs nonetheless. It's useful because those misuses are likely hidden bugs, even if they don't always manifest themselves. The warning would cause people to make portable code less portable (by removing the alloca (0) calls that were added there for portability reasons), or add hacks to work around the warning (whether #pragma GCC diagnostic or something else). That is something we do not want people to do. You asked for input and I gave it to you. If your mind was already made up and you're only willing to accept feedback that agrees with your view, don't ask. Martin
Re: About GSOC.
Hello. > I don't think you should have the unreachable "return false;" in is_even. > The last "else if" can just be "else". I don't think return false in is_even is unreachable. As per my understanding, when one else if is true in the else if ladder, all the subsequent "else ifs" including "else" are ignored. When REAL_EXP is less than SIGNIFICAND_BITS, but the number is odd, the inner "if" for even-odd will not return true in which case should return false. That case will ignore next "else if" and will reach return false. > Suppose REAL_EXP (r) > SIGNIFICAND_BITS. Then the number is definitely > even, so you should return true, not false. for this condition, else if can be modified to just else and return true. PATCH: gcc/ChangeLog: 2019-06-12 Tejas Joshi * builtins.c (mathfn_built_in_2): Added CASE_MATHFN for ROUNDEVEN. * builtins.def: Added function definitions for roundeven function variants. * fold-const-call.c (fold_const_call_ss): Added case for function call and fold_const_conversion call for roundeven function. * fold-const.c (negate_mathfn_p): Added case for roundeven function. (tree_call_nonnegative_warnv_p): Added case for roundeven function. (integer_valued_real_call_p): Added case for roundeven function. * real.c (is_even): New function. Returns true if real number is even, otherwise returns false. (is_halfway_below): New function. Returns true if real number is halfway between two integers, else return false. (real_roundeven): New function. Round real number to nearest integer, rounding halfway cases towards even. * real.h (real_value): Added descriptive comments. Added function declaration for roundeven function. gcc/testsuite/ChangeLog: 2019-06-12 Tejas Joshi * gcc.dg/torture/builtin-round-roundeven.c: New test. * gcc.dg/torture/builtin-round-roundevenf128.c: New test. On Tue, 11 Jun 2019 at 01:56, Joseph Myers wrote: > > On Sun, 9 Jun 2019, Tejas Joshi wrote: > > > Hello. > > I have created another patch which addresses the above points, > > attached herewith. > > I don't think you should have the unreachable "return false;" in is_even. > The last "else if" can just be "else". > > > > a conditional with < not <=; if REAL_EXP (r) == SIGNIFICAND_BITS, the > > > least significant bit has value 1 and the number must be an integer). > > > > The number is integer because of the whole word spaces is occupied by > > integer part? > > Also, why would the least significant bit will have value 1 if > > REAL_EXP (r) == SIGNIFICAND_BITS, as it only concerns with 2^0th > > position (even or odd)? > > My understanding is that the significand is, as per the comments in > real.c, in the range [0.5, 1.0). There are SIGNIFICAND_BITS bits. The > bit above the most significant one has value 2^REAL_EXP. The most > significant one has value 2^(REAL_EXP-1). The least significant one has > value 2^(REAL_EXP-SIGNIFICAND_BITS). If REAL_EXP == SIGNIFICAND_BITS, > that means the least significant bit has value 2^0 = 1, and there are no > bits with value 0.5 or below, so the number is an integer. > > -- > Joseph S. Myers > jos...@codesourcery.com diff --git a/gcc/builtins.c b/gcc/builtins.c index 3463ffb1539..85a945877a4 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -2085,6 +2085,7 @@ mathfn_built_in_2 (tree type, combined_fn fn) CASE_MATHFN (REMQUO) CASE_MATHFN_FLOATN (RINT) CASE_MATHFN_FLOATN (ROUND) +CASE_MATHFN (ROUNDEVEN) CASE_MATHFN (SCALB) CASE_MATHFN (SCALBLN) CASE_MATHFN (SCALBN) diff --git a/gcc/builtins.def b/gcc/builtins.def index 6d41bdb4f44..8bb7027aac7 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -542,12 +542,18 @@ DEF_C99_BUILTIN(BUILT_IN_RINTL, "rintl", BT_FN_LONGDOUBLE_LONGDOUBLE, AT #define RINT_TYPE(F) BT_FN_##F##_##F DEF_EXT_LIB_FLOATN_NX_BUILTINS (BUILT_IN_RINT, "rint", RINT_TYPE, ATTR_CONST_NOTHROW_LEAF_LIST) #undef RINT_TYPE +DEF_EXT_LIB_BUILTIN(BUILT_IN_ROUNDEVEN, "roundeven", BT_FN_DOUBLE_DOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST) +DEF_EXT_LIB_BUILTIN(BUILT_IN_ROUNDEVENF, "roundevenf", BT_FN_FLOAT_FLOAT, ATTR_CONST_NOTHROW_LEAF_LIST) +DEF_EXT_LIB_BUILTIN(BUILT_IN_ROUNDEVENL, "roundevenl", BT_FN_LONGDOUBLE_LONGDOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST) DEF_C99_BUILTIN(BUILT_IN_ROUND, "round", BT_FN_DOUBLE_DOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST) DEF_C99_BUILTIN(BUILT_IN_ROUNDF, "roundf", BT_FN_FLOAT_FLOAT, ATTR_CONST_NOTHROW_LEAF_LIST) DEF_C99_BUILTIN(BUILT_IN_ROUNDL, "roundl", BT_FN_LONGDOUBLE_LONGDOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST) #define ROUND_TYPE(F) BT_FN_##F##_##F DEF_EXT_LIB_FLOATN_NX_BUILTINS (BUILT_IN_ROUND, "round", ROUND_TYPE, ATTR_CONST_NOTHROW_LEAF_LIST) #undef ROUND_TYPE +#define ROUNDEVEN_TYPE(F) BT_FN_##F##_##F +DEF_EXT_LIB_FLOATN_NX_BUILTINS (BUILT_IN_ROUNDEVEN, "roundeven", ROUNDEVEN_TYPE, ATTR_CONST_NOTHROW_LEAF_LIST) +#undef ROUNDEVEN_TYPE DEF_EXT_LIB_BUILTIN(BUILT_IN_SCALB, "scal
gcc: -ftest-coverage and -auxbase
When doing a build, we use a pipe between GCC and GAS. And because we wish to do some analysis of the assembly code, we do not use -pipe but instead do '-S -c -'. And this has worked fine for many years. I was recently looking into collecting some coverage information. To that end, I added --coverage to the invocation line. And it slowed things down by more than an order of magnitude! Investigating, it appears that the problem is the writing of the GCNO files. We do our builds on a build cluster with a lot of parallelism. With the result that a dozen machines are each doing a bunch of writes to the file '-.gcno' in an NFS mounted directory. Rather than have a full, not incremental, build take 5-10 minutes, It takes 4 hours. And rather than have each of several thousand compiles produce their own GCNO file, they all get overwritten... Grep'ing around, I found '-auxbase'. If I correctly understand it, when compiling some/path/name.c into bin/some-product/some/path/name.o, I could simply say -auxbase $(@:%.o=%) The problem is that in common.opt, auxbase is marked RejectDriver. It looks like removing it would some my problem. Anyone have a reason why removing that would be a bad idea? Or have a different solution? Thanks. David
