Re: Free memory_block pool
On Wed, 20 Nov 2019, Jan Hubicka wrote:
> Hi,
> I have noticed that for Firefox around 1GB of peak memory use goes into
> the fact that we never free memory_block_pool::freelist.
>
> This patch adds memory_block_pool::trim which reduces freelist to a given
> size. It is called from ggc_collect which is a convenient place to return
> heap allocations too and fully freeed prior forking in ggc_collect.
>
> I originaly was freeing block directly in memory_block_pool::release
> but that makes it non-leaf function which prevents optimization.
> So I decided to go this way we get tiny bit better code
> given that we already have ggc_collect that is conveninet place
> to do such a bookeeping.
>
> Bootstrapped/regtested x86_64-linux, tested on Firefox build, OK?
Huh, I think given that trimming happens explicitely the whole
overhead of counting the number of freelist entries plus adding
a prev member to the block list is useless overhead. In trim
just do
block_list **tailp = _blocks;
for (unsigned cnt = freelist_size; cnt != 0 && *tailp; --cnt)
tailp = &(*tailp)->next;
while (*tailp)
free blocks starting from here
the list walk is O(constant) and the above would just mean adding
a single method rather than cahnges all over the place.
Richard.
> Honza
>
> * memory-block.h (memory_block_pool::freelist): New constant.
> (memory_block_pool::clear_free_list): Rename to ...
> (memory_block_pool::reduce_free_list): ... this.
> (memory_block_pool::trim): New function.
> (memory_block_pool::block_list): Add m_prev.
> (memory_block_pool::m_num_blocks): New field.
> (memory_block_pool::m_block_end): New field.
> (memory_block_pool::allocate): Maintain m_num_blocks and m_blocks_end.
> (memory_block_pool::release): Likewise.
> * memory-block.cc (memory_block_pool::memory_block_pool): Initialize
> new fields.
> (memory_block_pool::clear_free_list): Rename to ...
> (memory_block_pool::reduce_free_list): ... this one; free from end
> and add NUM parameter.
> (memory_block_pool::trim): New.
> * ggc-page.c (ggc_collect): Call memory_block_pool::trim.
>
> * lto.c: Call memory_block_pool::trim.
> Index: memory-block.h
> ===
> --- memory-block.h(revision 278464)
> +++ memory-block.h(working copy)
> @@ -28,12 +28,15 @@ class memory_block_pool
> public:
>/* Blocks have fixed size. This is necessary for sharing. */
>static const size_t block_size = 64 * 1024;
> + /* Number of blocks we keep in the freelists. */
> + static const size_t freelist_size = 1024 * 1024 / block_size;
>
>memory_block_pool ();
>
>static inline void *allocate () ATTRIBUTE_MALLOC;
>static inline void release (void *);
> - void clear_free_list ();
> + static void trim (int nblocks = freelist_size);
> + void reduce_free_list (int);
>
> private:
>/* memory_block_pool singleton instance, defined in memory-block.cc. */
> @@ -42,10 +45,13 @@ private:
>struct block_list
>{
> block_list *m_next;
> +block_list *m_prev;
>};
>
>/* Free list. */
>block_list *m_blocks;
> + block_list *m_blocks_end;
> + int m_num_blocks;
> };
>
> /* Allocate a single block. Reuse a previously returned block, if possible.
> */
> @@ -57,6 +63,9 @@ memory_block_pool::allocate ()
>
>void *result = instance.m_blocks;
>instance.m_blocks = instance.m_blocks->m_next;
> + instance.m_num_blocks--;
> + if (!instance.m_blocks)
> +instance.m_blocks_end = NULL;
>VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (result, block_size));
>return result;
> }
> @@ -67,7 +76,12 @@ memory_block_pool::release (void *uncast
> {
>block_list *block = new (uncast_block) block_list;
>block->m_next = instance.m_blocks;
> + if (instance.m_blocks)
> +instance.m_blocks->m_prev = block;
> + else
> +instance.m_blocks_end = block;
>instance.m_blocks = block;
> + instance.m_num_blocks++;
>
>VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS ((char *)uncast_block
> + sizeof (block_list),
> Index: memory-block.cc
> ===
> --- memory-block.cc (revision 278464)
> +++ memory-block.cc (working copy)
> @@ -26,18 +27,27 @@ along with GCC; see the file COPYING3.
> /* Global singleton-like instance. */
> memory_block_pool memory_block_pool::instance;
>
> -memory_block_pool::memory_block_pool () : m_blocks (NULL) {}
> +/* Default constructor. */
> +memory_block_pool::memory_block_pool ()
> + : m_blocks (NULL), m_blocks_end (NULL), m_num_blocks (0)
> +{
> +}
>
> -/* Return all blocks from free list to the OS. */
> +/* Reduce free list to NUM blocks. */
> void
> -memory_block_pool::clear_free_list ()
> +memory_block_pool::reduce_free_list (int num)
> {
> - while (m_blocks)
> + gcc_checking_assert (num
Re: [PATCH v3] Extend the simd function attribute
On 11/20/19 7:54 AM, Szabolcs Nagy wrote:
> On 14/11/2019 20:23, Szabolcs Nagy wrote:
>> Sorry v2 had a bug.
>>
>> v2: added documentation and tests.
>> v3: fixed expand_simd_clones so different isa variants are actually
>> generated.
>>
>> GCC currently supports two ways to declare the availability of vector
>> variants of a scalar function:
>>
>>#pragma omp declare simd
>>void f (void);
>>
>> and
>>
>>__attribute__ ((simd))
>>void f (void);
>>
>> However these declare a set of symbols that are different simd variants
>> of f, so a library either provides definitions for all those symbols or
>> it cannot use these declarations. (The set of declared symbols can be
>> narrowed down with additional omp clauses, but not enough to allow
>> declaring a single symbol. OpenMP 5 has a declare variant feature that
>> allows declaring more specific simd variants, but it is complicated and
>> still requires gcc or vendor extension for unambiguous declarations.)
>>
>> This patch extends the gcc specific simd attribute such that it can
>> specify a single vector variant of simple scalar functions (functions
>> that only take and return scalar integer or floating type values):
>>
>>__attribute__ ((simd (mask, simdlen, simdabi, name
> ping.
>
> so the comments so far
>
> - make all attribute arguments mandatory (e.g don't allow
>simd(mask, simdlen)), this is fine with me if others agree.
works for me :)
> - support the linear clause for pointer arguments (sincos).
>this requires listing arguments to which linear applies,
>i would only want to do that if there is a hope that it
>will ever be useful (currently gcc won't vectorize calls
>with pointer arguments, but maybe it should?).
If the C attribute inherit the properties of `declare simd` (or the
`variant` equivalent), it means that the function can be invoked
concurrently. This to me is enough to say that the following loop is
vectorizable (provided that the presence of vector sincos is the only
condition that prevents the loop from vectorizing)
void sincos(double , double *, double *) __attribute__((simd(noinbranch,
2, {1,2} /*linear*/, "n", "_ZGVnN2vl8l8_sincos"));
...
for (int i=...)
sincos(in[i], [i], [i]);
So overall, yes, I think a compiler should vectorize this example.
Please let me know if I am missing anything.
Side question: what would be the behavior of the attribute when attached
to a function definition? Are you expecting the compiler to
auto-vectorize the function? Given that the attribute is needed only for
interfacing libraries, I wouldn't recommend to use to auto-vectorize
functions. I am asking because I think you mentioned that the attribute
mimics the OpenMP clause...
> i don't know
>of a precedent for "list of integers" used in the attribute
>syntax, so i wonder what's the right way to do it.
>
> - plain simd should have fixed abi for a given architecture:
>aarch64 can of course do this, but if we include sve, then
>libmvec with plain simd attr won't be testable with gcc-10
>since gcc-10 does not support simd attr for sve, so we still
>need the attribute extension to do work on libmvec.
>
> any more comments on supporting linear clause in simd attr?
> or if the posted patch is reasonable as is?
Hum, I can't find the patch update in your last message...
>> where mask is "inbranch" or "notinbranch" like now, simdlen is an int
>> with the same meaning as in omp declare simd and simdabi is a string
>> specifying the call ABI (which the intel vector ABI calls ISA). The
>> name is optional and allows a library to use a different symbol name
>> than what the vector ABI specifies.
>>
>> The simd attribute currently can be used for both declarations and
>> definitions, in the latter case the simd varaints of the function are
>> generated, which should work with the extended simd attribute too.
>>
>> Tested on aarch64-linux-gnu and x86_64-linux-gnu.
>>
>> gcc/ChangeLog:
>>
>> 2019-11-14 Szabolcs Nagy
>>
>> * cgraph.h (struct cgraph_simd_clone): Add simdname field.
>> * doc/extend.texi: Update the simd attribute documentation.
>> * tree.h (OMP_CLAUSE__SIMDABI__EXPR): Define.
>> (OMP_CLAUSE__SIMDNAME__EXPR): Define.
>> * tree.c (walk_tree_1): Handle new omp clauses.
>> * tree-core.h (enum omp_clause_code): Likewise.
>> * tree-nested.c (convert_nonlocal_omp_clauses): Likewise.
>> * tree-pretty-print.c (dump_omp_clause): Likewise.
>> * omp-low.c (scan_sharing_clauses): Likewise.
>> * omp-simd-clone.c (simd_clone_clauses_extract): Likewise.
>> (simd_clone_mangle): Handle simdname.
>> (expand_simd_clones): Reset vecsize_mangle when generating clones.
>> * config/aarch64/aarch64.c
>> (aarch64_simd_clone_compute_vecsize_and_simdlen): Warn about
>> unsupported SIMD ABI.
>> * config/i386/i386.c
>> (ix86_simd_clone_compute_vecsize_and_simdlen): Likewise.
>>
>>
[PATCH] Fix attribute((section)) with -flto
When building an executable with LTO, GCC effectively ignores
__attribute__((section)) on C++ inline member functions. Moving such
functions into the .text section seems to be intentional, but I think
ignoring the section attribute is unintentional:
https://gcc.gnu.org/ml/gcc-patches/2010-07/msg00580.html
> From: Jan Hubicka
>
> The patch uncovered two latent problems. [...] Also I think we
> should clear NAMED_SECTION for comdats to conserve size. This is the
> cgraph_make_decl_local change.
Stop resetting the section of localized comdat symbols. This fixes
__attribute__((section)) with -flto.
Testing: Bootstrap on x86_64-linux-gnu with --disable-multilib
--enable-checking=release --enable-languages=c,c++. Observe no change in
test results (aside from the added tests).
2019-11-12 Matthew Glazar
* gcc/ipa-visibility.c (localize_node): Never set section_name to NULL.
---
gcc/ipa-visibility.c | 4 ---
.../ext/section-class-inline-function-lto.C | 17 +++
.../ext/section-class-inline-function.C | 25
gcc/testsuite/lib/scanasm.exp | 30 ++-
4 files changed, 71 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/ext/section-class-inline-function-lto.C
create mode 100644 gcc/testsuite/g++.dg/ext/section-class-inline-function.C
diff --git gcc/ipa-visibility.c gcc/ipa-visibility.c
index f470465f935..cf4c9101a2a 100644
--- gcc/ipa-visibility.c
+++ gcc/ipa-visibility.c
@@ -571,8 +571,6 @@ localize_node (bool whole_program, symtab_node *node)
next != node; next = next->same_comdat_group)
{
next->set_comdat_group (NULL);
- if (!next->alias)
-next->set_section (NULL);
if (!next->transparent_alias)
next->make_decl_local ();
next->unique_name
@@ -595,8 +593,6 @@ localize_node (bool whole_program, symtab_node *node)
if (TREE_PUBLIC (node->decl))
node->set_comdat_group (NULL);
- if (DECL_COMDAT (node->decl) && !node->alias)
-node->set_section (NULL);
if (!node->transparent_alias)
{
node->resolution = LDPR_PREVAILING_DEF_IRONLY;
diff --git gcc/testsuite/g++.dg/ext/section-class-inline-function-lto.C
gcc/testsuite/g++.dg/ext/section-class-inline-function-lto.C
new file mode 100644
index 000..4567d03a512
--- /dev/null
+++ gcc/testsuite/g++.dg/ext/section-class-inline-function-lto.C
@@ -0,0 +1,17 @@
+// attribute((section)) should affect inline member functions even with -flto.
+
+// { dg-do link }
+// { dg-require-effective-target lto }
+// { dg-require-named-sections "" }
+// { dg-options "-flto --save-temps" }
+
+// { dg-final { scan-lto-assembler-symbol-section {callee}
{^(\.testsection|__TEXT,__testsection)$} } }
+#include "section-class-inline-function.C"
+
+// { dg-final { cleanup-saved-temps } }
+
+int
+main()
+{
+ return f();
+}
diff --git gcc/testsuite/g++.dg/ext/section-class-inline-function.C
gcc/testsuite/g++.dg/ext/section-class-inline-function.C
new file mode 100644
index 000..01a12d02aff
--- /dev/null
+++ gcc/testsuite/g++.dg/ext/section-class-inline-function.C
@@ -0,0 +1,25 @@
+// attribute((section)) should affect inline member functions.
+
+// { dg-do compile }
+// { dg-require-named-sections "" }
+
+#if defined(__APPLE__)
+#define TESTSECTION "__TEXT,__testsection"
+#else
+#define TESTSECTION ".testsection"
+#endif
+
+// { dg-final { scan-assembler-symbol-section {callee}
{^(\.testsection|__TEXT,__testsection)$} } }
+struct s {
+ __attribute__((section(TESTSECTION)))
+ int callee()
+ {
+return 0;
+ }
+};
+
+int
+f()
+{
+ return s().callee();
+}
diff --git gcc/testsuite/lib/scanasm.exp gcc/testsuite/lib/scanasm.exp
index e9ebf52d77e..2eeb5b1e1fb 100644
--- gcc/testsuite/lib/scanasm.exp
+++ gcc/testsuite/lib/scanasm.exp
@@ -183,6 +183,29 @@ proc scan-assembler-symbol-section { args } {
$expected_section_pattern
}
+# Check that symbols are emitted in the desired section.
+# Like scan-assembler-symbol-section, but using the assembly output
generated by
+# the compiler with '-flto --save-temps'.
+#
+# Example:
+#
+# // All numbered functions (func1, func2, etc.) must be in the .text
section or
+# // in a .text sub-section (like .text._func1).
+# { dg-final { scan-lto-assembler-symbol-section {^_func[1-5]$}
{^\.text($|\.)} } }
+
+proc scan-lto-assembler-symbol-section { args } {
+set testcase [testname-for-summary]
+set output_file [lto_assembly_output_file $testcase]
+set symbol_pattern [lindex $args 0]
+set expected_section_pattern [lindex $args 1]
+dg-scan-symbol-section \
+"scan-lto-assembler-symbol-section" \
+$testcase \
+$output_file \
+$symbol_pattern \
+$expected_section_pattern
+}
+
# Check that symbols are emitted in the desired section.
#
# Symbols and sections are interpreted as regexp patterns.
@@ -724,11 +747,16 @@ proc dg-function-on-line { args } {
proc scan-lto-assembler { args } {
set testcase
[PATCH] Simplify testing symbol sections
While fixing some bugs in __attribute__((section)), I found it difficult
to write tests. Make testing easier: introduce the
scan-assembler-symbol-section and scan-symbol-section helpers. See
in-line documentation for details.
Testing:
* Run `make check` on x86_64-linux-gnu with --disable-multilib
--enable-checking=release --enable-languages=c,c++. Observe no new
failures in test results.
* Run `make check` on macOS x86_64-apple-darwin16.7.0 with
--disable-multilib --enable-checking=release --enable-languages=c,c++.
Observe no new failures in test results.
* Run test-framework.exp with CHECK_TEST_FRAMEWORK=1, and post-process
results with test-framework.awk. Observe no new failures test
results.
2019-11-12 Matthew Glazar
* gcc/testsuite/lib/scanasm.exp (dg-scan): Extract file globbing
code ...
(dg_glob_remote): ... into this new procedure.
(scan-assembler-symbol-section): Define.
(scan-symbol-section): Define.
---
gcc/testsuite/g++.dg/gomp/tls-5.C | 2 +
gcc/testsuite/g++.dg/opt/const4.C | 3 +-
gcc/testsuite/gcc.dg/20021029-1.c | 1 +
gcc/testsuite/gcc.dg/array-quals-1.c | 20 ++
gcc/testsuite/gcc.dg/darwin-sections.c| 2 +
gcc/testsuite/gcc.dg/pr25376.c| 1 +
.../dg-scan-symbol-section-1-exp-F.S | 13 ++
.../dg-scan-symbol-section-2-exp-F.S | 9 +
.../dg-scan-symbol-section-3-exp-F.S | 10 +
.../dg-scan-symbol-section-exp-P.S| 50 +
.../gcc.test-framework/test-framework.exp | 3 +-
gcc/testsuite/lib/scanasm.exp | 184 +-
12 files changed, 292 insertions(+), 6 deletions(-)
create mode 100644
gcc/testsuite/gcc.test-framework/dg-scan-symbol-section-1-exp-F.S
create mode 100644
gcc/testsuite/gcc.test-framework/dg-scan-symbol-section-2-exp-F.S
create mode 100644
gcc/testsuite/gcc.test-framework/dg-scan-symbol-section-3-exp-F.S
create mode 100644
gcc/testsuite/gcc.test-framework/dg-scan-symbol-section-exp-P.S
diff --git gcc/testsuite/g++.dg/gomp/tls-5.C gcc/testsuite/g++.dg/gomp/tls-5.C
index e83ff1179e6..a1d3120fbfb 100644
--- gcc/testsuite/g++.dg/gomp/tls-5.C
+++ gcc/testsuite/g++.dg/gomp/tls-5.C
@@ -1,6 +1,8 @@
// The reference temp should be TLS, not normal data.
// { dg-require-effective-target c++11 }
// { dg-final { scan-assembler-not "\\.data" { target tls_native
xfail powerpc-*-aix* } } }
+// { dg-final { scan-assembler-symbol-section {^_?ir$} {^\.tbss} } }
+// { dg-final { scan-assembler-symbol-section {^_?_ZGR2ir_$} {^\.tdata} } }
extern int&& ir;
#pragma omp threadprivate (ir)
diff --git gcc/testsuite/g++.dg/opt/const4.C gcc/testsuite/g++.dg/opt/const4.C
index 883c24b55fc..51d5313a312 100644
--- gcc/testsuite/g++.dg/opt/const4.C
+++ gcc/testsuite/g++.dg/opt/const4.C
@@ -3,7 +3,8 @@
// that have it.
// { dg-do compile }
-const int a[] __attribute__ ((__used__)) = { 0, 1, 2, 3 };
+// { dg-final { scan-assembler-symbol-section {constant_variable}
{^\.(const|rodata)} } }
+const int constant_variable[] __attribute__ ((__used__)) = { 0, 1, 2, 3 };
// The MMIX port always switches to the .data section at the end of a file.
// { dg-final { scan-assembler-not "\\.data(?!\\.rel\\.ro)" { xfail
powerpc*-*-aix* mmix-*-* } } }
diff --git gcc/testsuite/gcc.dg/20021029-1.c gcc/testsuite/gcc.dg/20021029-1.c
index f11a6e4a920..c8ae4aa60e2 100644
--- gcc/testsuite/gcc.dg/20021029-1.c
+++ gcc/testsuite/gcc.dg/20021029-1.c
@@ -3,6 +3,7 @@
/* { dg-do compile { target fpic } } */
/* { dg-options "-O2 -fpic" } */
/* { dg-final { scan-assembler-not ".data.rel.ro.local" } } */
+/* { dg-final { scan-assembler-symbol-section {ar} {^\.(const|rodata)} } } */
/* { dg-require-effective-target label_values } */
/* { dg-require-effective-target indirect_jumps } */
diff --git gcc/testsuite/gcc.dg/array-quals-1.c
gcc/testsuite/gcc.dg/array-quals-1.c
index 3981c916021..819bd24af76 100644
--- gcc/testsuite/gcc.dg/array-quals-1.c
+++ gcc/testsuite/gcc.dg/array-quals-1.c
@@ -6,26 +6,46 @@
/* { dg-options "-Wno-discarded-array-qualifiers" } */
/* The MMIX port always switches to the .data section at the end of a file. */
/* { dg-final { scan-assembler-not "\\.data(?!\\.rel\\.ro)" { xfail
powerpc*-*-aix* mmix-*-* x86_64-*-mingw* } } } */
+/* { dg-final { scan-assembler-symbol-section {^_?a$}
{^\.(const|rodata)} } } */
static const int a[2] = { 1, 2 };
+/* { dg-final { scan-assembler-symbol-section {^_?a1$}
{^\.(const|rodata)} } } */
const int a1[2] = { 1, 2 };
typedef const int ci;
+/* { dg-final { scan-assembler-symbol-section {^_?b$}
{^\.(const|rodata)} } } */
static ci b[2] = { 3, 4 };
+/* { dg-final { scan-assembler-symbol-section {^_?b1$}
{^\.(const|rodata)} } } */
ci b1[2] = { 3, 4 };
typedef int ia[2];
+/* { dg-final { scan-assembler-symbol-section {^_?c$}
{^\.(const|rodata)} } } */
static const ia c = { 5, 6 };
+/* { dg-final { scan-assembler-symbol-section {^_?c1$}
{^\.(const|rodata)} } } */
const ia c1 = { 5, 6 };
PR92608 - ICE: Segmentation fault (in find_loop_guard)
Hi,
The issue seems to happen with -O1, because header only contains phi:
[local count: 118111600]:
# iter.12_9 = PHI <0(2), iter.12_10(10)>
and thus we hit segfault in following hunk in find_loop_guard:
else
{
cond = dyn_cast (last_stmt (header));
if (! cond)
return NULL;
extract_true_false_edges_from_block (header, , );
since last_stmt (header) returns NULL.
The attached patch, simply punts if last_stmt returns NULL, which
avoids the segfault.
Does it look OK ?
Thanks,
Prathamesh
diff --git a/gcc/tree-ssa-loop-unswitch.c b/gcc/tree-ssa-loop-unswitch.c
index e60019db946..1439c8556ba 100644
--- a/gcc/tree-ssa-loop-unswitch.c
+++ b/gcc/tree-ssa-loop-unswitch.c
@@ -587,7 +587,10 @@ find_loop_guard (class loop *loop)
next = single_succ (header);
else
{
- cond = dyn_cast (last_stmt (header));
+ gimple *last = last_stmt (header);
+ if (!last)
+ return NULL;
+ cond = dyn_cast (last);
if (! cond)
return NULL;
extract_true_false_edges_from_block (header, , );
Re: C++ PATCH for c++/91363 - P0960R3: Parenthesized initialization of aggregates
On 11/20/19 8:37 PM, Marek Polacek wrote:
On Tue, Nov 19, 2019 at 05:33:09PM -0500, Jason Merrill wrote:
On 11/19/19 1:44 AM, Marek Polacek wrote:
It also looks like you're using the LOOKUP flag to mean two different
things:
1) try to treat parenthesized args as an aggregate initializer
(build_new_method_call_1)
2) treat this CONSTRUCTOR as coming from parenthesized args
(store_init_value/digest_init)
Correct.
Why is the flag needed for #1? When do we not want to try to treat the args
as an aggregate initializer?
There are cases where treating the args as an aggregate initializer causes
spurious overload resolution successes, e.g.
void swap(int&, int&);
int& get();
struct pair {
void swap(pair&) noexcept(noexcept(swap(get(), get( { } // { dg-error "no
matching function for call" }
};
There are no viable candidates for pair::swap (# of args mismatch) but since
pair is an aggregate, build_new_method_call_1 would return a CONSTRUCTOR so
overload resolution would succeed. Another case had to do with SFINAE and
decltype where we didn't evaluate the arg, but succeeding in the
no-viable-function case caused the compiler to choose the wrong function.
Hmm, but then the parenthesized list is arguments for swap, not an
initializer for a single argument of swap. That would be using it for
copy-initialization, and we only want to treat parenthesized args as an
aggregate initializer in direct-initialization. Can we check for
direct-init (i.e. !LOOKUP_ONLYCONVERTING) instead?
Unfortunately that doesn't work. We call build_new_method_call from context
where LOOKUP_ONLYCONVERTING isn't set and so it would still break things.
How so? If we call it for a constructor, surely we can check that flag
to distinguish between copy- and direct-initialization, or I'd expect
wrong behavior wrt explicit.
+ if (BRACE_ENCLOSED_INITIALIZER_P (exp))
+{
+ gcc_assert (cxx_dialect >= cxx2a);
+ return finish_compound_literal (type, exp, complain,
+ fcl_functional_paren);
+}
How about handling this in build_cplus_new instead of places that also call
build_cplus_new?
Is it really what we want? We now have two spots where we need to handle
the case when build_special_member_call returns a BRACE_ENCLOSED_INITIALIZER_P
but build_cplus_new is called in many other spots where we don't expect to see
a CONSTRUCTOR.
I think build_cplus_new should be able to handle whatever
build_special_member_call returns.
@@ -921,8 +921,20 @@ perform_member_init (tree member, tree init)
inform (DECL_SOURCE_LOCATION (member),
"%q#D should be initialized", member );
}
- finish_expr_stmt (build_aggr_init (decl, init, flags,
-tf_warning_or_error));
+ init = build_aggr_init (decl, init, flags, tf_warning_or_error);
+ /* In C++20, a member initializer list can be initializing an
+aggregate from a parenthesized list of values:
+
+ struct S {
+A aggr;
+S() : aggr(1, 2, 3) { }
+ };
+
+ In such case, build_aggr_init will build up an INIT_EXPR like
+ we do for aggr{1, 2, 3}, so that build_data_member_initialization
+ can grok it. */
+ if (TREE_CODE (init) != INIT_EXPR)
+ finish_expr_stmt (init);
Why don't we want to finish_expr_stmt an INIT_EXPR?
@@ -1272,8 +1274,13 @@ digest_init_r (tree type, tree init, int nested, int
flags,
inform (loc, "remove %<{ }%> around initializer");
}
else if (flag_checking)
- /* We should have fixed this in reshape_init. */
- gcc_unreachable ();
+ /* We should have fixed this in reshape_init. Except that we
+ don't reshape parenthesized lists where brace elision is
+ not permitted. */
+ {
+ gcc_assert (flags & LOOKUP_AGGREGATE_PAREN_INIT);
+ return error_mark_node;
We shouldn't get here for parenthesized lists, either, I thnk; we should
have called the copy constructor rather than try aggregate initialization.
@@ -1319,12 +1326,19 @@ digest_init_r (tree type, tree init, int nested, int
flags,
tree
digest_init (tree type, tree init, tsubst_flags_t complain)
{
- return digest_init_r (type, init, 0, LOOKUP_IMPLICIT, complain);
+ int flags = LOOKUP_IMPLICIT;
+ if (BRACE_ENCLOSED_INITIALIZER_P (init)
+ && CONSTRUCTOR_IS_PAREN_INIT (init))
+flags |= LOOKUP_AGGREGATE_PAREN_INIT;
+ return digest_init_r (type, init, 0, flags, complain);
}
tree
digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain)
{
+ if (BRACE_ENCLOSED_INITIALIZER_P (init)
+ && CONSTRUCTOR_IS_PAREN_INIT (init))
+flags |= LOOKUP_AGGREGATE_PAREN_INIT;
Maybe do this in digest_init_r? What happens if we have a braced
aggregate
Re: [PATCH] Fix libgo build (was Re: [PATCH v3] PR85678: Change default to -fno-common)
On Wed, Nov 20, 2019 at 4:18 PM Jakub Jelinek wrote: > > On Tue, Nov 05, 2019 at 05:17:10PM +, Wilco Dijkstra wrote: > > Passes bootstrap and regress on AArch64 and x64. OK for commit? > > This broke bootstrap on x86_64-linux as well as i686-linux (guess all > targets that go supports). > The following patch fixes it for me, though not sure which *.c file is best > and what location in there for the definition. > With this bootstrap succeeded on both x86_64-linux and i686-linux, regtest > is still pending, but without it it just failed to link libgo. I just committed a fix for this. I put the variable in libgo/runtime/stack.c. Sorry about the difficulties. Ian
libgo patch committed: Declare runtime_usestackmaps in .c file, not .h file
This libgo patch declares runtime_usestackmaps in stack.c, not
runtime.h. This fixes https://gcc.gnu.org/PR92605. Bootstrapped and
ran Go testsuite on x86_64-pc-linux-gnu. Committed to mainline.
Ian
Index: gcc/go/gofrontend/MERGE
===
--- gcc/go/gofrontend/MERGE (revision 278539)
+++ gcc/go/gofrontend/MERGE (working copy)
@@ -1,4 +1,4 @@
-9cc7092b84c38d77d98ed856f1f613a6ca27122d
+017830d2a4bd2efbddf5e841ba9ccff8acf9d7c8
The first line of this file holds the git revision number of the last
merge done from the gofrontend repository.
Index: libgo/runtime/runtime.h
===
--- libgo/runtime/runtime.h (revision 277299)
+++ libgo/runtime/runtime.h (working copy)
@@ -475,7 +475,7 @@ bool scanstackwithmap(void*)
bool doscanstack(G*, void*)
__asm__("runtime.doscanstack");
-bool runtime_usestackmaps;
+extern bool runtime_usestackmaps;
bool probestackmaps(void)
__asm__("runtime.probestackmaps");
Index: libgo/runtime/stack.c
===
--- libgo/runtime/stack.c (revision 277299)
+++ libgo/runtime/stack.c (working copy)
@@ -16,6 +16,8 @@ extern void * __splitstack_find_context
#endif
+bool runtime_usestackmaps;
+
// Calling unwind_init in doscanstack only works if it does not do a
// tail call to doscanstack1.
#pragma GCC optimize ("-fno-optimize-sibling-calls")
libgo patch committed: Use type aliases for time struct field types
This patch to the libgo mksysinfo script uses type aliases for time
struct field types. It also fixes a case where grep wasn't
redirecting to /dev/null. This lets some syscall package types be
identical to some golang.org/x/sys/unix types, and fixes
https://golang.org/issue/35713. Bootstrapped and ran Go testsuite on
x86_64-pc-linux-gnu. Committed to mainline.
Ian
Index: gcc/go/gofrontend/MERGE
===
--- gcc/go/gofrontend/MERGE (revision 278470)
+++ gcc/go/gofrontend/MERGE (working copy)
@@ -1,4 +1,4 @@
-245904ac148f15db530fb8d50f526c47d08024c5
+9cc7092b84c38d77d98ed856f1f613a6ca27122d
The first line of this file holds the git revision number of the last
merge done from the gofrontend repository.
Index: libgo/mksysinfo.sh
===
--- libgo/mksysinfo.sh (revision 277299)
+++ libgo/mksysinfo.sh (working copy)
@@ -436,8 +436,8 @@ fi
timeval=`grep '^type _timeval ' gen-sysinfo.go`
timeval_sec=`echo $timeval | sed -n -e 's/^.*tv_sec \([^ ]*\);.*$/\1/p'`
timeval_usec=`echo $timeval | sed -n -e 's/^.*tv_usec \([^ ]*\);.*$/\1/p'`
-echo "type Timeval_sec_t $timeval_sec" >> ${OUT}
-echo "type Timeval_usec_t $timeval_usec" >> ${OUT}
+echo "type Timeval_sec_t = $timeval_sec" >> ${OUT}
+echo "type Timeval_usec_t = $timeval_usec" >> ${OUT}
echo $timeval | \
sed -e 's/type _timeval /type Timeval /' \
-e 's/tv_sec *[a-zA-Z0-9_]*/Sec Timeval_sec_t/' \
@@ -449,8 +449,8 @@ if test "$timespec" = ""; then
fi
timespec_sec=`echo $timespec | sed -n -e 's/^.*tv_sec \([^ ]*\);.*$/\1/p'`
timespec_nsec=`echo $timespec | sed -n -e 's/^.*tv_nsec \([^ ]*\);.*$/\1/p'`
-echo "type Timespec_sec_t $timespec_sec" >> ${OUT}
-echo "type Timespec_nsec_t $timespec_nsec" >> ${OUT}
+echo "type Timespec_sec_t = $timespec_sec" >> ${OUT}
+echo "type Timespec_nsec_t = $timespec_nsec" >> ${OUT}
echo $timespec | \
sed -e 's/^type ___timespec /type Timespec /' \
-e 's/^type _timespec /type Timespec /' \
@@ -461,8 +461,8 @@ timestruc=`grep '^type _timestruc_t ' ge
if test "$timestruc" != ""; then
timestruc_sec=`echo $timestruc | sed -n -e 's/^.*tv_sec \([^ ]*\);.*$/\1/p'`
timestruc_nsec=`echo $timestruc | sed -n -e 's/^.*tv_nsec \([^
]*\);.*$/\1/p'`
- echo "type Timestruc_sec_t $timestruc_sec" >> ${OUT}
- echo "type Timestruc_nsec_t $timestruc_nsec" >> ${OUT}
+ echo "type Timestruc_sec_t = $timestruc_sec" >> ${OUT}
+ echo "type Timestruc_nsec_t = $timestruc_nsec" >> ${OUT}
echo $timestruc | \
sed -e 's/^type _timestruc_t /type Timestruc /' \
-e 's/tv_sec *[a-zA-Z0-9_]*/Sec Timestruc_sec_t/' \
@@ -1126,7 +1126,7 @@ statfs=`grep '^type _statfs64 ' gen-sysi
if test "$statfs" = ""; then
statfs=`grep '^type _statfs ' gen-sysinfo.go || true`
fi
-if ! echo "$statfs" | grep f_flags; then
+if ! echo "$statfs" | grep f_flags >/dev/null 2>&1; then
statfs=`echo "$statfs" | sed -e 's/f_spare \[4+1\]\([^ ;]*\)/f_flags \1;
f_spare [3+1]\1/'`
fi
echo "$statfs" | sed -e 's/type _statfs64/type Statfs_t/' \
Re: [PATCH] Fix libgo build (was Re: [PATCH v3] PR85678: Change default to -fno-common)
On Thu, Nov 21, 2019 at 01:41:47AM +0100, Rainer Orth wrote: > Same on sparc-sun-solaris2.11 and i386-pc-solaris2.11. > > There where quite a number of non-Go regressions all over the place. > Many are like this: > > FAIL: gcc.c-torture/execute/complex-6.c -O0 (test for excess errors) > > ld: warning: symbol 'err' has differing types: > (file /var/tmp//ccWQCyMc.o type=OBJT; file /lib/libc.so type=FUNC); > /var/tmp//ccWQCyMc.o definition taken On i686-linux, I see just: +FAIL: gcc.target/i386/memcpy-strategy-1.c scan-assembler-times movdqa 4 +FAIL: gcc.target/i386/memcpy-strategy-2.c scan-assembler-times movdqa 4 +FAIL: gcc.target/i386/memcpy-vector_loop-1.c scan-assembler-times movdqa 4 +FAIL: gcc.target/i386/pr69052.c scan-assembler-not leal[ \\t]ind@GOTOFF(%[^,]*), % +FAIL: gfortran.dg/global_vars_f90_init.f90 -O0 (test for excess errors) +UNRESOLVED: gfortran.dg/global_vars_f90_init.f90 -O0 compilation failed to produce executable +FAIL: gfortran.dg/global_vars_f90_init.f90 -O1 (test for excess errors) +UNRESOLVED: gfortran.dg/global_vars_f90_init.f90 -O1 compilation failed to produce executable +FAIL: gfortran.dg/global_vars_f90_init.f90 -O2 (test for excess errors) +UNRESOLVED: gfortran.dg/global_vars_f90_init.f90 -O2 compilation failed to produce executable +FAIL: gfortran.dg/global_vars_f90_init.f90 -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions (test for excess errors) +UNRESOLVED: gfortran.dg/global_vars_f90_init.f90 -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions compilation failed to produce executable +FAIL: gfortran.dg/global_vars_f90_init.f90 -O3 -g (test for excess errors) +UNRESOLVED: gfortran.dg/global_vars_f90_init.f90 -O3 -g compilation failed to produce executable +FAIL: gfortran.dg/global_vars_f90_init.f90 -Os (test for excess errors) +UNRESOLVED: gfortran.dg/global_vars_f90_init.f90 -Os compilation failed to produce executable Jakub
Re: [C++ PATCH] c++/92450 - ICE with invalid nested name specifier.
On 11/20/19 8:49 PM, Marek Polacek wrote:
On Tue, Nov 19, 2019 at 05:34:45PM -0500, Jason Merrill wrote:
On 11/18/19 7:04 PM, Marek Polacek wrote:
The newly added diagnostic causes an ICE because the new grokdeclarator call
returned error_mark_node and DECL_SOURCE_LOCATION crashes on that. So don't
attempt to print the new diagnostic if we've encountered something not
resembling a bit-field.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2019-11-18 Marek Polacek
PR c++/92450 - ICE with invalid nested name specifier.
* parser.c (cp_parser_member_declaration): Bail out for invalid code.
* g++.dg/parse/crash71.C: New test.
diff --git gcc/cp/parser.c gcc/cp/parser.c
index c473e7fd92f..caed6946977 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -25052,6 +25052,9 @@ cp_parser_member_declaration (cp_parser* parser)
tree d = grokdeclarator (declarator, _specifiers,
BITFIELD, /*initialized=*/false,
);
+ /* Hopelessly invalid code, give up. */
+ if (error_operand_p (d))
+ goto out;
error_at (DECL_SOURCE_LOCATION (d),
"bit-field %qD has non-integral type %qT",
d, TREE_TYPE (d));
Don't we still want the rest of the error-recovery code there, i.e. skipping
to the end of the statement?
Sure, that works too, we just emit fewer diagnostics. But maybe that's not
that interesting at this point, because we've already given an error.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
OK, thanks.
2019-11-20 Marek Polacek
PR c++/92450 - ICE with invalid nested name specifier.
* parser.c (cp_parser_member_declaration): Don't attempt to print
erroneous bit-field diagnostic if grokdeclarator returns
error_mark_node.
* g++.dg/parse/crash71.C: New test.
diff --git gcc/cp/parser.c gcc/cp/parser.c
index c473e7fd92f..ad35945f954 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -25052,9 +25052,10 @@ cp_parser_member_declaration (cp_parser* parser)
tree d = grokdeclarator (declarator, _specifiers,
BITFIELD, /*initialized=*/false,
);
- error_at (DECL_SOURCE_LOCATION (d),
- "bit-field %qD has non-integral type %qT",
- d, TREE_TYPE (d));
+ if (!error_operand_p (d))
+ error_at (DECL_SOURCE_LOCATION (d),
+ "bit-field %qD has non-integral type %qT",
+ d, TREE_TYPE (d));
cp_parser_skip_to_end_of_statement (parser);
/* Avoid "extra ;" pedwarns. */
if (cp_lexer_next_token_is (parser->lexer,
diff --git gcc/testsuite/g++.dg/parse/crash71.C
gcc/testsuite/g++.dg/parse/crash71.C
new file mode 100644
index 000..13f484801fe
--- /dev/null
+++ gcc/testsuite/g++.dg/parse/crash71.C
@@ -0,0 +1,11 @@
+// PR c++/92450 - ICE with invalid nested name specifier.
+
+typedef int C2;
+struct B1 {
+ struct B2 {
+ };
+};
+
+struct S6g {
+ C2 : B1:B2; // { dg-error "" }
+};
Re: [C++ PATCH] Fix concepts vs. PCH (PR c++/92458)
On 11/20/19 7:21 PM, Jakub Jelinek wrote: On Mon, Nov 18, 2019 at 02:41:48PM -0500, Jason Merrill wrote: 2019-11-11 Jakub Jelinek PR c++/92458 * constraint.cc: Include tree-hash-traits.h. (decl_tree_cache_traits): New type. (decl_tree_cache_map): New typedef. (decl_constraints): Change type to decl_tree_cache_map * from tree_cache_map *. Do we need to change other tree_cache_map uses, too? It looks like many of them map from decls. I guess it depends on whether those hash tables can have data in them across PCH save/restore. As an experiment, I've built stdc++.h.gch with -std=c++2a and put a breakpoint in c_common_read_pch after gt_pch_restore. Besides decl_constraints I've changed, I see also defarg_inst table with data on it, which means that defarg_inst lookups after PCH read might not find saved instantiations in the table. So, defarg_inst might be another candidate for decl_tree_cache_map, especially because PARM_DECLs are the keys in it. All the other C++ FE tree_cache_map hash tables are empty, so no idea if it is needed or not. If decl_tree_cache_map will be needed in more than one spot, I'll probably need to move it to some generic header. Most of them probably need it, for code that uses the relevant features. Except debug_type_map, which probably needs to use TYPE_UID. Or we might make default_hash_traits use DECL_UID for decls and TYPE_UID for types even if it doesn't do the more complex analysis of tree_operand_hash. Jason
Re: [PATCH] Fix libgo build (was Re: [PATCH v3] PR85678: Change default to -fno-common)
Hi Jakub, > On Tue, Nov 05, 2019 at 05:17:10PM +, Wilco Dijkstra wrote: >> Passes bootstrap and regress on AArch64 and x64. OK for commit? > > This broke bootstrap on x86_64-linux as well as i686-linux (guess all > targets that go supports). indeed: just saw it on Solaris with the native ld. > The following patch fixes it for me, though not sure which *.c file is best > and what location in there for the definition. I've placed it in runtime/go-unwind.c: probestackmaps is defined there which is declared close to runtime_usestackmaps. > With this bootstrap succeeded on both x86_64-linux and i686-linux, regtest > is still pending, but without it it just failed to link libgo. Same on sparc-sun-solaris2.11 and i386-pc-solaris2.11. There where quite a number of non-Go regressions all over the place. Many are like this: FAIL: gcc.c-torture/execute/complex-6.c -O0 (test for excess errors) ld: warning: symbol 'err' has differing types: (file /var/tmp//ccWQCyMc.o type=OBJT; file /lib/libc.so type=FUNC); /var/tmp//ccWQCyMc.o definition taken Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
[PATCH] Fix libgo build (was Re: [PATCH v3] PR85678: Change default to -fno-common)
On Tue, Nov 05, 2019 at 05:17:10PM +, Wilco Dijkstra wrote:
> Passes bootstrap and regress on AArch64 and x64. OK for commit?
This broke bootstrap on x86_64-linux as well as i686-linux (guess all
targets that go supports).
The following patch fixes it for me, though not sure which *.c file is best
and what location in there for the definition.
With this bootstrap succeeded on both x86_64-linux and i686-linux, regtest
is still pending, but without it it just failed to link libgo.
--- libgo/runtime/runtime.h.jj 2019-09-06 23:00:17.755294355 +0200
+++ libgo/runtime/runtime.h 2019-11-21 00:32:51.146992951 +0100
@@ -475,7 +475,7 @@ bool scanstackwithmap(void*)
bool doscanstack(G*, void*)
__asm__("runtime.doscanstack");
-bool runtime_usestackmaps;
+extern bool runtime_usestackmaps;
bool probestackmaps(void)
__asm__("runtime.probestackmaps");
--- libgo/runtime/proc.c.jj 2019-08-31 13:26:29.646735239 +0200
+++ libgo/runtime/proc.c2019-11-21 00:34:23.509610234 +0100
@@ -676,6 +676,8 @@ makeGContext(G* gp, byte* sp, uintptr sp
__go_makecontext(uc, kickoff, sp, (size_t)spsize);
}
+bool runtime_usestackmaps;
+
// The goroutine g is about to enter a system call.
// Record that it's not using the cpu anymore.
// This is called only from the go syscall library and cgocall,
Jakub
[PATCH] Don't put objects with flexible array members in small data
This patch is for PR target/92499, a bug reported by GLIBC maintainers
against nios2 target. The initial failure symptom was a linker error
resulting from use of GP-relative addressing on an object that wasn't
allocated in the small data section. It turns out that the real problem
is that the TARGET_IN_SMALL_DATA_P hook uses the function
int_size_in_bytes to check whether an object is "small", and this
function treats flexible array members as having size 0. All backends
except for tic6x that implement TARGET_IN_SMALL_DATA_P use a similar
implementation of the hook that has the same problem.
The solution here is to add a new function that returns -1 as the size
of any type with a flexible array member, and adjust all the affected
backends to use it. I swiped the predicate for identifying these types
from the C front end.
I regression-tested this on nios2-elf and nios2-linux-gnu. I'm not set
up to test any of the other affected back ends, but the code change is
trivial and identical to what I did for nios2. OK to commit? I'd like
to backport this patch to all active branches as well, once it is in on
trunk.
-Sandra
2019-11-20 Sandra Loosemore
Don't put objects with flexible array members in small data.
PR target/92499
gcc/c/
* c-decl.c (flexible_array_type_p): Move to common code.
gcc/
* config/alpha/alpha.c (alpha_in_small_data_p): Use
fixed_int_size_in_bytes.
* config/arc/arc.c (arc_in_small_data_p): Likewise.
* config/frv/frv.c (frv_in_small_data_p): Likewise.
* config/ia64/ia64.c (ia64_in_small_data_p): Likewise.
* config/lm32/lm32.c (lm32_in_small_data_p): Likewise.
* config/m32r/m32r.c (m32r_in_small_data_p): Likewise.
* config/microblaze/microblaze.c (microblaze_elf_in_small_data_p):
Likewise.
* config/mips/mips.c (mips_in_small_data_p): Likewise.
* config/nios2/nios2.c (nios2_in_small_data_p): Likewise.
* config/riscv/riscv.c (riscv_in_small_data_p): Likewise.
* config/rs6000/rs6000.c (rs6000_elf_in_small_data_p): Likewise.
* config/rx/rx.c (rx_in_small_data_p): Likewise.
* tree.c (int_size_in_bytes): Clarify comments.
(flexible_array_type_p): Move from C front end, and generalize
to handle fields in non-C structures.
(fixed_int_size_in_bytes): New function.
* tree.h (flexible_array_type_p): Declare.
(fixed_int_size_in_bytes): Declare.
gcc/testsuite/
* gcc.dg/pr92499.c: New.
Index: gcc/c/c-decl.c
===
--- gcc/c/c-decl.c (revision 278366)
+++ gcc/c/c-decl.c (working copy)
@@ -5667,39 +5667,6 @@ check_compound_literal_type (location_t
"defining a type in a compound literal is invalid in C++");
}
-/* Determine whether TYPE is a structure with a flexible array member,
- or a union containing such a structure (possibly recursively). */
-
-static bool
-flexible_array_type_p (tree type)
-{
- tree x;
- switch (TREE_CODE (type))
-{
-case RECORD_TYPE:
- x = TYPE_FIELDS (type);
- if (x == NULL_TREE)
- return false;
- while (DECL_CHAIN (x) != NULL_TREE)
- x = DECL_CHAIN (x);
- if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE
- && TYPE_SIZE (TREE_TYPE (x)) == NULL_TREE
- && TYPE_DOMAIN (TREE_TYPE (x)) != NULL_TREE
- && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (x))) == NULL_TREE)
- return true;
- return false;
-case UNION_TYPE:
- for (x = TYPE_FIELDS (type); x != NULL_TREE; x = DECL_CHAIN (x))
- {
- if (flexible_array_type_p (TREE_TYPE (x)))
- return true;
- }
- return false;
-default:
-return false;
- }
-}
-
/* Performs sanity checks on the TYPE and WIDTH of the bit-field NAME,
replacing with appropriate values if they are invalid. */
Index: gcc/config/alpha/alpha.c
===
--- gcc/config/alpha/alpha.c (revision 278366)
+++ gcc/config/alpha/alpha.c (working copy)
@@ -796,7 +796,7 @@ alpha_in_small_data_p (const_tree exp)
}
else
{
- HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
+ HOST_WIDE_INT size = fixed_int_size_in_bytes (TREE_TYPE (exp));
/* If this is an incomplete type with size 0, then we can't put it
in sdata because it might be too big when completed. */
Index: gcc/config/arc/arc.c
===
--- gcc/config/arc/arc.c (revision 278366)
+++ gcc/config/arc/arc.c (working copy)
@@ -8643,7 +8643,7 @@ arc_in_small_data_p (const_tree decl)
section. */
else if (TREE_PUBLIC (decl))
{
- size = int_size_in_bytes (TREE_TYPE (decl));
+ size = fixed_int_size_in_bytes (TREE_TYPE (decl));
return (size > 0 && size <= g_switch_value);
}
return false;
Index: gcc/config/frv/frv.c
===
--- gcc/config/frv/frv.c (revision 278366)
+++ gcc/config/frv/frv.c (working copy)
@@ -9319,7 +9319,7 @@ frv_in_small_data_p (const_tree decl)
return false;
}
-
Re: Make more bad uses of fallthrough attribute into pedwarns
On Wed, Nov 20, 2019 at 11:15:56PM +, Joseph Myers wrote: > Various bad uses of the [[fallthrough]] attribute are constraint > violations in C2x, so need pedwarns rather than warnings. > > This patch duly turns the relevant warnings into pedwarns. The > relevant code is not specific to C, and does not know which form the > attribute was given in ([[fallthrough]] or [[gnu::fallthrough]] or > __attribute__((fallthrough))), but as I understand it these usages are > also erroneous for C++ and it seems reasonable to give a pedwarn here > even when a form other than [[fallthrough]] is being used. > > The precise meaning of the standard wording about "The next statement > that would be executed" seems a but unclear in some corner cases; the > tests added keep to cases where it is clear whether or not the next > statement executed is of the required form. > > Bootstrapped with no regressions for x86_64-pc-linux-gnu. OK to commit > (the gimplify.c changes)? Can't approve but as the author of the code in question, I think this is OK. -- Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
[PATCH] rs6000: Don't split FP comparisons at expand time
We currently expand various floating point comparisons early, to some
sequences with cror insns and the like. This doesn't optimize well.
Change that to allow any of the 14 floating point comparisons in the
instruction stream, and split them after combine (at split1).
Tested on powerpc64-linux {-m32,-m64}; also tested with -ffast-math
(it doesn't change anything for that). All 14 "cstore" codes give
optimal code now. Not all "cbranch" codes do yet. An example that
does work fine is UNEQ:
fcmpu 0,1,2 # 7[c=4 l=4] *cmpdf_fpr/0
cror 2,0,1 # 23 [c=4 l=4] cceq_ior_compare_si/0
beqlr 0 # 24 [c=4 l=4] *creturn
but one that does not is UNGT:
fcmpu 0,1,2 # 7[c=4 l=4] *cmpdf_fpr/0
bgt 0,.L16 # 15 [c=4 l=4] *cbranch
bnulr 0 # 22 [c=4 l=4] *creturn
.L16:
Coming from gimple this is
_5 = a_3(D) u<= b_4(D);
_7 = ~_5;
_8 = a_3(D) unord b_4(D);
_9 = _7 | _8;
if (_9 != 0)
goto ; [50.00%]
else
goto ; [50.00%]
which could use some improvement.
Anyway, testing on p8le and p9le as well; will commit if that works fine.
Segher
2019-11-20 Segher Boessenkool
* config/rs6000/predicates.md (extra_insn_branch_comparison_operator):
New predicate.
* config/rs6000/rs6000-protos.h (rs6000_emit_fp_cror): New declaration.
* config/rs6000/rs6000.c (rs6000_generate_compare): Don't do anything
special for FP comparisons that need a cror instruction eventually.
(rs6000_emit_fp_cror): New function.
(rs6000_emit_sCOND): Expand all floating point comparisons to one
instruction, for normal FP modes, with HONOR_NANS.
(rs6000_emit_cbranch): Reformat.
* config/rs6000/rs6000.md (fp_rev): New iterator.
(fp_two): New iterator.
*_cc for fp_rev and GPR: New define_insn_and_split.
*_cc for fp_two and GPR: New define_insn_and_split.
*cbranch_2insn: New define_insn_and_split.
---
gcc/config/rs6000/predicates.md | 10
gcc/config/rs6000/rs6000-protos.h | 1 +
gcc/config/rs6000/rs6000.c| 98 ---
gcc/config/rs6000/rs6000.md | 78 +++
4 files changed, 130 insertions(+), 57 deletions(-)
diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
index f4ecc41..42c41b3 100644
--- a/gcc/config/rs6000/predicates.md
+++ b/gcc/config/rs6000/predicates.md
@@ -1143,6 +1143,16 @@ (define_predicate "branch_comparison_operator"
GET_MODE (XEXP (op, 0))),
1")))
+;; Return 1 if OP is a comparison that needs an extra instruction to do (a
+;; crlogical or an extra branch).
+(define_predicate "extra_insn_branch_comparison_operator"
+ (and (match_operand 0 "comparison_operator")
+ (match_test "GET_MODE (XEXP (op, 0)) == CCFPmode")
+ (match_code "ltgt,le,ge,unlt,ungt,uneq")
+ (match_test "validate_condition_mode (GET_CODE (op),
+ GET_MODE (XEXP (op, 0))),
+1")))
+
;; Return 1 if OP is an unsigned comparison operator.
(define_predicate "unsigned_comparison_operator"
(match_code "ltu,gtu,leu,geu"))
diff --git a/gcc/config/rs6000/rs6000-protos.h
b/gcc/config/rs6000/rs6000-protos.h
index 0dddb40..69e67ac 100644
--- a/gcc/config/rs6000/rs6000-protos.h
+++ b/gcc/config/rs6000/rs6000-protos.h
@@ -112,6 +112,7 @@ extern const char *rs6000_pltseq_template (rtx *, int);
extern enum rtx_code rs6000_reverse_condition (machine_mode,
enum rtx_code);
extern rtx rs6000_emit_eqne (machine_mode, rtx, rtx, rtx);
+extern rtx rs6000_emit_fp_cror (rtx_code, machine_mode, rtx);
extern void rs6000_emit_sCOND (machine_mode, rtx[]);
extern void rs6000_emit_cbranch (machine_mode, rtx[]);
extern char * output_cbranch (rtx, const char *, int, rtx_insn *);
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 0282ebd..2995348 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -13954,42 +13954,6 @@ rs6000_generate_compare (rtx cmp, machine_mode mode)
gen_rtx_COMPARE (comp_mode, op0, op1)));
}
- /* Some kinds of FP comparisons need an OR operation;
- under flag_finite_math_only we don't bother. */
- if (FLOAT_MODE_P (mode)
- && (!FLOAT128_IEEE_P (mode) || TARGET_FLOAT128_HW)
- && !flag_finite_math_only
- && (code == LE || code == GE
- || code == UNEQ || code == LTGT
- || code == UNGT || code == UNLT))
-{
- enum rtx_code or1, or2;
- rtx or1_rtx, or2_rtx, compare2_rtx;
- rtx or_result = gen_reg_rtx (CCEQmode);
-
- switch (code)
- {
- case LE: or1 = LT; or2 = EQ; break;
- case GE: or1 = GT; or2 = EQ; break;
- case UNEQ: or1 = UNORDERED; or2 = EQ; break;
- case LTGT: or1
Make more bad uses of fallthrough attribute into pedwarns
Various bad uses of the [[fallthrough]] attribute are constraint
violations in C2x, so need pedwarns rather than warnings.
This patch duly turns the relevant warnings into pedwarns. The
relevant code is not specific to C, and does not know which form the
attribute was given in ([[fallthrough]] or [[gnu::fallthrough]] or
__attribute__((fallthrough))), but as I understand it these usages are
also erroneous for C++ and it seems reasonable to give a pedwarn here
even when a form other than [[fallthrough]] is being used.
The precise meaning of the standard wording about "The next statement
that would be executed" seems a but unclear in some corner cases; the
tests added keep to cases where it is clear whether or not the next
statement executed is of the required form.
Bootstrapped with no regressions for x86_64-pc-linux-gnu. OK to commit
(the gimplify.c changes)?
gcc:
2019-11-20 Joseph Myers
* gimplify.c (expand_FALLTHROUGH_r, expand_FALLTHROUGH): Use
pedwarn instead of warning_at for fallthrough not preceding a case
or default label.
gcc/c-family:
2019-11-20 Joseph Myers
* c-attribs.c (handle_fallthrough_attribute): Use pedwarn instead
of warning.
gcc/testsuite:
2019-11-20 Joseph Myers
* gcc.dg/c2x-attr-fallthrough-6.c: New test. Split out from
c2x-attr-fallthrough-3.c.
* gcc.dg/c2x-attr-fallthrough-1.c: Add more tests.
* gcc.dg/c2x-attr-fallthrough-2.c: Update expected diagnostics.
* gcc.dg/c2x-attr-fallthrough-3.c: Split inside-switch part of
test out to c2x-attr-fallthrough-6.c.
Index: gcc/c-family/c-attribs.c
===
--- gcc/c-family/c-attribs.c(revision 278510)
+++ gcc/c-family/c-attribs.c(working copy)
@@ -4117,7 +4117,7 @@ tree
handle_fallthrough_attribute (tree *, tree name, tree, int,
bool *no_add_attrs)
{
- warning (OPT_Wattributes, "%qE attribute ignored", name);
+ pedwarn (input_location, OPT_Wattributes, "%qE attribute ignored", name);
*no_add_attrs = true;
return NULL_TREE;
}
Index: gcc/gimplify.c
===
--- gcc/gimplify.c (revision 278510)
+++ gcc/gimplify.c (working copy)
@@ -2405,8 +2405,8 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p,
gsi_next ();
}
if (!found)
- warning_at (loc, 0, "attribute % not preceding "
- "a case label or default label");
+ pedwarn (loc, 0, "attribute % not preceding "
+"a case label or default label");
}
break;
default:
@@ -2428,8 +2428,8 @@ expand_FALLTHROUGH (gimple_seq *seq_p)
if (wi.callback_result == integer_zero_node)
/* We've found [[fallthrough]]; at the end of a switch, which the C++
standard says is ill-formed; see [dcl.attr.fallthrough]. */
-warning_at (loc, 0, "attribute % not preceding "
- "a case label or default label");
+pedwarn (loc, 0, "attribute % not preceding "
+"a case label or default label");
}
Index: gcc/testsuite/gcc.dg/c2x-attr-fallthrough-1.c
===
--- gcc/testsuite/gcc.dg/c2x-attr-fallthrough-1.c (revision 278510)
+++ gcc/testsuite/gcc.dg/c2x-attr-fallthrough-1.c (working copy)
@@ -3,7 +3,7 @@
/* { dg-options "-std=c2x -pedantic-errors -Wextra" } */
int
-f (int a)
+f (int a, int c)
{
int b = 2;
switch (a)
@@ -22,6 +22,21 @@ int
case 5:
b += 1;
break;
+case 6:
+ if (c == 2)
+ {
+ [[fallthrough]];
+ }
+ else
+ {
+ [[fallthrough]];
+ }
+case 7:
+ b += 3;
+ [[fallthrough]];
+default:
+ b += 8;
+ break;
}
return b;
}
Index: gcc/testsuite/gcc.dg/c2x-attr-fallthrough-2.c
===
--- gcc/testsuite/gcc.dg/c2x-attr-fallthrough-2.c (revision 278510)
+++ gcc/testsuite/gcc.dg/c2x-attr-fallthrough-2.c (working copy)
@@ -15,7 +15,8 @@ int z = sizeof (int [[fallthrough]]); /* { dg-erro
int
f (int a)
{
- [[fallthrough]] int b = 2; /* { dg-warning "not followed by|ignored" } */
+ [[fallthrough]] int b = 2; /* { dg-warning "not followed by" } */
+ /* { dg-error "ignored" "ignored" { target *-*-* } .-1 } */
switch (a)
{
case 1:
Index: gcc/testsuite/gcc.dg/c2x-attr-fallthrough-3.c
===
--- gcc/testsuite/gcc.dg/c2x-attr-fallthrough-3.c (revision 278510)
+++ gcc/testsuite/gcc.dg/c2x-attr-fallthrough-3.c (working copy)
@@ -1,5 +1,5 @@
/* Test C2x attribute syntax. Invalid use of fallthrough attribute
- outside switch or in bad context inside switch. */
+ outside switch. */
/* { dg-do compile } */
/* { dg-options "-std=c2x
[committed] Add test for c++/92443
This started crashing with the <=> implementation but has now been
fixed by r278465.
Tested on x86_64-linux, applying to trunk.
2019-11-20 Marek Polacek
PR c++/92443
* g++.dg/cpp0x/constexpr-92443.C: New test.
diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-92443.C
gcc/testsuite/g++.dg/cpp0x/constexpr-92443.C
new file mode 100644
index 000..8d03e3ed871
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-92443.C
@@ -0,0 +1,12 @@
+// PR c++/92443
+// { dg-do compile { target c++11 } }
+
+struct a {
+ constexpr a(long) : b() {}
+ operator long() const;
+ operator bool();
+ constexpr friend a operator|(a, a c) { return c; }
+ long b;
+};
+using d = a;
+constexpr d e = 6, f = f | e;
Re: [PATCH] Fix slowness in demangler
On Tue, Nov 19, 2019 at 08:00:22PM +0100, Tim Rühsen wrote: > Thanks. Where exactly did it go ? Still can't see it in > git://sourceware.org/git/binutils-gdb.git. Is there another repository ? It will appear in the binutils repo when someone syncs libiberty with the master sources in the gcc repo. -- Alan Modra Australia Development Lab, IBM
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
Am 20.11.19 um 23:18 schrieb Janne Blomqvist: On Wed, Nov 20, 2019 at 11:35 PM Thomas König wrote: Am 20.11.19 um 21:45 schrieb Janne Blomqvist: BTW, since this is done for the purpose of optimization, have you done testing on some suitable benchmark suite such as polyhedron, whether it a) generates any different code b) does it make it go faster? I haven't run any actual benchmarks. However, there is a simple example which shows its advantages. Consider subroutine foo(n,m) m = 0 do 100 i=1,100 call bar m = m + n 100 continue end (I used old-style DO loops just because :-) Without the optimization, the inner loop is translated to .L2: xorl%eax, %eax callbar_ movl(%r12), %eax addl%eax, 0(%rbp) subl$1, %ebx jne .L2 and with the optimization to .L2: xorl%eax, %eax callbar_ addl%r12d, 0(%rbp) subl$1, %ebx jne .L2 so the load of the address is missing. (Why do we zero %eax before each call? It should not be a variadic call right?) Not sure. Maybe some belt and suspenders thing? I guess someone better versed in ABI minutiae knows better. It's not Fortran-specific though, the C frontend does the same when calling a void function. OK, so considering your other e-mail, this is a separate issue that we can fix another time. AFAIK on reasonably current OoO CPU's xor'ing a register with itself is handled by the renamer and doesn't consume an execute slot, so it's in effect a zero-cycle instruction. Still bloats the code slightly, though. Of course, Fortran language rules specify that the call to bar cannot do anything to n Hmm, does it? What about the following modification to your testcase: module nmod integer :: n end module nmod subroutine foo(n,m) m = 0 do 100 i=1,100 call bar m = m + n 100 continue end subroutine foo subroutine bar() use nmod n = 0 end subroutine bar program main use nmod implicit none integer :: m n = 1 m = 0 call foo(n, m) print *, m end program main That is not allowed: # 15.5.2.13 Restrictions on entities associated with dummy arguments [...] # (3) Action that affects the value of the entity or any subobject of it # shall be taken only through the dummy argument unless [none of the restrictions apply]. So, a copy in / copy out for variables where we can not be sure that no value is assigned? Does anybody see a downside for that?) In principle sounds good, unless my concerns above are real and affect this case too. So, how to proceed? Commit the patch with the maximum length for a mangled symbol, and then maybe try for the copy-out variant in a follow-up patch? I agree with Tobias that dealing with this in the middle end is probably the right thing to do in the long run (especially since we could also handle arrays and structs this way). Until we get around to doing this (gcc 11 at earliest), we could still profit somewhat from this optimization in the meantime. Regards Thomas
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On 11/20/19 11:18 PM, Janne Blomqvist wrote: Of course, Fortran language rules specify that the call to bar cannot do anything to n Hmm, does it? What about the following modification to your testcase: This code violates (quote from F2018): "15.5.2.13 Restrictions on entities associated with dummy arguments" "While an entity is associated with a dummy argument, the following restrictions hold." "[…] (3) Action that affects the value of the entity or any subobject of it shall be taken only through the dummy argument unless" "(a) the dummy argument has the POINTER attribute, […]" (Some more related to TARGET attribute and to coarrays, which also do not apply here.) Not listed there, but the asynchronous attribute (Section 8.5.4) would be also a way to disable the optimization we are talking about. Cheers, Tobias module nmod integer :: n end module nmod subroutine foo(n,m) m = 0 do 100 i=1,100 call bar m = m + n 100 continue end subroutine foo subroutine bar() use nmod n = 0 end subroutine bar program main use nmod implicit none integer :: m n = 1 m = 0 call foo(n, m) print *, m end program main So, a copy in / copy out for variables where we can not be sure that no value is assigned? Does anybody see a downside for that?) In principle sounds good, unless my concerns above are real and affect this case too. Is there a risk of performance regressions due to higher register pressure? I don't think so. Either the compiler realizes that it can keep the variable in a register (then it makes no difference), or it has to load it fresh from its address (then there is one additional register needed). Yes, true. Good point.
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On Thu, Nov 21, 2019 at 12:18 AM Janne Blomqvist
wrote:
>
> On Wed, Nov 20, 2019 at 11:35 PM Thomas König wrote:
> > (Why do we zero %eax
> > before each call? It should not be a variadic call right?)
>
> Not sure. Maybe some belt and suspenders thing? I guess someone better
> versed in ABI minutiae knows better. It's not Fortran-specific though,
> the C frontend does the same when calling a void function.
Ah, scratch that, it is some varargs-thing, I had forgot that a C
function with no arguments or lacking a prototype is considered a
varargs. The code
void foo();
void bar(void);
void testfoo()
{
foo();
}
void testbar()
{
bar();
}
void testunprototyped()
{
baz();
}
generates code (elided scaffolding):
testfoo:
xorl %eax, %eax
jmp foo
testbar:
jmp bar
testunprototyped:
xorl %eax, %eax
jmp baz
So probably this is due to the Fortran procedures lacking an interface
being considered varargs by the caller. Starts to smell like some
leftover from PR 87689?
--
Janne Blomqvist
Re: [PATCH] Switch gcc ftp URL's to http
On Wed, 20 Nov 2019, Janne Blomqvist wrote: > On Wed, Nov 20, 2019 at 7:53 PM Joseph Myers wrote: > > > > This patch is OK with http changed to https. (That is, with it changed > > where the patch is already changing the URL. While changing http to https > > makes sense more generally in the documentation whenever a site supports > > https, that's probably best not mixed with the move from ftp.) > > Thanks for the review, done, and committed as r278526. > > Any opinions on whether I should apply this to the 8 and 9 branches as > well? And the same question for my previous patch updating > contrib/download_prerequisites at > https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00957.html . I have no advice on that question. -- Joseph S. Myers jos...@codesourcery.com
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On 11/20/19 10:35 PM, Thomas König wrote: I haven't run any actual benchmarks. I think it would be interesting – I think there can be quite some advantages in some cases, while in most cases, it is not really noticable. Of course, Fortran language rules specify that the call to bar cannot do anything to n, but apparently we do not tell the gcc middle end that this is the case, or maybe that there is no way to really specify this. To my knowledge, the ME does not support the Fortran semantics but assumes that once a pointer escapes in one procedure call, any other procedure might have access to it and modify it as well. This matches (effectively) the Fortran semantics of asynchronous. See also PR 49733. The only thing we can do currently is to specify the intent via 'fn spec' which helps a bit – but not if the pointer has already escaped. Maybe we should think again about handling this properly in the ME. Cheers, Tobias
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On Wed, Nov 20, 2019 at 11:35 PM Thomas König wrote: > > Am 20.11.19 um 21:45 schrieb Janne Blomqvist: > > BTW, since this is done for the purpose of optimization, have you done > > testing on some suitable benchmark suite such as polyhedron, whether > > it a) generates any different code b) does it make it go faster? > > I haven't run any actual benchmarks. > > However, there is a simple example which shows its advantages. > Consider > >subroutine foo(n,m) >m = 0 >do 100 i=1,100 > call bar > m = m + n > 100 continue >end > > (I used old-style DO loops just because :-) > > Without the optimization, the inner loop is translated to > > .L2: > xorl%eax, %eax > callbar_ > movl(%r12), %eax > addl%eax, 0(%rbp) > subl$1, %ebx > jne .L2 > > and with the optimization to > > .L2: > xorl%eax, %eax > callbar_ > addl%r12d, 0(%rbp) > subl$1, %ebx > jne .L2 > > so the load of the address is missing. (Why do we zero %eax > before each call? It should not be a variadic call right?) Not sure. Maybe some belt and suspenders thing? I guess someone better versed in ABI minutiae knows better. It's not Fortran-specific though, the C frontend does the same when calling a void function. AFAIK on reasonably current OoO CPU's xor'ing a register with itself is handled by the renamer and doesn't consume an execute slot, so it's in effect a zero-cycle instruction. Still bloats the code slightly, though. > Of course, Fortran language rules specify that the call to bar > cannot do anything to n Hmm, does it? What about the following modification to your testcase: module nmod integer :: n end module nmod subroutine foo(n,m) m = 0 do 100 i=1,100 call bar m = m + n 100 continue end subroutine foo subroutine bar() use nmod n = 0 end subroutine bar program main use nmod implicit none integer :: m n = 1 m = 0 call foo(n, m) print *, m end program main > So, a copy in / copy out for variables where we can not be sure that > no value is assigned? Does anybody see a downside for that?) In principle sounds good, unless my concerns above are real and affect this case too. > > Is there a risk of performance regressions due to higher register pressure? > > I don't think so. Either the compiler realizes that it can > keep the variable in a register (then it makes no difference), > or it has to load it fresh from its address (then there is > one additional register needed). Yes, true. Good point. -- Janne Blomqvist
Re: [PATCH 0/4] Eliminate cc0 from m68k
On 20/11/2019 20:48, Bernd Schmidt wrote: > On 11/20/19 8:27 PM, Mikael Pettersson wrote: >> On Wed, Nov 20, 2019 at 3:16 PM Bernd Schmidt wrote: >>> Probably best to just run tests on stage1 and hope something shows up. >> >> Ok, how do I did that? I've always just done 'make -k check' after >> full bootstraps. >> I assume the stage 1 artifacts are the ones in the prev-* directories. > > There's a --disable-bootstrap configure option. > >>> What distro are you using for native builds? The m68k debian I'm using >>> does not have an installable gcc package. >> >> I run a bespoke distro on m68k and sparc64, derived from Fedora but >> massively cut down in size, with target patches done by myself or >> ported from Debian and Gentoo as necessary. >> >> Debian/m68k not having a gcc package? That sounds odd; I see e.g. a >> gcc-9 in http://ftp.ports.debian.org/debian-ports/pool-m68k/main/g/ > > Turns out I wasn't sufficiently familiar with how debian works. I was > missing an "apt update" step. I kind of assumed a package like gcc would > install out of the box (others did, things like emacs). > > > Bernd > If you're building gcc on debian/ubuntu you probably also want apt build-dep gcc to install all the packages needed for building the compiler. R.
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
Am 20.11.19 um 21:45 schrieb Janne Blomqvist: BTW, since this is done for the purpose of optimization, have you done testing on some suitable benchmark suite such as polyhedron, whether it a) generates any different code b) does it make it go faster? I haven't run any actual benchmarks. However, there is a simple example which shows its advantages. Consider subroutine foo(n,m) m = 0 do 100 i=1,100 call bar m = m + n 100 continue end (I used old-style DO loops just because :-) Without the optimization, the inner loop is translated to .L2: xorl%eax, %eax callbar_ movl(%r12), %eax addl%eax, 0(%rbp) subl$1, %ebx jne .L2 and with the optimization to .L2: xorl%eax, %eax callbar_ addl%r12d, 0(%rbp) subl$1, %ebx jne .L2 so the load of the address is missing. (Why do we zero %eax before each call? It should not be a variadic call right?) Of course, Fortran language rules specify that the call to bar cannot do anything to n, but apparently we do not tell the gcc middle end that this is the case, or maybe that there is no way to really specify this. (Actually, I just tried out subroutine foo(n,m) integer :: dummy_n, dummy_m dummy_n = n dummy_m = 0 do 100 i=1,100 call bar dummy_m = dummy_m + dummy_n 100 continue m = dummy_m end This is optimized even further: .L2: xorl%eax, %eax callbar_ subl$1, %ebx jne .L2 imull $100, %r12d, %r12d So, a copy in / copy out for variables where we can not be sure that no value is assigned? Does anybody see a downside for that?) Is there a risk of performance regressions due to higher register pressure? I don't think so. Either the compiler realizes that it can keep the variable in a register (then it makes no difference), or it has to load it fresh from its address (then there is one additional register needed). Regards Thomas
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On Wed, 20 Nov 2019 22:38:30 +0200
Janne Blomqvist wrote:
> On Wed, Nov 20, 2019 at 8:00 PM Bernhard Reutner-Fischer
> wrote:
> >
> > On 19 November 2019 23:54:55 CET, Thomas Koenig
> > wrote:
> > >Am 19.11.19 um 11:39 schrieb Bernhard Reutner-Fischer:
> > >> + char name[GFC_MAX_SYMBOL_LEN + 1];
> > >> + snprintf (name, GFC_MAX_SYMBOL_LEN, "__dummy_%d_%s", var_num++,
> > >> +f->sym->name);
> > >> +
> > >> + if (gfc_get_sym_tree (name, ns, , false) != 0)
> > >>
> > >> (I) you should + sizeof(__dummy__) + 10 for unsigned long %d or the
> > >like.
> > >
> > >GFC_MAX_SYMBOL_LEN is the maximum length of a gfortran symbol. AFAIK,
> >
> > This is only true for user-provided names in the source code.
> >
> > Think e.g. class names as can be seen in the dumps..
>
> We have GFC_MAX_MANGLED_SYMBOL_LEN for that. *Insert my standard pet
> peeve rant that we should use heap allocated unlimited length strings
> for these rather than copying stack allocated strings around, or
> preferable a symbol table*
yea, which i started to lay grounds to address that in
https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/aldot/fortran-fe-stringpool
about a year ago ;) Reminds me: i had to change the symbol names that
are persisted in module-files to make it work; Still not sure if that's
acceptable so if somebody would be willing to lend me a hand for
sanity-checking that aspect of the series i'd be glad. Would certainly
help to trick me into continuing the thing now, during winter.
Looks like i've another memory leak plug lying around on that tree that
i didn't try to push yet; It's the hunk in gfc_release_symbol() in the
attached brain-dump i think, don't remember and should revisit to
have it fixed for good i suppose..
>
> > >it
> > >is not possible to use a longer symbol name than that, so it needs to
> > >be
> > >truncated. Uniqueness of the variable name is guaranteed by the var_num
> > >variable.
> > >
> > >If the user puts dummy arguments Supercalifragilisticexpialidociousa
> > >and
> > >Supercalifragilisticexpialidociousb into the argument list of a
> > >procedure, he will have to look at the numbers to differentiate them
> > >:-)
> > >
> > >> (II) s/__dummy/__intent_in/ for clarity?
> > >
> > >It's moved away a bit from INTENT(IN) now, because an argument which
> > >cannot be modified (even by passing to a procedure with a dummy
> > >argument
> > >with unknown intent) is now also handled.
> >
> > So maybe __readonly_ , __rdonly_, __rd_or the like? dummy is really
> > misleading a name in the dumps..
>
> Well, dummy is a term with a precise definition in the Fortran
> standard, so in a way it's good so one realizes it has something to do
> with a dummy argument. But yes, it's a bit misleading because it's not
> the dummy argument itself but rather a dereferenced copy of it. I
> suggest __readonly_dereferenced_dummy_copy_yes_this_is_a_really_long_name_.
> :)
:) __rodummy_ then?
but bikeshedding either way, so, Thomas, please go for __dummy_ short of
sensible alternatives.
cheers,
diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c
index e0bb381a55f..30b2a517246 100644
--- a/gcc/fortran/class.c
+++ b/gcc/fortran/class.c
@@ -680,6 +680,7 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
c->ts.type = BT_DERIVED;
c->attr.access = ACCESS_PRIVATE;
c->ts.u.derived = ts->u.derived;
+ c->attr.artificial = 1;
c->attr.class_pointer = attr->pointer;
c->attr.pointer = attr->pointer || (attr->dummy && !attr->allocatable)
|| attr->select_type_temporary;
@@ -687,7 +688,7 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
c->attr.dimension = attr->dimension;
c->attr.codimension = attr->codimension;
c->attr.abstract = fclass->attr.abstract;
- c->as = (*as);
+ c->as = *as;
c->initializer = NULL;
/* Add component '_vptr'. */
@@ -696,6 +697,7 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr,
c->ts.type = BT_DERIVED;
c->attr.access = ACCESS_PRIVATE;
c->attr.pointer = 1;
+ c->attr.artificial = 1;
if (ts->u.derived->attr.unlimited_polymorphic)
{
@@ -2296,6 +2298,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
goto cleanup;
vtype->attr.access = ACCESS_PUBLIC;
vtype->attr.vtype = 1;
+ vtype->attr.artificial = 1;
gfc_set_sym_referenced (vtype);
/* Add component '_hash'. */
@@ -2304,6 +2307,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
c->ts.type = BT_INTEGER;
c->ts.kind = 4;
c->attr.access = ACCESS_PRIVATE;
+ c->attr.artificial = 1;
c->initializer = gfc_get_int_expr (gfc_default_integer_kind,
NULL, derived->hash_value);
@@ -2313,6 +2317,7 @@ gfc_find_derived_vtab (gfc_symbol *derived)
c->ts.type = BT_INTEGER;
c->ts.kind = gfc_size_kind;
c->attr.access = ACCESS_PRIVATE;
+
Re: [PATCH 2/7] Include new generated gcc/params.opt file.
On Wed, 2019-11-20 at 16:07 +0100, Martin Liška wrote: > On 11/20/19 4:02 PM, David Malcolm wrote: > > On Wed, 2019-11-20 at 15:51 +0100, Martin Liška wrote: > > > On 11/20/19 3:49 PM, David Malcolm wrote: > > > > On Wed, 2019-11-06 at 11:30 +0100, Martin Liska wrote: > > > > > gcc/ChangeLog: > > > > > > > > > > 2019-11-06 Martin Liska > > > > > > > > > > * Makefile.in: Include params.opt. > > > > > * flag-types.h (enum parloops_schedule_type): Add > > > > > parloops_schedule_type used in params.opt. > > > > > * params.opt: New file. > > > > > --- > > > > >gcc/Makefile.in | 2 +- > > > > >gcc/flag-types.h | 11 + > > > > >gcc/params.opt | 967 > > > > > +++ > > > > >3 files changed, 979 insertions(+), 1 deletion(-) > > > > >create mode 100644 gcc/params.opt > > > > > > > > Is your params.def -> params.opt script available > > > > somewhere? (sorry if > > > > I missed it, I didn't see it looking over the patch kit) > > > > > > Hi David. > > > > > > > (I'm rebasing my static analyzer patch kit and am about to > > > > convert > > > > my > > > > params.def additions to be params.opt additions, though I'm > > > > only > > > > adding > > > > 4 params, so hopefully doing it by hand will be trivial) > > > > > > No, it's not public. Please convert it by hand, it will be faster > > > ;) > > > > Fair enough. > > > > Does this new machinery mean we can have per-frontend params (by > > putting the Param options in the pertinent .opt file) ? (not sure > > what > > that will do to LTO) > > I guess so. Note that now parameters are first class citizens same as > options. > So that, having a per-FE should work fine. Thanks. FWIW, given my branch adds support for in-tree plugins (which can add options), I've added them to the plugin.opt file: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02030.html Dave
[PATCH 11/11] [analyzer] Updates to internal documentation
gcc/ChangeLog:
* doc/analyzer.texi (Analyzer Paths): Add path pruning and
precision-of-wording vfuncs.
(Debugging the Analyzer): Consolidate duplicated material.
---
gcc/doc/analyzer.texi | 47 +--
1 file changed, 37 insertions(+), 10 deletions(-)
diff --git a/gcc/doc/analyzer.texi b/gcc/doc/analyzer.texi
index adf375a..3f5999f 100644
--- a/gcc/doc/analyzer.texi
+++ b/gcc/doc/analyzer.texi
@@ -342,9 +342,39 @@ each duplicate warning's shortest path is feasible,
rejecting any
warnings for which the shortest path is infeasible (which could lead to
false negatives).
-We use the shortest feasible path through the exploded_graph (a list of
-edges) to build a @code{diagnostic_path} (a list of events for the
-diagnostic subsystem).
+We use the shortest feasible @code{exploded_path} through the
+@code{exploded_graph} (a list of @code{exploded_edge *}) to build a
+@code{diagnostic_path} (a list of events for the diagnostic subsystem) -
+specifically a @code{checker_path}.
+
+Having built the @code{checker_path}, we prune it to try to eliminate
+events that aren't relevant, to minimize how much the user has to read.
+
+After pruning, we notify each event in the path of its ID and record the
+IDs of interesting events, allowing for events to refer to other events
+in their descriptions. The @code{pending_diagnostic} class has various
+vfuncs to support emitting more precise descriptions, so that e.g.
+
+@itemize @bullet
+@item
+a deref-of-unchecked-malloc diagnostic might use:
+@smallexample
+ returning possibly-NULL pointer to 'make_obj' from 'allocator'
+@end smallexample
+for a @code{return_event} to make it clearer how the unchecked value moves
+from callee back to caller
+@item
+a double-free diagnostic might use:
+@smallexample
+ second 'free' here; first 'free' was at (3)
+@end smallexample
+and a use-after-free might use
+@smallexample
+ use after 'free' here; memory was freed at (2)
+@end smallexample
+@end itemize
+
+At this point we can emit the diagnostic.
@subsection Limitations
@@ -442,21 +472,18 @@ will dump just the number of nodes, and their IDs.
will also dump all of the states within those nodes.
-@code{region_model::get_value_by_name} can be used when inserting custom
-debugging code (e.g. adding it @code{region_model::validate} to detect the
-point at which a named variable acquires an unexpected state).
-
@subsection Other Debugging Techniques
One approach when tracking down where a particular bogus state is
introduced into the @code{exploded_graph} is to add custom code to
@code{region_model::validate}.
-For example, this custom code breaks with an assertion failure when a
-variable called @code{ptr} acquires a value that's unknown:
+For example, this custom code (added to @code{region_model::validate})
+breaks with an assertion failure when a variable called @code{ptr}
+acquires a value that's unknown, using
+@code{region_model::get_value_by_name} to locate the variable
@smallexample
-
/* Find a variable matching "ptr". */
svalue_id sid = get_value_by_name ("ptr");
if (!sid.null_p ())
--
1.8.5.3
[PATCH 10/11] [analyzer] Fix issues in diagnostic_manager::prune_path
There was a bad upper limit in diagnostic_manager::prune_path's test for
pruning redundant interprocedural pass information, leading to an ICE.
Additional, the test for pruning
[..., call, function-entry, return, ...]
triples doesn't work for -fanalyzer-verbosity=0, which doesn't generate
function-entry events, leading to cases where -fanalyzer-verbosity=0 could
be more verbose than higher limits, e.g.:
'test_3': events 1-4
|
| NN | free (ptr);
| | ^~
| | |
| | (1) first 'free' here
| NN | called_by_test_3 ();
| | ~~~
| | |
| | (2) calling 'called_by_test_3' from 'test_3'
| | (3) returning to 'test_3' from 'called_by_test_3'
| NN | free (ptr); /* { dg-warning "double-'free' of 'ptr'" } */
| | ~~
| | |
| | (4) second 'free' here; first 'free' was at (1)
|
where the call/return would be pruned at higher verbosities.
This patch fixes the limits, and prunes the case of a [call, return] pair.
gcc/ChangeLog:
* analyzer/diagnostic-manager.cc (diagnostic_manager::prune_path):
When pruning [..., call, function-entry, return, ...] triples,
fix the bounds test and move it inside the loop. Also test for
[..., call, return, ...] pairs.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/analyzer-verbosity-0.c: Add coverage for pruning
a call/return to an empty function.
* gcc.dg/analyzer/analyzer-verbosity-1.c: Likewise.
* gcc.dg/analyzer/analyzer-verbosity-2.c: Likewise.
---
gcc/analyzer/diagnostic-manager.cc | 32 --
.../gcc.dg/analyzer/analyzer-verbosity-0.c | 29
.../gcc.dg/analyzer/analyzer-verbosity-1.c | 30
.../gcc.dg/analyzer/analyzer-verbosity-2.c | 30
4 files changed, 118 insertions(+), 3 deletions(-)
diff --git a/gcc/analyzer/diagnostic-manager.cc
b/gcc/analyzer/diagnostic-manager.cc
index 926900b..8cd4507 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -1076,10 +1076,12 @@ diagnostic_manager::prune_path (checker_path *path,
do
{
changed = false;
- int idx = path->m_events.length () - 3;
- while (idx >= 0 && idx < (signed)path->m_events.length ())
+ int idx = path->m_events.length () - 1;
+ while (idx >= 0)
{
- if (path->m_events[idx]->is_call_p ()
+ /* Prune [..., call, function-entry, return, ...] triples. */
+ if (idx + 2 < (signed)path->m_events.length ()
+ && path->m_events[idx]->is_call_p ()
&& path->m_events[idx + 1]->is_function_entry_p ()
&& path->m_events[idx + 2]->is_return_p ())
{
@@ -1095,7 +1097,31 @@ diagnostic_manager::prune_path (checker_path *path,
path->delete_event (idx + 1);
path->delete_event (idx);
changed = true;
+ idx--;
+ continue;
}
+
+ /* Prune [..., call, return, ...] pairs
+(for -fanalyzer-verbosity=0). */
+ if (idx + 1 < (signed)path->m_events.length ()
+ && path->m_events[idx]->is_call_p ()
+ && path->m_events[idx + 1]->is_return_p ())
+ {
+ if (get_logger ())
+ {
+ label_text desc (path->m_events[idx]->get_desc (false));
+ log ("filtering events %i-%i:"
+ " irrelevant call/return: %s",
+ idx, idx + 1, desc.m_buffer);
+ desc.maybe_free ();
+ }
+ path->delete_event (idx + 1);
+ path->delete_event (idx);
+ changed = true;
+ idx--;
+ continue;
+ }
+
idx--;
}
diff --git a/gcc/testsuite/gcc.dg/analyzer/analyzer-verbosity-0.c
b/gcc/testsuite/gcc.dg/analyzer/analyzer-verbosity-0.c
index 98200b3..1103cc6 100644
--- a/gcc/testsuite/gcc.dg/analyzer/analyzer-verbosity-0.c
+++ b/gcc/testsuite/gcc.dg/analyzer/analyzer-verbosity-0.c
@@ -131,3 +131,32 @@ void test_2 (void *ptr, int a, int b)
// TODO: range cases
+/* The call/return to this function shouldn't appear in the path. */
+
+void called_by_test_3 (void)
+{
+}
+
+void test_3 (void *ptr)
+{
+ free (ptr);
+ called_by_test_3 ();
+ free (ptr); /* { dg-warning "double-'free' of 'ptr'" } */
+}
+
+/* { dg-begin-multiline-output "" }
+ NN | free (ptr);
+ | ^~
+ 'test_3': events 1-2
+|
+| NN | free (ptr);
+| | ^~
+| | |
+| | (1) first 'free' here
+| NN | called_by_test_3 ();
+| NN | free (ptr);
+| | ~~
+| | |
+| | (2) second 'free' here; first 'free' was at (1)
+|
+ { dg-end-multiline-output "" } */
diff --git
[PATCH 08/11] [analyzer] Show rewind destination for leaks due to longjmp
This patch adds some special-case logic to how paths are generated
so that leaks due to longjmp rewinding past a frame now show where the
longjmp rewinds to (longjmp and setjmp are already something of a
special-case so this seems reasonable).
A colorized LTO example of the output can be seen at:
https://dmalcolm.fedorapeople.org/gcc/2019-11-18/lto-longjmp-leak-demo.html
gcc/ChangeLog:
* analyzer/diagnostic-manager.cc
(saved_diagnostic::saved_diagnostic): Initialize new field
m_trailing_eedge.
(diagnostic_manager::emit_saved_diagnostic): If m_trailing_eedge
is set, use it to add extra events after the "final" event.
* analyzer/diagnostic-manager.h (saved_diagnostic::operator==):
Compare m_trailing_eedge fields.
(saved_diagnostic::m_trailing_eedge): New field.
(diagnostic_manager::get_num_diagnostics): New accessor.
(diagnostic_manager::get_saved_diagnostic): New accessor.
* analyzer/engine.cc (exploded_node::on_longjmp): Set
m_trailing_edge on any diagnostics saved when handling
region_model::on_longjmp.
(exploded_graph::add_edge): Return the newly-created eedge.
* analyzer/exploded-graph.h (exploded_graph::add_edge): Convert
return type from void to exploded_edge *.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/setjmp-7a.c: Update expected multiline output to
include a final pair of events showing the longjmp and the setjmp
that it rewinds to.
---
gcc/analyzer/diagnostic-manager.cc| 9 -
gcc/analyzer/diagnostic-manager.h | 13 ++-
gcc/analyzer/engine.cc| 56 ---
gcc/analyzer/exploded-graph.h | 8 ++---
gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c | 13 +--
5 files changed, 86 insertions(+), 13 deletions(-)
diff --git a/gcc/analyzer/diagnostic-manager.cc
b/gcc/analyzer/diagnostic-manager.cc
index fae38c7..926900b 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -47,7 +47,7 @@ saved_diagnostic::saved_diagnostic (const state_machine *sm,
outlive that. */
m_stmt_finder (stmt_finder ? stmt_finder->clone () : NULL),
m_var (var), m_state (state),
- m_d (d)
+ m_d (d), m_trailing_eedge (NULL)
{
gcc_assert (m_stmt || m_stmt_finder);
@@ -427,6 +427,13 @@ diagnostic_manager::emit_saved_diagnostic (const
exploded_graph ,
emission_path.add_final_event (sd.m_sm, epath.get_final_enode (), stmt,
sd.m_var, sd.m_state);
+ /* The "final" event might not be final; if the saved_diagnostic has a
+ trailing eedge stashed, add any events for it. This is for use
+ in handling longjmp, to show where a longjmp is rewinding to. */
+ if (sd.m_trailing_eedge)
+add_events_for_eedge (*sd.m_trailing_eedge, eg.get_ext_state (),
+ _path);
+
emission_path.prepare_for_emission (sd.m_d);
gcc_rich_location rich_loc (stmt->location);
diff --git a/gcc/analyzer/diagnostic-manager.h
b/gcc/analyzer/diagnostic-manager.h
index 0fc..8591d2e 100644
--- a/gcc/analyzer/diagnostic-manager.h
+++ b/gcc/analyzer/diagnostic-manager.h
@@ -45,7 +45,8 @@ public:
/* We don't compare m_stmt_finder. */
&& m_var == other.m_var
&& m_state == other.m_state
- && m_d->equal_p (*other.m_d));
+ && m_d->equal_p (*other.m_d)
+ && m_trailing_eedge == other.m_trailing_eedge);
}
//private:
@@ -57,6 +58,7 @@ public:
tree m_var;
state_machine::state_t m_state;
pending_diagnostic *m_d;
+ exploded_edge *m_trailing_eedge;
};
/* A class with responsibility for saving pending diagnostics, so that
@@ -93,6 +95,15 @@ public:
const gimple *stmt,
int num_dupes);
+ unsigned get_num_diagnostics () const
+ {
+return m_saved_diagnostics.length ();
+ }
+ saved_diagnostic *get_saved_diagnostic (unsigned idx)
+ {
+return m_saved_diagnostics[idx];
+ }
+
private:
void build_emission_path (const exploded_graph ,
const exploded_path ,
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 9936e1e..aa4a359 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -1083,6 +1083,13 @@ exploded_node::on_longjmp (exploded_graph ,
>= setjmp_point.get_stack_depth ());
/* Update the state for use by the destination node. */
+
+ /* Stash the current number of diagnostics so that we can update
+ any that this adds to show where the longjmp is rewinding to. */
+
+ diagnostic_manager *dm = _diagnostic_manager ();
+ unsigned prev_num_diagnostics = dm->get_num_diagnostics ();
+
new_region_model->on_longjmp (longjmp_call, setjmp_call,
setjmp_point.get_stack_depth (), ctxt);
@@ -1095,9 +1102,46 @@ exploded_node::on_longjmp (exploded_graph ,
[PATCH 09/11] [analyzer] Add checker_path::debug
This patch adds a new debugging function.
gcc/ChangeLog:
* analyzer/checker-path.cc (checker_path::debug): New member
function.
* analyzer/checker-path.h (checker_path::debug): New decl.
---
gcc/analyzer/checker-path.cc | 19 +++
gcc/analyzer/checker-path.h | 2 +-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/gcc/analyzer/checker-path.cc b/gcc/analyzer/checker-path.cc
index 5302504..0a18406 100644
--- a/gcc/analyzer/checker-path.cc
+++ b/gcc/analyzer/checker-path.cc
@@ -883,6 +883,25 @@ checker_path::maybe_log (logger *logger, const char *desc)
const
}
}
+/* Print a multiline form of this path to STDERR. */
+
+DEBUG_FUNCTION void
+checker_path::debug () const
+{
+ checker_event *e;
+ int i;
+ FOR_EACH_VEC_ELT (m_events, i, e)
+{
+ label_text event_desc (e->get_desc (false));
+ fprintf (stderr,
+ "[%i]: %s \"%s\"\n",
+ i,
+ event_kind_to_string (m_events[i]->m_kind),
+ event_desc.m_buffer);
+ event_desc.maybe_free ();
+}
+}
+
/* Add a warning_event to the end of this path. */
void
diff --git a/gcc/analyzer/checker-path.h b/gcc/analyzer/checker-path.h
index f042f47..916e5a7 100644
--- a/gcc/analyzer/checker-path.h
+++ b/gcc/analyzer/checker-path.h
@@ -499,8 +499,8 @@ public:
return *m_events[idx];
}
-
void dump (pretty_printer *pp) const;
+ void debug () const;
void maybe_log (logger *logger, const char *desc) const;
--
1.8.5.3
[PATCH 06/11] [analyzer] More LTO test coverage
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/malloc-ipa-8-lto-a.c: New test.
* gcc.dg/analyzer/malloc-ipa-8-lto-b.c: New test.
* gcc.dg/analyzer/malloc-ipa-8-lto-c.c: New test.
* gcc.dg/analyzer/malloc-ipa-8-lto.h: New test.
---
gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-a.c | 12
gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-b.c | 18 ++
gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c | 17 +
gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto.h | 12
4 files changed, 59 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-a.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-b.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto.h
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-a.c
b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-a.c
new file mode 100644
index 000..c34e4be
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-a.c
@@ -0,0 +1,12 @@
+#include
+#include "malloc-ipa-8-lto.h"
+
+void *wrapped_malloc (size_t size)
+{
+ return malloc (size);
+}
+
+void wrapped_free (void *ptr)
+{
+ free (ptr);
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-b.c
b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-b.c
new file mode 100644
index 000..c9d7df1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-b.c
@@ -0,0 +1,18 @@
+#include
+#include "malloc-ipa-8-lto.h"
+
+boxed_int *
+make_boxed_int (int i)
+{
+ boxed_int *result = (boxed_int *)wrapped_malloc (sizeof (boxed_int));
+ if (!result)
+abort ();
+ result->i = i;
+ return result;
+}
+
+void
+free_boxed_int (boxed_int *bi)
+{
+ wrapped_free (bi);
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c
b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c
new file mode 100644
index 000..d332db1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c
@@ -0,0 +1,17 @@
+/* { dg-do link } */
+/* { dg-require-effective-target lto } */
+/* { dg-additional-options "-flto" } */
+/* { dg-additional-sources "malloc-ipa-8-lto-a.c malloc-ipa-8-lto-b.c" } */
+
+#include
+#include "malloc-ipa-8-lto.h"
+
+void test (int i)
+{
+ boxed_int *obj = make_boxed_int (i);
+
+ free_boxed_int (obj);
+ free (obj); /* { dg-warning "double-free" } */
+}
+
+int main() { return 0; }
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto.h
b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto.h
new file mode 100644
index 000..f24eefc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto.h
@@ -0,0 +1,12 @@
+#include
+
+extern void *wrapped_malloc (size_t size);
+extern void wrapped_free (void *ptr);
+
+typedef struct boxed_int
+{
+ int i;
+} boxed_int;
+
+extern boxed_int *make_boxed_int (int i);
+extern void free_boxed_int (boxed_int *bi);
--
1.8.5.3
[PATCH 04/11] [analyzer] Add params to plugin.opt
Various commits on 2019-11-12 including r278083 through r278087 reimplemented parameter-handling in terms of options, so that params are defined in params.opt rather than params.def. This patch adds the params for the analyzer to plugin.opt, replacing the patch: [PATCH 22/49] analyzer: params.def: new parameters https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01520.html from the original version of the patch kit. gcc/ChangeLog: * analyzer/plugin.opt (-param=analyzer-bb-explosion-factor=): New param. (-param=analyzer-max-enodes-per-program-point=): New param. (-param=analyzer-max-recursion-depth=): New param. (-param=analyzer-min-snodes-for-call-summary=): New param. --- gcc/analyzer/plugin.opt | 16 1 file changed, 16 insertions(+) diff --git a/gcc/analyzer/plugin.opt b/gcc/analyzer/plugin.opt index 55f54bb..8408f1c 100644 --- a/gcc/analyzer/plugin.opt +++ b/gcc/analyzer/plugin.opt @@ -22,6 +22,22 @@ ; Please try to keep this file in ASCII collating order. +-param=analyzer-bb-explosion-factor= +Common Joined UInteger Var(param_analyzer_bb_explosion_factor) Init(5) Param +The maximum number of 'after supernode' exploded nodes within the analyzer per supernode, before terminating analysis. + +-param=analyzer-max-enodes-per-program-point= +Common Joined UInteger Var(param_analyzer_max_enodes_per_program_point) Init(8) Param +The maximum number of exploded nodes per program point within the analyzer, before terminating analysis of that point. + +-param=analyzer-max-recursion-depth= +Common Joined UInteger Var(param_analyzer_max_recursion_depth) Init(2) Param +The maximum number of times a callsite can appear in a call stack within the analyzer, before terminating analysis of a call tha would recurse deeper. + +-param=analyzer-min-snodes-for-call-summary= +Common Joined UInteger Var(param_analyzer_min_snodes_for_call_summary) Init(10) Param +The minimum number of supernodes within a function for the analyzer to consider summarizing its effects at call sites. + Wanalyzer-double-fclose Common Var(warn_analyzer_double_fclose) Init(1) Warning Warn about code paths in which a stdio FILE can be closed more than once. -- 1.8.5.3
[PATCH 07/11] [analyzer] Fix missing leak on longjmp past a free
This patch fixes the missing leak report in setjmp-7.c, which failed to report on an unchecked "malloc" result leaking due to a longjmp skipping past its frame to a setjmp in its caller. The root cause issue is that impl_region_model_context::on_state_leak would call the state machine's on_leak vfunc with a tree; this would call back into impl_region_model_context's warn_for_state vfunc, which would add a leak pending_diagnostic if the tree was in the correct sm-state. The latter check was redundant: we already know that the expression in an sm-state for which we ought to report a leak. The check was getting the wrong result: by passing around a tree (for the SSA_NAME for the result of "malloc" in the "middle" frame), the state-lookup was implicitly converting this back to a path_var, and looking for the SSA_NAME's state in the "inner" frame. I looked at updating warn_for_state to take a path_var rather than a tree, but this led to further cleanups being needed in on_transition. This patch takes the simpler approach of having on_leak only be called once a leak of a report-worthy state has been detected, and having it return a pending_diagnostic instance, eliminating the redundant state check. This simplification fixes the missing leak report (although the readability of the report could be improved; it's missing a description of where the longjmp is rewinding to. I'll leave that for a followup). gcc/ChangeLog: * analyzer/engine.cc (impl_region_model_context::on_state_leak): Rather than calling sm.on_leak and having it call back with warn_for_state, get pending_diagnostic from return value of on_leak, and add it directly, avoiding a redundant check of the state of the var. * analyzer/sm-file.cc (fileptr_state_machine::on_leak): Update for simpler API. * analyzer/sm-malloc.cc (malloc_state_machine::on_leak): Likewise. * analyzer/sm-pattern-test.cc (pattern_test_state_machine::on_leak): Delete. * analyzer/sm-sensitive.cc (sensitive_state_machine::on_leak): Delete. * analyzer/sm-taint.cc (taint_state_machine::on_leak): Delete. * analyzer/sm.h (state_machine::on_leak): Convert return type from "void" to "pending_diagnostic *". Drop all parameters except tree. Provide empty base class implementation. gcc/testsuite/ChangeLog: * gcc.dg/analyzer/setjmp-7.c: Remove xfails. * gcc.dg/analyzer/setjmp-7a.c: New test. --- gcc/analyzer/engine.cc| 8 ++- gcc/analyzer/sm-file.cc | 34 -- gcc/analyzer/sm-malloc.cc | 33 -- gcc/analyzer/sm-pattern-test.cc | 17 - gcc/analyzer/sm-sensitive.cc | 16 - gcc/analyzer/sm-taint.cc | 16 - gcc/analyzer/sm.h | 12 ++-- gcc/testsuite/gcc.dg/analyzer/setjmp-7.c | 4 +- gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c | 101 ++ 9 files changed, 137 insertions(+), 104 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index c4ca5bf..9936e1e 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -521,8 +521,12 @@ impl_region_model_context::on_state_leak (const state_machine , } } - sm.on_leak (_ctxt, m_enode_for_diag->get_supernode (), m_stmt, - leaked_tree, state); + pending_diagnostic *pd = sm.on_leak (leaked_tree); + if (pd) +m_eg->get_diagnostic_manager ().add_diagnostic + (, m_enode_for_diag, m_enode_for_diag->get_supernode (), + m_stmt, _finder, + leaked_tree, state, pd); } /* Implementation of region_model_context::on_inherited_svalue vfunc diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc index 4fc308c..f2e8030 100644 --- a/gcc/analyzer/sm-file.cc +++ b/gcc/analyzer/sm-file.cc @@ -52,12 +52,8 @@ public: enum tree_code op, tree rhs) const FINAL OVERRIDE; - void on_leak (sm_context *sm_ctxt, - const supernode *node, - const gimple *stmt, - tree var, - state_machine::state_t state) const FINAL OVERRIDE; bool can_purge_p (state_t s) const FINAL OVERRIDE; + pending_diagnostic *on_leak (tree var) const FINAL OVERRIDE; /* Start state. */ state_t m_start; @@ -299,24 +295,6 @@ fileptr_state_machine::on_condition (sm_context *sm_ctxt, } } -/* Implementation of state_machine::on_leak vfunc for - fileptr_state_machine. - Complain about leaks of FILE * in state 'unchecked' and 'nonnull'. */ - -void -fileptr_state_machine::on_leak (sm_context *sm_ctxt, - const supernode *node, - const gimple *stmt, - tree var, - state_machine::state_t state ATTRIBUTE_UNUSED) -
[PATCH 03/11] [analyzer] Fixup diagnostic_path for "json::number" to "json::integer_number"
r277284 eliminated json::number in favor of json::float_number and
json::integer_number. Update the diagnostic_path code accordingly.
gcc/ChangeLog:
* tree-diagnostic-path.cc (default_tree_make_json_for_path): Fix
overlong line. Use json::integer_number.
---
gcc/tree-diagnostic-path.cc | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/gcc/tree-diagnostic-path.cc b/gcc/tree-diagnostic-path.cc
index d044152..abb418d 100644
--- a/gcc/tree-diagnostic-path.cc
+++ b/gcc/tree-diagnostic-path.cc
@@ -484,7 +484,8 @@ default_tree_diagnostic_path_printer (diagnostic_context
*context,
doesn't have access to trees (for m_fndecl). */
json::value *
-default_tree_make_json_for_path (diagnostic_context *, const diagnostic_path
*path)
+default_tree_make_json_for_path (diagnostic_context *,
+const diagnostic_path *path)
{
json::array *path_array = new json::array ();
for (unsigned i = 0; i < path->num_events (); i++)
@@ -504,7 +505,8 @@ default_tree_make_json_for_path (diagnostic_context *,
const diagnostic_path *pa
= identifier_to_locale (lang_hooks.decl_printable_name (fndecl, 2));
event_obj->set ("function", new json::string (function));
}
- event_obj->set ("depth", new json::number (event.get_stack_depth ()));
+ event_obj->set ("depth",
+ new json::integer_number (event.get_stack_depth ()));
path_array->append (event_obj);
}
return path_array;
--
1.8.5.3
[PATCH 05/11] [analyzer] Avoid using "convert"
"convert" is not implemented in lto1, leading to an ICE when used.
This patch implements a build_cast function for the analyzer and uses it
to replace the call to "convert" when handling casts in region_model.
This fixes numerous ICEs when running the analyzer from LTO.
gcc/ChangeLog:
* analyzer/region-model.cc: Include "convert.h" and "target.h".
(build_cast): New function, adapted from jit-playback.c.
(region_model::maybe_cast_1): Use it to replace the call to
"convert".
* doc/analyzer.texi (Limitations): Remove the reference to convert
and ICEs for casts in LTO.
---
gcc/analyzer/region-model.cc | 55 +++-
gcc/doc/analyzer.texi| 7 +++---
2 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index f28661f..197f1be 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -29,6 +29,8 @@ along with GCC; see the file COPYING3. If not see
#include "cgraph.h"
#include "tree-dfa.h"
#include "stringpool.h"
+#include "convert.h"
+#include "target.h"
#include "selftest.h"
#include "diagnostic-color.h"
#include "diagnostic-metadata.h"
@@ -4694,6 +4696,56 @@ region_model::get_region_for_label (tree label)
return func_reg->get_or_create (this, func_rid, label, TREE_TYPE (label));
}
+/* Build a cast of SRC_EXPR to DST_TYPE, or return NULL_TREE.
+
+ Adapted from gcc::jit::playback::context::build_cast, which in turn is
+ adapted from
+ - c/c-typeck.c:build_c_cast
+ - c/c-convert.c: convert
+ - convert.h
+ Only some kinds of cast are currently supported here. */
+
+static tree
+build_cast (tree dst_type, tree src_expr)
+{
+ tree result = targetm.convert_to_type (dst_type, src_expr);
+ if (result)
+return result;
+ enum tree_code dst_code = TREE_CODE (dst_type);
+ switch (dst_code)
+{
+case INTEGER_TYPE:
+case ENUMERAL_TYPE:
+ result = convert_to_integer (dst_type, src_expr);
+ goto maybe_fold;
+
+case BOOLEAN_TYPE:
+ /* Compare with c_objc_common_truthvalue_conversion and
+c_common_truthvalue_conversion. */
+ /* For now, convert to: (src_expr != 0) */
+ result = build2 (NE_EXPR, dst_type,
+ src_expr,
+ build_int_cst (TREE_TYPE (src_expr), 0));
+ goto maybe_fold;
+
+case REAL_TYPE:
+ result = convert_to_real (dst_type, src_expr);
+ goto maybe_fold;
+
+case POINTER_TYPE:
+ result = build1 (NOP_EXPR, dst_type, src_expr);
+ goto maybe_fold;
+
+default:
+ return NULL_TREE;
+
+maybe_fold:
+ if (TREE_CODE (result) != C_MAYBE_CONST_EXPR)
+ result = fold (result);
+ return result;
+}
+}
+
/* If the type of SID's underlying value is DST_TYPE, return SID.
Otherwise, attempt to create (or reuse) an svalue representing an access
of SID as a DST_TYPE and return that value's svalue_id. */
@@ -4740,7 +4792,8 @@ region_model::maybe_cast_1 (tree dst_type, svalue_id sid)
/* Attempt to cast constants. */
if (tree src_cst = sval->maybe_get_constant ())
{
- tree dst = convert (dst_type, src_cst);
+ tree dst = build_cast (dst_type, src_cst);
+ gcc_assert (dst != NULL_TREE);
if (CONSTANT_CLASS_P (dst))
return get_or_create_constant_svalue (dst);
}
diff --git a/gcc/doc/analyzer.texi b/gcc/doc/analyzer.texi
index 8e7b26f..adf375a 100644
--- a/gcc/doc/analyzer.texi
+++ b/gcc/doc/analyzer.texi
@@ -382,11 +382,10 @@ The checkers are currently hardcoded and don't allow for
user extensibility
(e.g. adding allocate/release pairs).
@item
Although the analyzer's test suite has a proof-of-concept test case for
-LTO, it will crash on anything non-trivial. There are various
+LTO, LTO support hasn't had extensive testing. There are various
lang-specific things in the analyzer that assume C rather than LTO.
-For example, casts are currently implemented via @code{convert}, and this
-leads to an ICE on LTO. Similarly, SSA names are printed to the user in
-``raw'' form, rather than printing the underlying variable name.
+For example, SSA names are printed to the user in ``raw'' form, rather
+than printing the underlying variable name.
@end itemize
Some ideas for other checkers
--
1.8.5.3
[PATCH 02/11] [analyzer] Fixup metadata-handling for "json::number" to "json::integer_number"
r277284 eliminated json::number in favor of json::float_number and
json::integer_number. Update the diagnostic metadata code accordingly.
gcc/ChangeLog:
* diagnostic-format-json.cc (json_from_metadata): Use
json::integer_number.
---
gcc/diagnostic-format-json.cc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic-format-json.cc
index 24a08c0..f73d09b 100644
--- a/gcc/diagnostic-format-json.cc
+++ b/gcc/diagnostic-format-json.cc
@@ -112,7 +112,8 @@ json_from_metadata (const diagnostic_metadata *metadata)
json::object *metadata_obj = new json::object ();
if (metadata->get_cwe ())
-metadata_obj->set ("cwe", new json::number (metadata->get_cwe ()));
+metadata_obj->set ("cwe",
+ new json::integer_number (metadata->get_cwe ()));
return metadata_obj;
}
--
1.8.5.3
[PATCH 00/11] Static analysis v2
I've rebased my static analysis work (from r276961 to r278495) This patch kit contains the changes that were needed (patches 1-4), along with various followups (patches 5-11). These patches fix the worst of the issues with LTO compatibility; an example LTO diagnostic is: https://dmalcolm.fedorapeople.org/gcc/2019-11-18/lto-longjmp-leak-demo.html which diagnoses a memory leak due to a longjmp rewinding past cleanup code, where the malloc/free, the setjmp and the longjmp are in 3 separate source files. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. I've pushed these patches to dmalcolm/analyzer-v2 and to dmalcolm/analyzer on the GCC git mirror. David Malcolm (11): [analyzer] Fix up for params refactoring [analyzer] Fixup metadata-handling for "json::number" to "json::integer_number" [analyzer] Fixup diagnostic_path for "json::number" to "json::integer_number" [analyzer] Add params to plugin.opt [analyzer] Avoid using "convert" [analyzer] More LTO test coverage [analyzer] Fix missing leak on longjmp past a free [analyzer] Show rewind destination for leaks due to longjmp [analyzer] Add checker_path::debug [analyzer] Fix issues in diagnostic_manager::prune_path [analyzer] Updates to internal documentation gcc/analyzer/analysis-plan.cc | 3 +- gcc/analyzer/checker-path.cc | 19 gcc/analyzer/checker-path.h| 2 +- gcc/analyzer/diagnostic-manager.cc | 41 +++- gcc/analyzer/diagnostic-manager.h | 13 ++- gcc/analyzer/engine.cc | 70 ++--- gcc/analyzer/exploded-graph.h | 8 +- gcc/analyzer/plugin.opt| 16 +++ gcc/analyzer/program-point.cc | 3 +- gcc/analyzer/region-model.cc | 55 ++- gcc/analyzer/sm-file.cc| 34 +++ gcc/analyzer/sm-malloc.cc | 33 +++ gcc/analyzer/sm-pattern-test.cc| 17 gcc/analyzer/sm-sensitive.cc | 16 --- gcc/analyzer/sm-taint.cc | 16 --- gcc/analyzer/sm.h | 12 +-- gcc/diagnostic-format-json.cc | 3 +- gcc/doc/analyzer.texi | 54 +++--- .../gcc.dg/analyzer/analyzer-verbosity-0.c | 29 ++ .../gcc.dg/analyzer/analyzer-verbosity-1.c | 30 ++ .../gcc.dg/analyzer/analyzer-verbosity-2.c | 30 ++ gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-a.c | 12 +++ gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-b.c | 18 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c | 17 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto.h | 12 +++ gcc/testsuite/gcc.dg/analyzer/setjmp-7.c | 4 +- gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c | 110 + gcc/tree-diagnostic-path.cc| 6 +- 28 files changed, 538 insertions(+), 145 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-a.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-b.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto.h create mode 100644 gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c -- 1.8.5.3
[PATCH 01/11] [analyzer] Fix up for params refactoring
gcc/ChangeLog:
* analyzer/analysis-plan.cc: Remove include of "params.h".
(analysis_plan::use_summary_p): Update for conversion of params to
options.
* analyzer/engine.cc: Remove include of "params.h".
(exploded_graph::get_or_create_node): Update for conversion of
params to options.
(exploded_graph::process_worklist): Likewise.
* analyzer/program-point.cc: Remove include of "params.h".
(program_point::on_edge): Update for conversion of params to
options.
---
gcc/analyzer/analysis-plan.cc | 3 +--
gcc/analyzer/engine.cc| 6 ++
gcc/analyzer/program-point.cc | 3 +--
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/gcc/analyzer/analysis-plan.cc b/gcc/analyzer/analysis-plan.cc
index 920783b..38123ff 100644
--- a/gcc/analyzer/analysis-plan.cc
+++ b/gcc/analyzer/analysis-plan.cc
@@ -26,7 +26,6 @@ along with GCC; see the file COPYING3. If not see
#include "cgraph.h"
#include "timevar.h"
#include "ipa-utils.h"
-#include "params.h"
#include "analyzer/analyzer.h"
#include "analyzer/analysis-plan.h"
#include "analyzer/supergraph.h"
@@ -108,7 +107,7 @@ analysis_plan::use_summary_p (const cgraph_edge *edge) const
/* Require the callee to be sufficiently complex to be worth
summarizing. */
if ((int)m_sg.get_num_snodes (callee->get_fun ())
- < PARAM_VALUE (PARAM_ANALYZER_MIN_SNODES_FOR_CALL_SUMMARY))
+ < param_analyzer_min_snodes_for_call_summary)
return false;
return true;
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 34941d2..c4ca5bf 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -23,7 +23,6 @@ along with GCC; see the file COPYING3. If not see
#include "system.h"
#include "coretypes.h"
#include "tree.h"
-#include "params.h"
#include "gcc-rich-location.h"
#include "analyzer/exploded-graph.h"
#include "analyzer/analysis-plan.h"
@@ -1719,7 +1718,7 @@ exploded_graph::get_or_create_node (const program_point
,
/* Impose a limit on the number of enodes per program point, and
simply stop if we exceed it. */
if ((int)per_point_data->m_enodes.length ()
- > PARAM_VALUE (PARAM_ANALYZER_MAX_ENODES_PER_PROGRAM_POINT))
+ > param_analyzer_max_enodes_per_program_point)
{
log ("not creating enode; too many at program point");
warning_at (point.get_location (), OPT_Wanalyzer_too_complex,
@@ -2044,8 +2043,7 @@ exploded_graph::process_worklist ()
entry ENs, one per phi; the number of PK_AFTER_SUPERNODE ought
to be equivalent to the number of supernodes multiplied by the
number of states. */
- const int limit
- = m_sg.num_nodes () * PARAM_VALUE (PARAM_ANALYZER_BB_EXPLOSION_FACTOR);
+ const int limit = m_sg.num_nodes () * param_analyzer_bb_explosion_factor;
if (m_global_stats.m_num_nodes[PK_AFTER_SUPERNODE] > limit)
{
log ("bailing out; too many nodes");
diff --git a/gcc/analyzer/program-point.cc b/gcc/analyzer/program-point.cc
index fcf77af..ea84238 100644
--- a/gcc/analyzer/program-point.cc
+++ b/gcc/analyzer/program-point.cc
@@ -25,7 +25,6 @@ along with GCC; see the file COPYING3. If not see
#include "tree.h"
#include "gimple-pretty-print.h"
#include "gcc-rich-location.h"
-#include "params.h"
#include "analyzer/program-point.h"
#include "analyzer/exploded-graph.h"
#include "analyzer/analysis-plan.h"
@@ -262,7 +261,7 @@ program_point::on_edge (exploded_graph ,
applies to recursion (and mutual recursion), not to
general call stacks. */
if (m_call_string.calc_recursion_depth ()
- > PARAM_VALUE (PARAM_ANALYZER_MAX_RECURSION_DEPTH))
+ > param_analyzer_max_recursion_depth)
{
eg.log ("rejecting call edge: recursion limit exceeded");
// TODO: issue a sorry for this?
--
1.8.5.3
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On Wed, Nov 20, 2019 at 10:38:30PM +0200, Janne Blomqvist wrote: > On Wed, Nov 20, 2019 at 8:00 PM Bernhard Reutner-Fischer > wrote: > > > > On 19 November 2019 23:54:55 CET, Thomas Koenig > > wrote: > > >Am 19.11.19 um 11:39 schrieb Bernhard Reutner-Fischer: > > >> + char name[GFC_MAX_SYMBOL_LEN + 1]; > > >> + snprintf (name, GFC_MAX_SYMBOL_LEN, "__dummy_%d_%s", var_num++, > > >> +f->sym->name); > > >> + > > >> + if (gfc_get_sym_tree (name, ns, , false) != 0) > > >> > > >> (I) you should + sizeof(__dummy__) + 10 for unsigned long %d or the > > >like. > > > > > >GFC_MAX_SYMBOL_LEN is the maximum length of a gfortran symbol. AFAIK, > > > > This is only true for user-provided names in the source code. > > > > Think e.g. class names as can be seen in the dumps.. > > We have GFC_MAX_MANGLED_SYMBOL_LEN for that. *Insert my standard pet > peeve rant that we should use heap allocated unlimited length strings > for these rather than copying stack allocated strings around, or > preferable a symbol table* > I agree with Janne. This seems like a very good candidate for someone that would like to contribute to gfortran, but does not know where to start. Any lurkers on the mailing list care to give it shot? -- Steve
Re: [PATCH, rs6000][committed] Fix PR92090: Allow MODE_PARTIAL_INT modes for integer constant input operands.
On 11/7/19 1:06 PM, Peter Bergner wrote: > Before, LRA, we have an insn that sets a TImode pseudo with an integer > constant and a following insn that copies that TImode pseudo to a PTImode > pseudo. During LRA spilling, we generate a new insn that sets a PTImode > pseudo to that constant directly and we ICE because we do not recognize > that as a valid insn. The fix below fixes the ICE reported in PR92090 by > modifying our input_operand predicate to allow MODE_PARTIAL_INT modes for > integer constant input operands. > > This patch (preapproved by Segher) passed bootstrap and regtesting > with no errors. Committed. I have now committed the backports to the GCC 9 and GCC 8 branches, including the test case updates from David. Peter
Re: [C++ PATCH] c++/92450 - ICE with invalid nested name specifier.
On Tue, Nov 19, 2019 at 05:34:45PM -0500, Jason Merrill wrote:
> On 11/18/19 7:04 PM, Marek Polacek wrote:
> > The newly added diagnostic causes an ICE because the new grokdeclarator call
> > returned error_mark_node and DECL_SOURCE_LOCATION crashes on that. So don't
> > attempt to print the new diagnostic if we've encountered something not
> > resembling a bit-field.
> >
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >
> > 2019-11-18 Marek Polacek
> >
> > PR c++/92450 - ICE with invalid nested name specifier.
> > * parser.c (cp_parser_member_declaration): Bail out for invalid code.
> >
> > * g++.dg/parse/crash71.C: New test.
> >
> > diff --git gcc/cp/parser.c gcc/cp/parser.c
> > index c473e7fd92f..caed6946977 100644
> > --- gcc/cp/parser.c
> > +++ gcc/cp/parser.c
> > @@ -25052,6 +25052,9 @@ cp_parser_member_declaration (cp_parser* parser)
> > tree d = grokdeclarator (declarator, _specifiers,
> >BITFIELD, /*initialized=*/false,
> >);
> > + /* Hopelessly invalid code, give up. */
> > + if (error_operand_p (d))
> > + goto out;
> > error_at (DECL_SOURCE_LOCATION (d),
> > "bit-field %qD has non-integral type %qT",
> > d, TREE_TYPE (d));
>
> Don't we still want the rest of the error-recovery code there, i.e. skipping
> to the end of the statement?
Sure, that works too, we just emit fewer diagnostics. But maybe that's not
that interesting at this point, because we've already given an error.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2019-11-20 Marek Polacek
PR c++/92450 - ICE with invalid nested name specifier.
* parser.c (cp_parser_member_declaration): Don't attempt to print
erroneous bit-field diagnostic if grokdeclarator returns
error_mark_node.
* g++.dg/parse/crash71.C: New test.
diff --git gcc/cp/parser.c gcc/cp/parser.c
index c473e7fd92f..ad35945f954 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -25052,9 +25052,10 @@ cp_parser_member_declaration (cp_parser* parser)
tree d = grokdeclarator (declarator, _specifiers,
BITFIELD, /*initialized=*/false,
);
- error_at (DECL_SOURCE_LOCATION (d),
- "bit-field %qD has non-integral type %qT",
- d, TREE_TYPE (d));
+ if (!error_operand_p (d))
+ error_at (DECL_SOURCE_LOCATION (d),
+ "bit-field %qD has non-integral type %qT",
+ d, TREE_TYPE (d));
cp_parser_skip_to_end_of_statement (parser);
/* Avoid "extra ;" pedwarns. */
if (cp_lexer_next_token_is (parser->lexer,
diff --git gcc/testsuite/g++.dg/parse/crash71.C
gcc/testsuite/g++.dg/parse/crash71.C
new file mode 100644
index 000..13f484801fe
--- /dev/null
+++ gcc/testsuite/g++.dg/parse/crash71.C
@@ -0,0 +1,11 @@
+// PR c++/92450 - ICE with invalid nested name specifier.
+
+typedef int C2;
+struct B1 {
+ struct B2 {
+ };
+};
+
+struct S6g {
+ C2 : B1:B2; // { dg-error "" }
+};
Re: [PATCH 0/4] Eliminate cc0 from m68k
On 11/20/19 8:27 PM, Mikael Pettersson wrote: > On Wed, Nov 20, 2019 at 3:16 PM Bernd Schmidt wrote: >> Probably best to just run tests on stage1 and hope something shows up. > > Ok, how do I did that? I've always just done 'make -k check' after > full bootstraps. > I assume the stage 1 artifacts are the ones in the prev-* directories. There's a --disable-bootstrap configure option. >> What distro are you using for native builds? The m68k debian I'm using >> does not have an installable gcc package. > > I run a bespoke distro on m68k and sparc64, derived from Fedora but > massively cut down in size, with target patches done by myself or > ported from Debian and Gentoo as necessary. > > Debian/m68k not having a gcc package? That sounds odd; I see e.g. a > gcc-9 in http://ftp.ports.debian.org/debian-ports/pool-m68k/main/g/ Turns out I wasn't sufficiently familiar with how debian works. I was missing an "apt update" step. I kind of assumed a package like gcc would install out of the box (others did, things like emacs). Bernd
Re: Add a new combine pass
On Wed, Nov 20, 2019 at 06:20:34PM +, Richard Sandiford wrote: > Segher Boessenkool writes: > > So this would work if you had pseudos here, instead of the hard reg? > > Because it is a hard reg it is the same number in both places, making it > > hard to move. > > Yeah, probably. But the hard reg is a critical part of this. > Going back to the example: > > (set (reg/v:VNx16BI 102 [ ok ]) > (reg:VNx16BI 85 ffrt)) > (set (reg:VNx16BI 85 ffrt) > (unspec:VNx16BI [(reg:VNx16BI 85 ffrt)] UNSPEC_UPDATE_FFRT)) > (set (reg:CC_NZC 66 cc) > (unspec:CC_NZC >[(reg:VNx16BI 106) repeated x2 > (const_int 1 [0x1]) > (reg/v:VNx16BI 102 [ ok ])] UNSPEC_PTEST)) > > FFR is the real first fault register. FFRT is actually a fake register > whose only purpose is to describe the dependencies (in rtl) between writes > to the FFR, reads from the FFR and first-faulting loads. The whole scheme > depends on having only one fixed FFRT register. Right. The reason this cannot work in combine is that combine always combines to just *one* insn, at i3; later, if it turns out that it needs to split it, it can put something at i2. But that doesn't even happen here, only the first and the last of those three insns are what is combined. It is important combine only moves things forward in the insn stream, to make sure this whole process is finite. Or this was true years ago, at least :-) > > Why don't you use DF for the DU chains? > > The problem with DF_DU_CHAIN is that it's quadratic in the worst case. Oh, wow. > fwprop.c gets around that by using the MD problem and having its own > dominator walker to calculate limited def-use chains: > > /* We use the multiple definitions problem to compute our restricted > use-def chains. */ It's not great if every pass invents its own version of some common infrastructure thing because that common one is not suitable. I.e., can this be fixed somehow? Maybe just by having a restricted DU chains df problem? > So taking that approach here would still require some amount of > roll-your-own. Other reasons are: > > * Even what fwprop does is more elaborate than we need for now. > > * We need to handle memory too, and it's nice to be able to handle > it in the same way as registers. > > * Updating a full, ordered def-use chain after a move is a linear-time > operation, so whatever happens, we'd need to apply some kind of limit > on the number of uses we maintain, with something like that integer > point range for the rest. > > * Once we've analysed the insn and built its def-use chains, we don't > look at the df_refs again until we update the chains after a successful > combination. So it should be more efficient to maintain a small array > of insn_info_rec pointers alongside the numerical range, rather than > walk and pollute chains of df_refs and then link back the insn uids > to the pass-local info. So you need something like combine's LOG_LINKS? Not that handling those is not quadratic in the worst case, but in practice it works well. And it *could* be made linear. > >> Tracking limited def-use chains is what makes this last bit easy. > >> We can just try parallelising two instructions from the (bounded) list > >> of uses. And for this case there's not any garbage rtl involved, since > >> we reuse the same PARALLEL rtx between attempts. The cost is basically > >> all in the recog call (which would obviously mount up if we went > >> overboard). > > > > *All* examples above and below are just this. > > Yeah, the powerpc and s390x examples were. The motivating FFR example > above isn't though: it's a def-use combination in parallel with the > existing definition. Right, good point :-) > >> >> To get a feel for the effect on multiple targets, I did my usual > >> >> bogo-comparison of number of lines of asm for gcc.c-torture, gcc.dg > >> >> and g++.dg, this time comparing run-combine=2 and run-combine=6 > >> >> using -O2 -ftree-vectorize: > >> > > >> > One problem with this is that these are very short functions on average. > >> > >> There are some long ones too :-) > > > > Yes, but this isn't a good stand-in for representative programs. > > Right. And number of lines of asm isn't a good stand-in for anything much. For combine, number of insns generated is a surprisingly good measure of how it performed. Sometimes not, when it goes over a border of an inlining decision, say, or bb-reorder decides to duplicate more because it is cheaper now. > Like I say, the whole thing is just to get a feel, on tests that are readily > to hand and are easy to compile without a full toolchain. Absolutely. But I have no experience with using your test set, so the numbers do not necessarily mean so much to me :-) > > So I'd love to see statistics for *only* combining two uses of the same > > thing, this is something combine cannot do, and arguably *shouldn't* do! > > OK, here are two sets of results. The first is for:
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On Sat, Nov 16, 2019 at 10:34 PM Thomas Koenig wrote: > > Hello world, > > here is an update to the patch. > > I have now included variables where the user did not specify INTENT(IN) > by checking that the dummy variables to be replaced by temporaries > are not, indeed, assigned a value. This also includes being passed > as an actual argument to a non-INTENT(IN) dummy argument. > > Extending this led to being able to catch a few more bugs. > > I have addes one test case to check where the new temporaries are added. > > Regression-tested. The only change I see in the testsuite now is > > XPASS: gfortran.dg/goacc/kernels-loop-n.f95 -O scan-tree-dump-times > parloops1 "(?n)__attribute__\\(\\(oacc kernels parallelized, oacc > function \\(, , \\), oacc kernels, omp target entrypoint\\)\\)" 1 BTW, since this is done for the purpose of optimization, have you done testing on some suitable benchmark suite such as polyhedron, whether it a) generates any different code b) does it make it go faster? Is there a risk of performance regressions due to higher register pressure? -- Janne Blomqvist
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On Wed, Nov 20, 2019 at 8:00 PM Bernhard Reutner-Fischer wrote: > > On 19 November 2019 23:54:55 CET, Thomas Koenig wrote: > >Am 19.11.19 um 11:39 schrieb Bernhard Reutner-Fischer: > >> + char name[GFC_MAX_SYMBOL_LEN + 1]; > >> + snprintf (name, GFC_MAX_SYMBOL_LEN, "__dummy_%d_%s", var_num++, > >> +f->sym->name); > >> + > >> + if (gfc_get_sym_tree (name, ns, , false) != 0) > >> > >> (I) you should + sizeof(__dummy__) + 10 for unsigned long %d or the > >like. > > > >GFC_MAX_SYMBOL_LEN is the maximum length of a gfortran symbol. AFAIK, > > This is only true for user-provided names in the source code. > > Think e.g. class names as can be seen in the dumps.. We have GFC_MAX_MANGLED_SYMBOL_LEN for that. *Insert my standard pet peeve rant that we should use heap allocated unlimited length strings for these rather than copying stack allocated strings around, or preferable a symbol table* > >it > >is not possible to use a longer symbol name than that, so it needs to > >be > >truncated. Uniqueness of the variable name is guaranteed by the var_num > >variable. > > > >If the user puts dummy arguments Supercalifragilisticexpialidociousa > >and > >Supercalifragilisticexpialidociousb into the argument list of a > >procedure, he will have to look at the numbers to differentiate them > >:-) > > > >> (II) s/__dummy/__intent_in/ for clarity? > > > >It's moved away a bit from INTENT(IN) now, because an argument which > >cannot be modified (even by passing to a procedure with a dummy > >argument > >with unknown intent) is now also handled. > > So maybe __readonly_ , __rdonly_, __rd_or the like? dummy is really > misleading a name in the dumps.. Well, dummy is a term with a precise definition in the Fortran standard, so in a way it's good so one realizes it has something to do with a dummy argument. But yes, it's a bit misleading because it's not the dummy argument itself but rather a dereferenced copy of it. I suggest __readonly_dereferenced_dummy_copy_yes_this_is_a_really_long_name_. :) -- Janne Blomqvist
Re: C++ PATCH for c++/91363 - P0960R3: Parenthesized initialization of aggregates
On Tue, Nov 19, 2019 at 05:33:09PM -0500, Jason Merrill wrote:
> On 11/19/19 1:44 AM, Marek Polacek wrote:
> > > It also looks like you're using the LOOKUP flag to mean two different
> > > things:
> > >
> > > 1) try to treat parenthesized args as an aggregate initializer
> > > (build_new_method_call_1)
> > > 2) treat this CONSTRUCTOR as coming from parenthesized args
> > > (store_init_value/digest_init)
> >
> > Correct.
> >
> > > Why is the flag needed for #1? When do we not want to try to treat the
> > > args
> > > as an aggregate initializer?
> >
> > There are cases where treating the args as an aggregate initializer causes
> > spurious overload resolution successes, e.g.
> >
> > void swap(int&, int&);
> >
> > int& get();
> >
> > struct pair {
> > void swap(pair&) noexcept(noexcept(swap(get(), get( { } // {
> > dg-error "no matching function for call" }
> > };
> >
> > There are no viable candidates for pair::swap (# of args mismatch) but since
> > pair is an aggregate, build_new_method_call_1 would return a CONSTRUCTOR so
> > overload resolution would succeed. Another case had to do with SFINAE and
> > decltype where we didn't evaluate the arg, but succeeding in the
> > no-viable-function case caused the compiler to choose the wrong function.
>
> Hmm, but then the parenthesized list is arguments for swap, not an
> initializer for a single argument of swap. That would be using it for
> copy-initialization, and we only want to treat parenthesized args as an
> aggregate initializer in direct-initialization. Can we check for
> direct-init (i.e. !LOOKUP_ONLYCONVERTING) instead?
Unfortunately that doesn't work. We call build_new_method_call from context
where LOOKUP_ONLYCONVERTING isn't set and so it would still break things.
> > - expr = get_target_expr_sfinae (digest_init (totype, expr, complain),
> > + /* We want an implicit lookup, but when initializing an aggregate
> > +from a parenthesized list, we must remember not to warn about
> > +narrowing conversions. */
> > + flags = LOOKUP_IMPLICIT;
> > + if (CONSTRUCTOR_IS_PAREN_INIT (expr))
> > + flags |= LOOKUP_AGGREGATE_PAREN_INIT;
> > + expr = get_target_expr_sfinae (digest_init_flags (totype, expr,
> > + flags, complain),
>
> I was thinking to set the flag inside digest_init, so callers don't need to
> know about it.
Got it, done.
> > @@ -6704,6 +6761,14 @@ check_initializer (tree decl, tree init, int flags,
> > vec **cleanups)
> > }
> > init_code = NULL_TREE;
> > }
> > + else if (BRACE_ENCLOSED_INITIALIZER_P (init_code))
> > + {
> > + /* In C++20, the call to build_aggr_init could have created
> > +a CONSTRUCTOR to handle A(1, 2). */
> > + gcc_assert (cxx_dialect >= cxx2a);
> > + init = init_code;
> > + flags |= LOOKUP_AGGREGATE_PAREN_INIT;
> > + }
>
> I think you want to clear init_code after copying it into init.
In fact, now I dropped this whole hunk.
> > + init = build_aggr_init (decl, init, flags, tf_warning_or_error);
> > + /* In C++20, a member initializer list can be initializing an
> > +aggregate from a parenthesized list of values:
> > +
> > + struct S {
> > +A aggr;
> > +S() : aggr(1, 2, 3) { }
> > + };
> > +
> > + Build up an INIT_EXPR like we do for aggr{1, 2, 3}, so that
> > + build_data_member_initialization can grok it. */
> > + if (cxx_dialect >= cxx2a)
> > + {
> > + tree t = init;
> > + while (TREE_CODE (t) == EXPR_STMT
> > +|| TREE_CODE (t) == CONVERT_EXPR)
> > + t = TREE_OPERAND (t, 0);
> > + if (BRACE_ENCLOSED_INITIALIZER_P (t))
> > + {
> > + t = digest_init_flags (type, t,
> > +(LOOKUP_IMPLICIT
> > + | LOOKUP_AGGREGATE_PAREN_INIT),
> > +tf_warning_or_error);
> > + init = build2 (INIT_EXPR, type, decl, t);
> > + }
> > + }
>
> All this should happen within the call to build_aggr_init.
> expand_default_init already builds INIT_EXPR in various cases; it could do
> so for this case, too.
True, done.
> > + if (BRACE_ENCLOSED_INITIALIZER_P (exp))
> > +{
> > + gcc_assert (cxx_dialect >= cxx2a);
> > + return finish_compound_literal (type, exp, complain,
> > + fcl_functional_paren);
> > +}
>
> How about handling this in build_cplus_new instead of places that also call
> build_cplus_new?
Is it really what we want? We now have two spots where we need to handle
the case when build_special_member_call returns a BRACE_ENCLOSED_INITIALIZER_P
but build_cplus_new is called in many other spots where we don't expect to see
a CONSTRUCTOR.
> It might also be better to call digest_init in
Re: [PATCH] Switch gcc ftp URL's to http
On Wed, Nov 20, 2019 at 7:53 PM Joseph Myers wrote: > > This patch is OK with http changed to https. (That is, with it changed > where the patch is already changing the URL. While changing http to https > makes sense more generally in the documentation whenever a site supports > https, that's probably best not mixed with the move from ftp.) Thanks for the review, done, and committed as r278526. Any opinions on whether I should apply this to the 8 and 9 branches as well? And the same question for my previous patch updating contrib/download_prerequisites at https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00957.html . -- Janne Blomqvist
Re: [PATCH 0/4] Eliminate cc0 from m68k
On 11/20/19 12:27 PM, Mikael Pettersson wrote: > On Wed, Nov 20, 2019 at 3:16 PM Bernd Schmidt > wrote: >> >> On 11/20/19 2:50 PM, Mikael Pettersson wrote: >>> On Mon, Nov 18, 2019 at 9:57 PM Mikael Pettersson >>> wrote: On Mon, Nov 18, 2019 at 8:31 PM Bernd Schmidt wrote: > > Hi Mikael, > >> This fixed the problem, thanks. > > Could you also run the testsuite to see if you can reproduce > the g++.old-deja failures Andreas reported? Yes, but it will probably take another week before the native bootstrap (on aranym) and test suite run is finished. It's currently in stage 2. >> >> Ugh, that suggests the stage2 compiler was miscompiled. That would >> be nasty to track down. >> >> Probably best to just run tests on stage1 and hope something shows >> up. > > Ok, how do I did that? I've always just done 'make -k check' after > full bootstraps. I assume the stage 1 artifacts are the ones in the > prev-* directories. I do something like this make all-gcc && make all-target-libgcc cd gcc make check-gcc Jeff
Re: [PATCH 1/2] PR 92463 MPFR modernization in GFortran
On Wed, Nov 20, 2019 at 5:46 PM Tobias Burnus wrote: > > Hi Janne, > > On 11/18/19 9:34 PM, Janne Blomqvist wrote: > > Now that we require a minimum of MPFR 3.1.0+ to build GCC, we can do > > some modernization of the MPFR usage in the GFortran frontend. > > OK – thanks for the cleanup. > > [For reference, those changes were introduced in MPFR 3.0.0, cf. > https://www.mpfr.org/mpfr-3.0.0/#changes ; 3.0 and 3.1 also added a > bunch of new functions, MPFR_RNDA and did remove some undefined behaviour.] > > Do you intent update the other places as well? Namely: > > > This patch replaces > > 1) GMP_RND* with MPFR_RND* > Also used by: configure.ac, gcc/builtins.c, gcc/fold-const-call.c, > gcc/gimple-ssa-sprintf.c, gcc/go/…, gcc/real.c, gcc/realmpfr.h, > gcc/ubsan.c. > > 2) mp_exp_t with mpfr_exp_t > > gcc/realmpfr.c + gcc/go/… > > > 3) mp_prec_t with mpfr_prec_t > (only used by gcc/fortran) > > 4) mp_rnd_t with mpfr_rnd_t > > gcc/builtins.c, gcc/fold-const-call.c, gcc/realmpfr.c > Thanks for the review, committed as r278523. My plan was that once the fortran-specific parts are done, I'll reassign the PR to the middle-end, and then me or somebody else can make a separate patch for that. However, since we're in stage3 now I believe this will have to wait until after the GCC 10 release as the middle-end people seem to take these rules more seriously. -- Janne Blomqvist
Re: [PATCH 0/4] Eliminate cc0 from m68k
On Wed, Nov 20, 2019 at 3:16 PM Bernd Schmidt wrote: > > On 11/20/19 2:50 PM, Mikael Pettersson wrote: > > On Mon, Nov 18, 2019 at 9:57 PM Mikael Pettersson > > wrote: > >> > >> On Mon, Nov 18, 2019 at 8:31 PM Bernd Schmidt > >> wrote: > >>> > >>> Hi Mikael, > >>> > This fixed the problem, thanks. > >>> > >>> Could you also run the testsuite to see if you can reproduce the > >>> g++.old-deja failures Andreas reported? > >> > >> Yes, but it will probably take another week before the native > >> bootstrap (on aranym) and test suite run is finished. It's currently > >> in stage 2. > > Ugh, that suggests the stage2 compiler was miscompiled. That would be > nasty to track down. > > Probably best to just run tests on stage1 and hope something shows up. Ok, how do I did that? I've always just done 'make -k check' after full bootstraps. I assume the stage 1 artifacts are the ones in the prev-* directories. > What distro are you using for native builds? The m68k debian I'm using > does not have an installable gcc package. I run a bespoke distro on m68k and sparc64, derived from Fedora but massively cut down in size, with target patches done by myself or ported from Debian and Gentoo as necessary. Debian/m68k not having a gcc package? That sounds odd; I see e.g. a gcc-9 in http://ftp.ports.debian.org/debian-ports/pool-m68k/main/g/
Re: [C++ PATCH] Fix concepts vs. PCH (PR c++/92458)
On Mon, Nov 18, 2019 at 02:41:48PM -0500, Jason Merrill wrote:
> > 2019-11-11 Jakub Jelinek
> >
> > PR c++/92458
> > * constraint.cc: Include tree-hash-traits.h.
> > (decl_tree_cache_traits): New type.
> > (decl_tree_cache_map): New typedef.
> > (decl_constraints): Change type to decl_tree_cache_map *
> > from tree_cache_map *.
>
> Do we need to change other tree_cache_map uses, too? It looks like many of
> them map from decls.
I guess it depends on whether those hash tables can have data in them across
PCH save/restore.
As an experiment, I've built stdc++.h.gch with -std=c++2a and put a
breakpoint in c_common_read_pch after gt_pch_restore.
Besides decl_constraints I've changed, I see also defarg_inst table with
data on it, which means that defarg_inst lookups after PCH read might not
find saved instantiations in the table.
So, defarg_inst might be another candidate for decl_tree_cache_map,
especially because PARM_DECLs are the keys in it.
All the other C++ FE tree_cache_map hash tables are empty, so no idea if it
is needed or not.
If decl_tree_cache_map will be needed in more than one spot, I'll probably
need to move it to some generic header.
> > --- gcc/cp/constraint.cc.jj 2019-11-07 09:50:51.755239234 +0100
> > +++ gcc/cp/constraint.cc2019-11-11 19:04:03.231862024 +0100
> > @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.
> > #include "decl.h"
> > #include "toplev.h"
> > #include "type-utils.h"
> > +#include "tree-hash-traits.h"
> > static tree satisfaction_value (tree t);
> > @@ -1113,7 +1114,10 @@ build_constraints (tree tr, tree dr)
> > /* A mapping from declarations to constraint information. */
> > -static GTY ((cache)) tree_cache_map *decl_constraints;
> > +struct decl_tree_cache_traits
> > + : simple_cache_map_traits { };
> > +typedef hash_map decl_tree_cache_map;
> > +static GTY ((cache)) decl_tree_cache_map *decl_constraints;
> > /* Returns the template constraints of declaration T. If T is not
> > constrained, return NULL_TREE. Note that T must be non-null. */
Jakub
Re: [C++ PATCH] Fix weird addr_expr not supported by dump_expr messages (PR c++/90767)
On 11/20/19 10:54 AM, David Malcolm wrote: On Wed, 2019-11-20 at 00:38 +0100, Jakub Jelinek wrote: Hi! The following patch is a minimal fix to avoid cannot convert ‘‘addr_expr’ not supported by dump_type’ to ‘X*’ and similar messages. The recently added complain_about_bad_argument function expects a from_type argument, but conv->from isn't necessarily a type, it can be an expression too. With this patch one gets error like: cannot convert ‘const X*’ to ‘X*’ and note like initializing argument 'this' of ‘void X::foo()’ Still, perhaps what GCC 8 and earlier used to emit might be clearer: pr90767-1.C: In member function ‘X::operator T() const’: pr90767-1.C:12:7: error: no matching function for call to ‘X::foo() const’ pr90767-1.C:6:8: note: candidate: ‘void X::foo()’ pr90767-1.C:6:8: note: passing ‘const X*’ as ‘this’ argument discards qualifiers There is the print_conversion_rejection function that handles the various cases, like this vs. other arguments, conv->from with expr type vs. type etc. Though, I must say I don't understand the reasons why complain_about_bad_argument has been added and whether we'd want to emit there what print_conversion_rejection prints as notes with 2 leading spaces instead as errors with no leading spaces. I added complain_about_bad_argument in r264250 (aka b78e49d1ddf1a09e16152353b41bf7c0b44d6405); the intent is to special- case when there's a single candidate due to a bad argument, underlining the pertinent bogus argument at the callsite, rather than having the caret at the final close-paren of the call. Since apparently we already have the relevant location in conversion_info, could we achieve that by improving print_conversion_rejection rather than using a separate complain_about_bad_argument function? Jason
Re: [C++ Patch] Avoid redundant error messages from build_x_arrow
On 11/20/19 4:40 PM, Paolo Carlini wrote: Hi, while working on improving the locations of cp_build_indirect_ref_1 & co, I noticed this nit which seems a separate issue. In a nutshell, at variance with many other cases, in build_x_arrow we don't immediately check for error_mark_node the return value of decay_conversion. Then, for the testcase, after a sensible: error: invalid use of member function ‘C* C::f()’ (did you forget the ‘()’ ?) we also issue: error: base operand of ‘->’ is not a pointer which is certainly redundant and a bit misleading, is talking about the 'f' mentioned in the first message. The amended behavior is also consistent with EDG and CLANG. Tested x86_64-linux, as usual. Thanks, Paolo. // OK.
Re: [C/C++ PATCH] Fix up build of GCC 4.6 and earlier with GCC 9+ (PR c/90677)
On 11/19/19 11:29 PM, Jakub Jelinek wrote:
Hi!
The following patch fixes build of older GCC releases with newer ones.
In GCC 4.6 and earlier, we had:
struct cgraph_node;
struct cgraph_node *cgraph_node (tree);
and that is something on which GCC 9+ code errors on if it sees any
__gcc_diag__ and similar attributes, because cgraph_node it wants to find is
not a type.
As older GCC releases don't have the __gcc_diag__ etc. attributes guarded on
no newer GCC releases, only on minimum GCC version that does support it,
I think we need to make sure we don't reject what older GCCs used to have.
The following patch does that. In addition, get_pointer_to_named_type
looked misnamed, because we actually aren't interested in getting gimple *
etc. type, but gimple.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
and eventually 9 too?
2019-11-19 Jakub Jelinek
PR c/90677
* c-format.c (get_pointer_to_named_type): Renamed to ...
(get_named_type): ... this. If result is FUNCTION_DECL for
cgraph_node, return cgraph_node from pointee of return type if
possible instead of emitting an error.
(init_dynamic_diag_info): Adjust get_pointer_to_named_type callers
to call get_named_type instead. Formatting fixes.
* c-c++-common/pr90677.c: New test.
--- gcc/c-family/c-format.c.jj 2019-10-05 09:35:12.917997709 +0200
+++ gcc/c-family/c-format.c 2019-11-19 13:05:10.113308578 +0100
@@ -4899,21 +4899,39 @@ init_dynamic_gfc_info (void)
}
}
-/* Lookup the type named NAME and return a pointer-to-NAME type if found.
- Otherwise, return void_type_node if NAME has not been used yet, or
NULL_TREE if
- NAME is not a type (issuing an error). */
+/* Lookup the type named NAME and return a NAME type if found.
+ Otherwise, return void_type_node if NAME has not been used yet,
+ or NULL_TREE if NAME is not a type (issuing an error). */
static tree
-get_pointer_to_named_type (const char *name)
+get_named_type (const char *name)
{
tree result;
- if ((result = maybe_get_identifier (name)))
+ if (tree id = maybe_get_identifier (name))
{
- result = identifier_global_value (result);
+ result = identifier_global_value (id);
I would think that get_named_type should find struct or enum names that
have been hidden by another declaration; that would fix this without
special-casing cgraph_node. For the C++ front-end, that would be
lookup_qualified_name (global_namespace, id, /*prefer_type*/2,
/*complain*/false)
Jason
Re: [C++ PATCH] Fix up lambda decl specifier parsing ICE (PR c++/90842)
On 11/20/19 9:16 AM, Jakub Jelinek wrote: On Tue, Nov 19, 2019 at 11:35:02PM -0500, Jason Merrill wrote: It would seem better to break after consuming the token, so we just skip the extra processing and still give the same error. And instead of this, maybe set CP_PARSER_FLAGS_NO_TYPE_DEFINITIONS so we keep the same diagnostic for other type-specifiers? That seems to work well. So like this if it passes bootstrap/regtest? OK. Jason
[AArch64] [mid-end] [__RTL] Allow backends to set state when skipping passes.
When compiling __RTL functions with a start pass, `skip_pass` needs to
set global state when skipping a pass that would have marked something
for future passes to see.
Existing examples are setting `reload_completed` and
`epilogue_completed` when skipping the reload and pro_and_epilogue
passes respectively.
This general problem occurs again in the AArch64 backend, where the
cfun->machine->frame.laid_out variable is set on each function when
`aarch64_layout_frame` is called (which can happen in both ira and
reload.
Currently any `return` statement will trigger an assert in the AArch64
backend due to checking if address signing is enabled when the above
variable is not set.
To account for the need to have a backend specific settings we add a
target hook that each backend can implement as required for their own
global state.
Here we also account for the motivating example by implementing this
hook for the AArch64 backend.
regtested on x86_64 and aarch64.
gcc/ChangeLog:
2019-11-20 Matthew Malcomson
* config/aarch64/aarch64.c (aarch64_skip_pass): New.
(TARGET_BACKEND_SKIP_PASS): New.
* doc/tm.texi: Document BACKEND_SKIP_PASS hook.
* doc/tm.texi.in: Document BACKEND_SKIP_PASS hook.
* passes.c (skip_pass): Call targetm.backend_skip_pass if it's
defined.
* target.def (backend_skip_pass): Introduce new hook.
gcc/testsuite/ChangeLog:
2019-11-20 Matthew Malcomson
* gcc.dg/rtl/aarch64/return-statement.c: New test.
### Attachment also inlined for ease of reply###
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index
599d07a729e7438080f8b5240ee95037a49fb983..fbe900dec8d5ef3daf7be691cdaf4350c13d5024
100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -75,6 +75,7 @@
#include "intl.h"
#include "expmed.h"
#include "function-abi.h"
+#include "tree-pass.h"
/* This file should be included last. */
#include "target-def.h"
@@ -21371,6 +21372,14 @@ aarch64_stack_protect_guard (void)
return NULL_TREE;
}
+/* Implement TARGET_BACKEND_SKIP_PASS. We need to set
+ cfun->machine->frame.laid_out when skipping the ira pass. */
+void aarch64_skip_pass (opt_pass *pass)
+{
+ if (strcmp (pass->name, "ira") == 0)
+cfun->machine->frame.laid_out = true;
+}
+
/* Implement TARGET_ASM_FILE_END for AArch64. This adds the AArch64 GNU NOTE
section at the end if needed. */
#define GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc000
@@ -21947,6 +21956,9 @@ aarch64_libgcc_floating_mode_supported_p
#undef TARGET_STRICT_ARGUMENT_NAMING
#define TARGET_STRICT_ARGUMENT_NAMING hook_bool_CUMULATIVE_ARGS_true
+#undef TARGET_BACKEND_SKIP_PASS
+#define TARGET_BACKEND_SKIP_PASS aarch64_skip_pass
+
struct gcc_target targetm = TARGET_INITIALIZER;
#include "gt-aarch64.h"
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index
cd9aed9874f4e6b2b0e2f8956ed6155975e643a8..d02fd747b479246cfdf7c7e6af03a1a2bd43e1c4
100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -12152,3 +12152,7 @@ This target hook can be used to generate a
target-specific code
@deftypefn {Target Hook} void TARGET_RUN_TARGET_SELFTESTS (void)
If selftests are enabled, run any selftests for this target.
@end deftypefn
+
+@deftypefn {Target Hook} void TARGET_BACKEND_SKIP_PASS (opt_pass *@var{pass})
+Given pass currently skipping, run any actions to set up state. When
compiling code from the RTL frontend, some passes are skipped. If a given pass
would set backend state to mark that a given action has been done, then the
backend should implement this hook to set the same state when that pass is
skipped. As an example, the AArch64 backend sets the backend specific
cfun->machine->frame.laid_out structure member when the ira pass is skipped.
+@end deftypefn
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index
2739e9ceec5ad7253ff9135da8dbe3bf6010e8d7..7cf446a88abe1eb3bfdcf1c685fc19905d46fca0
100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -8194,3 +8194,5 @@ maintainer is familiar with.
@hook TARGET_SPECULATION_SAFE_VALUE
@hook TARGET_RUN_TARGET_SELFTESTS
+
+@hook TARGET_BACKEND_SKIP_PASS
diff --git a/gcc/passes.c b/gcc/passes.c
index
d86af115ecb16fcab6bfce070f1f3e4f1d90ce71..16b3eb106f52da88d4a5717cdc4b66efd4f2c9d4
100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -2416,6 +2416,9 @@ skip_pass (opt_pass *pass)
rtl_register_cfg_hooks ();
cfun->curr_properties &= ~PROP_cfglayout;
}
+
+ if (targetm.backend_skip_pass)
+targetm.backend_skip_pass(pass);
}
/* Execute PASS. */
diff --git a/gcc/target.def b/gcc/target.def
index
8e83c2c7a7136511c07a5bc9e18876c91a38b955..f5313fa1ebe50875559e68792991c6ea37e15bcb
100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -6765,6 +6765,12 @@ DEFHOOK
void, (void),
NULL)
+DEFHOOK
+(backend_skip_pass,
+ "Given pass currently skipping, run any actions to set up state. When
compiling code from the RTL
Re: Add a new combine pass
Segher Boessenkool writes: >> /* Make sure that the value that is to be substituted for the register >> does not use any registers whose values alter in between. However, >> If the insns are adjacent, a use can't cross a set even though we >> think it might (this can happen for a sequence of insns each setting >> the same destination; last_set of that register might point to >> a NOTE). If INSN has a REG_EQUIV note, the register is always >> equivalent to the memory so the substitution is valid even if there >> are intervening stores. Also, don't move a volatile asm or >> UNSPEC_VOLATILE across any other insns. */ >> || (! all_adjacent >>&& (((!MEM_P (src) >> || ! find_reg_note (insn, REG_EQUIV, src)) >> && modified_between_p (src, insn, i3)) >>|| (GET_CODE (src) == ASM_OPERANDS && MEM_VOLATILE_P (src)) >>|| GET_CODE (src) == UNSPEC_VOLATILE)) > > So this would work if you had pseudos here, instead of the hard reg? > Because it is a hard reg it is the same number in both places, making it > hard to move. Yeah, probably. But the hard reg is a critical part of this. Going back to the example: (set (reg/v:VNx16BI 102 [ ok ]) (reg:VNx16BI 85 ffrt)) (set (reg:VNx16BI 85 ffrt) (unspec:VNx16BI [(reg:VNx16BI 85 ffrt)] UNSPEC_UPDATE_FFRT)) (set (reg:CC_NZC 66 cc) (unspec:CC_NZC [(reg:VNx16BI 106) repeated x2 (const_int 1 [0x1]) (reg/v:VNx16BI 102 [ ok ])] UNSPEC_PTEST)) FFR is the real first fault register. FFRT is actually a fake register whose only purpose is to describe the dependencies (in rtl) between writes to the FFR, reads from the FFR and first-faulting loads. The whole scheme depends on having only one fixed FFRT register. >> > How are dependencies represented in your new pass? If it just does >> > walks over the insn stream for everything, you get quadratic complexity >> > if you move insns backwards. We have that in combine already, mostly >> > from modified_between_p, but that is limited because of how LOG_LINKS >> > work, and we have been doing this for so long and there are no problems >> > found with it, so it must work in practice. But I am worried about it >> > when moving insns back an unlimited distance. >> >> It builds def-use chains, but using a constant limit on the number of >> explicitly-recorded uses. All other uses go in a numerical live range >> from which they (conservatively) never escape. The def-use chains >> represent memory as a single entity, a bit like in gimple. > > Ah. So that range thing ensures correctness. Yeah. > Why don't you use DF for the DU chains? The problem with DF_DU_CHAIN is that it's quadratic in the worst case. fwprop.c gets around that by using the MD problem and having its own dominator walker to calculate limited def-use chains: /* We use the multiple definitions problem to compute our restricted use-def chains. */ So taking that approach here would still require some amount of roll-your-own. Other reasons are: * Even what fwprop does is more elaborate than we need for now. * We need to handle memory too, and it's nice to be able to handle it in the same way as registers. * Updating a full, ordered def-use chain after a move is a linear-time operation, so whatever happens, we'd need to apply some kind of limit on the number of uses we maintain, with something like that integer point range for the rest. * Once we've analysed the insn and built its def-use chains, we don't look at the df_refs again until we update the chains after a successful combination. So it should be more efficient to maintain a small array of insn_info_rec pointers alongside the numerical range, rather than walk and pollute chains of df_refs and then link back the insn uids to the pass-local info. >> >> - it tries using REG_EQUAL notes for the final instruction. >> > >> > And that. >> >> I meant REG_EQUAL notes on i3, i.e. it tries replacing the src of i3 >> with i3's REG_EQUAL note and combining into that. Does combine do that? >> I couldn't see it, and in: >> >>https://gcc.gnu.org/ml/gcc/2019-06/msg00148.html >> >> you seemed to reject the idea of allowing it. > > Yes, I still do. Do you have an example where it helps? I'll run another set of tests for that. >> >> - it can parallelise two independent instructions that both read from >> >> the same register or both read from memory. >> > >> > That only if somehow there is a link between the two (so essentially >> > never). The only combinations tried by combine are those via LOG_LINKs, >> > which are between a SET and the first corresponding use. This is a key >> > factor that makes it kind of linear (instead of exponential) complexity. >> >> Tracking limited def-use chains is what makes this last bit easy. >> We can just try parallelising two instructions from the (bounded) list >> of uses. And for this case
Re: Reverting r278411
On Wed, 20 Nov 2019, Jakub Jelinek wrote: > On Wed, Nov 20, 2019 at 10:17:38AM -0600, Segher Boessenkool wrote: > > On Wed, Nov 20, 2019 at 03:46:29PM +, Richard Sandiford wrote: > > > Segher Boessenkool writes: > > > > UNLT & ORDERED is always LT. When would it not be true? > > > > > > LT traps on quiet NaNs for -ftrapping-math, UNLT and ORDERED don't. > > > > No? -ftrapping-math makes nothing trap. The only thing it does is to > > not do optimisations that are not valid if traps are considered to be > > a user-visible thing. > > > > Almost nothing ever traps on quiet NaNs. > > A lot traps even with quiet NaNs, assuming exceptions are enabled. > E.g. for x86 https://www.felixcloutier.com/x86/cmppd#tbl-3-1 lists the > details which operations rise FE_INVALID and which don't if it is enabled. The general IEEE rule is *operations with a non-floating-point result*. So comparisons (other than quiet ones) and conversions to integer types. (And the special case that it's unspecified whether fma (0, Inf, qNaN) traps or not.) -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH 5/X] [libsanitizer][mid-end] Introduce stack variable handling for HWASAN
On Wed, 20 Nov 2019, Matthew Malcomson wrote: > I don't have much of a plan. > > The most promising lead I have is that libiberty/alloca.c has a similar > functionality but with macros to account for a special case. The comment in libiberty/aclocal.m4 is: # We always want a C version of alloca() compiled into libiberty, # because native-compiler support for the real alloca is so !@#$% # unreliable that GCC has decided to use it only when being compiled # by GCC. This is the part of AC_FUNC_ALLOCA that calculates the # information alloca.c needs. This is the sort of thing that was relevant when GCC was built on lots of proprietary Unix systems with their system C compilers. Most of those proprietary Unix systems are long obsolete and are no longer supported by GCC. On the limited remaining set of host systems supported by GCC, there are a limited number of C++ compilers used to build most of the host code in GCC that is C++, and presumably a limited number of accompanying C compilers used to build libiberty. Now, libiberty is used by binutils more of which is written in C, but I doubt that expands the range of relevant host C compilers; the set of host OSes used nowadays is simply much smaller than it was when this code was written. So I'd suggest either completely eliminating C alloca from libiberty, or at least not building it at all when building with a compiler that defines __GNUC__. -- Joseph S. Myers jos...@codesourcery.com
[COMMITTED][AArch64] Fix vrbit_1.c test failure
The vrbit_1 test was missing a flag to disable code sharing.
Committed as obvious.
ChangeLog:
2019-11-20 Wilco Dijkstra
testsuite/
* gcc.target/aarch64/simd/vrbit_1.c: Add -fno-ipa-icf.
--
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/vrbit_1.c
b/gcc/testsuite/gcc.target/aarch64/simd/vrbit_1.c
index
ff19558cc6e0645ccd937bff25857cfbdb480714..c4ca4d140731943ba0028db651e45eccec5626f2
100644
--- a/gcc/testsuite/gcc.target/aarch64/simd/vrbit_1.c
+++ b/gcc/testsuite/gcc.target/aarch64/simd/vrbit_1.c
@@ -1,5 +1,5 @@
/* { dg-do run } */
-/* { dg-options "-O2 --save-temps -fno-inline" } */
+/* { dg-options "-O2 --save-temps -fno-inline -fno-ipa-icf" } */
#include
Re: [patch, fortran] Load scalar intent-in variables at the beginning of procedures
On 19 November 2019 23:54:55 CET, Thomas Koenig wrote: >Am 19.11.19 um 11:39 schrieb Bernhard Reutner-Fischer: >> + char name[GFC_MAX_SYMBOL_LEN + 1]; >> + snprintf (name, GFC_MAX_SYMBOL_LEN, "__dummy_%d_%s", var_num++, >> +f->sym->name); >> + >> + if (gfc_get_sym_tree (name, ns, , false) != 0) >> >> (I) you should + sizeof(__dummy__) + 10 for unsigned long %d or the >like. > >GFC_MAX_SYMBOL_LEN is the maximum length of a gfortran symbol. AFAIK, This is only true for user-provided names in the source code. Think e.g. class names as can be seen in the dumps.. >it >is not possible to use a longer symbol name than that, so it needs to >be >truncated. Uniqueness of the variable name is guaranteed by the var_num >variable. > >If the user puts dummy arguments Supercalifragilisticexpialidociousa >and >Supercalifragilisticexpialidociousb into the argument list of a >procedure, he will have to look at the numbers to differentiate them >:-) > >> (II) s/__dummy/__intent_in/ for clarity? > >It's moved away a bit from INTENT(IN) now, because an argument which >cannot be modified (even by passing to a procedure with a dummy >argument >with unknown intent) is now also handled. So maybe __readonly_ , __rdonly_, __rd_or the like? dummy is really misleading a name in the dumps.. thanks,
Re: [PATCH] Fix PR92462
On 11/19/19 12:42 AM, Richard Biener wrote:
> On Mon, 18 Nov 2019, Jeff Law wrote:
>
>> On 11/18/19 3:37 AM, Richard Biener wrote:
>>> On Mon, 18 Nov 2019, Jakub Jelinek wrote:
>>>
On Mon, Nov 18, 2019 at 11:07:22AM +0100, Richard Biener wrote:
> On Mon, 18 Nov 2019, Jakub Jelinek wrote:
>
>> On Mon, Nov 18, 2019 at 10:44:14AM +0100, Richard Biener wrote:
>>> The following adjusts the find_base_{term,value} heuristic when
>>> looking through ANDs to the case where it is obviously not
>>> aligning a base (when the LSB is set).
>>
>> What is specific about the LSB? I mean & 2 is obviously not an aligning
>> AND either.
>
> It aligns 0x1 and 0x3 ;)
>
>> Shouldn't we recurse only if the CONST_INT_P operand has
>> some set bits followed by clear bits, i.e. after the != 0 check
>> compute ctz_hwi and verify that INTVAL >> ctz == -1?
>
> I thought of more advanced heuristics but I guess that
> any contiguous set of bits with LSB unset might be aligning
> if the programmer knows upper bits are zero anyways? So I fear
> the == -1 test would not work reliable?
I'd say once a user does & 0x1ff8 or similar, he is doing something weird,
and not recursing is the conservatively correct answer (or maybe it isn't?
Aren't there cases where we punt if from a binary op we get base terms from
both sides and just use one if we get it only from one side? If so,
we might need to have some kind of annotated return, this could have a base
term but please don't use it).
>>>
>>> Yes, we might run into the "wrong" one via binary op handling, so
>>> there isn't really a conservative answer here :/
>>>
I guess the only non-weird case would be if the target has weird pointer
sizes and only has larger or smaller ints, say 24-bit pointer, and 8/16/32
integers, so the aligning then could be & 0xf8 etc.
>>>
>>> Yeah. Or the weird ones are exposed by nonzero bits "optimizations"
>>> of constants.
>> IIRC all the low order bitmask handling for pointers was primarily to
>> benefit the Alpha. I think over time we saw it was helpful for
>> varargs/stdarg, but that's about it. I'd be surprised if it's all that
>> important to dig deeply into addresses of this form.
>
> The whole find_base_{value,term} thing is most important for
> stack accesses which we otherwise can't handle well in aliasing
> and code effects mostly materialize in DSE. See my analysis
> in PR49330. But the code is really broken and we're clawing
> to the extra DSE in exchange for these wrong-code issues... :/
I'd rather fix the wrong-code issues :-)
Note that REG_POINTER should already be conservative, if it isn't, then
that needs to be fixed. Some backends certainly depend on anything with
REG_POINTER actually being a valid pointer.
For this RTX in c#18:
> (plus:DI (reg:DI 83 [ d.0_2 ])
> (symbol_ref:DI ("y") [flags 0x2] ))
I don't see how (reg 83) could ever be marked as REG_POINTER. It's an
index, not a pointer. I would _not_ expect the result (assuming its
stored in a REG) to be marked with REG_POINTER either since we don't
know if the addition of (reg 83) creates a result outside the object
(without additional information not in the BZ).
Jeff
[committed] jit: fix ICE with GCC_JIT_BOOL_OPTION_SELFCHECK_GC since r278084 (PR jit/92483)
Since r278084 (part of the params refactoring), most of libgccjit's
test suite has been ICEing.
The root cause is that jit-playback.c injects params to its fake_args
here:
/* Aggressively garbage-collect, to shake out bugs: */
if (get_bool_option (GCC_JIT_BOOL_OPTION_SELFCHECK_GC))
{
ADD_ARG ("--param");
ADD_ARG ("ggc-min-expand=0");
ADD_ARG ("--param");
ADD_ARG ("ggc-min-heapsize=0");
}
(building a vec of char * where the char * are allocated using xstrdup)
and r278084 added this logic to decode_cmdline_options_to_array:
964 /* Interpret "--param" "key=name" as "--param=key=name". */
965 const char *needle = "--param";
966 if (i + 1 < argc && strcmp (opt, needle) == 0)
967 {
968 const char *replacement
969 = opts_concat (needle, "=", argv[i + 1], NULL);
970 argv[++i] = replacement;
971 }
Note that at line 970 it manipulates the argv in-place, inserting a
new option allocated with opts_concat, which uses opts_obstack
(itself initialized from toplev::main).
jit-playback.c cleans up its fake arguments using "free", at which
point we have a free of the middle of an obstack and an ICE.
This patch fixes the issue by using the new syntax for the params.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Fixes all 60 FAILs in jit.sum, restoring the number of PASS results
from 2033 to 10469.
Committed to trunk as r278515.
gcc/jit/ChangeLog:
PR jit/92483
* jit-playback.c (gcc::jit::playback::context::make_fake_args):
Update GCC_JIT_BOOL_OPTION_SELFCHECK_GC for new --param syntax.
---
gcc/jit/jit-playback.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 9eeb2a7..c043d69 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -2273,10 +2273,8 @@ make_fake_args (vec *argvec,
/* Aggressively garbage-collect, to shake out bugs: */
if (get_bool_option (GCC_JIT_BOOL_OPTION_SELFCHECK_GC))
{
- ADD_ARG ("--param");
- ADD_ARG ("ggc-min-expand=0");
- ADD_ARG ("--param");
- ADD_ARG ("ggc-min-heapsize=0");
+ ADD_ARG ("--param=ggc-min-expand=0");
+ ADD_ARG ("--param=ggc-min-heapsize=0");
}
if (get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_EVERYTHING))
--
1.8.5.3
Re: [PATCH] Switch gcc ftp URL's to http
This patch is OK with http changed to https. (That is, with it changed where the patch is already changing the URL. While changing http to https makes sense more generally in the documentation whenever a site supports https, that's probably best not mixed with the move from ftp.) -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] include size and offset in -Wstringop-overflow
On 11/18/19 9:15 AM, Martin Sebor wrote:
> On 11/17/19 12:03 PM, Jeff Law wrote:
>> On 11/12/19 12:55 PM, Martin Sebor wrote:
>>> On 11/12/19 10:54 AM, Jeff Law wrote:
On 11/12/19 1:15 AM, Richard Biener wrote:
> On Tue, Nov 12, 2019 at 6:10 AM Jeff Law wrote:
>>
>> On 11/6/19 3:34 PM, Martin Sebor wrote:
>>> On 11/6/19 2:06 PM, Martin Sebor wrote:
On 11/6/19 1:39 PM, Jeff Law wrote:
> On 11/6/19 1:27 PM, Martin Sebor wrote:
>> On 11/6/19 11:55 AM, Jeff Law wrote:
>>> On 11/6/19 11:00 AM, Martin Sebor wrote:
The -Wstringop-overflow warnings for single-byte and multi-byte
stores mention the amount of data being stored and the
amount of
space remaining in the destination, such as:
warning: writing 4 bytes into a region of size 0
[-Wstringop-overflow=]
123 | *p = 0;
| ~~~^~~
note: destination object declared here
45 | char b[N];
| ^
A warning like this can take some time to analyze. First, the
size
of the destination isn't mentioned and may not be easy to tell
from
the sources. In the note above, when N's value is the
result of
some non-trivial computation, chasing it down may be a small
project
in and of itself. Second, it's also not clear why the region
size
is zero. It could be because the offset is exactly N, or
because
it's negative, or because it's in some range greater than N.
Mentioning both the size of the destination object and the
offset
makes the existing messages clearer, are will become essential
when
GCC starts diagnosing overflow into allocated buffers (as my
follow-on patch does).
The attached patch enhances -Wstringop-overflow to do this by
letting compute_objsize return the offset to its caller, doing
something similar in get_stridx, and adding a new function to
the strlen pass to issue this enhanced warning (eventually, I'd
like the function to replace the -Wstringop-overflow handler in
builtins.c). With the change, the note above might read
something
like:
note: at offset 11 to object ‘b’ with size 8 declared here
45 | char b[N];
| ^
Tested on x86_64-linux.
Martin
gcc-store-offset.diff
gcc/ChangeLog:
* builtins.c (compute_objsize): Add an argument and set
it to
offset
into destination.
* builtins.h (compute_objsize): Add an argument.
* tree-object-size.c (addr_object_size): Add an argument
and
set it
to offset into destination.
(compute_builtin_object_size): Same.
* tree-object-size.h (compute_builtin_object_size):
Add an
argument.
* tree-ssa-strlen.c (get_addr_stridx): Add an
argument and
set it
to offset into destination.
(maybe_warn_overflow): New function.
(handle_store): Call maybe_warn_overflow to issue
warnings.
gcc/testsuite/ChangeLog:
* c-c++-common/Wstringop-overflow-2.c: Adjust text of
expected
messages.
* g++.dg/warn/Wstringop-overflow-3.C: Same.
* gcc.dg/Wstringop-overflow-17.c: Same.
>>>
Index: gcc/tree-ssa-strlen.c
===
--- gcc/tree-ssa-strlen.c (revision 277886)
+++ gcc/tree-ssa-strlen.c (working copy)
@@ -189,6 +189,52 @@ struct laststmt_struct
static int get_stridx_plus_constant (strinfo *, unsigned
HOST_WIDE_INT, tree);
static void handle_builtin_stxncpy (built_in_function,
gimple_stmt_iterator *);
+/* Sets MINMAX to either the constant value or the
range VAL
is in
+ and returns true on success. */
+
+static bool
+get_range (tree val, wide_int minmax[2], const vr_values
*rvals =
NULL)
+{
Re: [PATCH v2] Add `--with-install-sysroot=' configuration option
On Wed, 20 Nov 2019, Maciej W. Rozycki wrote: > > But even then, if you configure GCC using "--with-sysroot" or > > "--with-build-sysroot", both of those paths are the top-level sysroot, to > > which the sysroot suffix gets appended before GCC uses it for any purpose, > > unless you explicitly build using --no-sysroot-suffix. So I still think > > it's natural for a user of GCC's configure scripts to expect the new > > option, like the other sysroot-related configure options, also to be one > > to which the per-multilib sysroot suffix gets appended before GCC uses it. > > And if it's not like that, the documentation needs to say so explicitly. > > Thanks for your concern, however again, AFAICT this change is tangential > to any sysroot suffix, which necessarily has to be already included in the > multilib OS directory as given by `-print-multi-os-directory', so that it > gets embedded within $toolexeclibdir for the purpose of target library > installation across the relevant subdirs, as per this excerpt from > `configure' code right after the assignments quoted in the example above: > > multi_os_directory=`$CC -print-multi-os-directory` > case $multi_os_directory in > .) ;; # Avoid trailing /. > *) toolexeclibdir=$toolexeclibdir/$multi_os_directory ;; > esac > > or otherwise the existing arrangement where > toolexeclibdir='$(toolexecdir)/lib' wouldn't have worked either (and > neither would in the native case where toolexeclibdir='$(libdir)'). > > Does this answer clear your concern? OK to apply with the documentation > thinko fixed? The answer explains the reasoning behind the design of the option (i.e., the design that means it's not particularly useful with sysroot suffixes, because the user would still need to relocate libraries manually to the correct suffixed sysroot). It is indeed the case that making a version useful with sysroot suffixes would not simply be a configuration change but involve changes in the compiler driver to disentangle two different uses of multilib OS directory suffixes. However, it's not enough to answer the question about the semantics of the option on the mailing list. The question is a natural one for anyone who knows about sysroot suffixes and is reading the documentation of the option. And so *the actual GCC documentation proposed to be committed*, not just explanations on the mailing list that people reading that documentation won't see, needs to say explicitly that the OS directory suffix is appended to lib/ in the *unsuffixed* sysroot, so that all libraries get installed in the same sysroot even if suffixes are in use. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH 1/3] [ARC] Fix failing pr77309 for ARC700
On 11/19/19 2:02 AM, Claudiu Zissulescu wrote: > The patterns neg_scc_insn and not_scc_insn are not correct, leading to > failing pr77309 test for ARC700. Add two new bic compare with zero > patterns to improve output code. > > gcc/ > -xx-xx Claudiu Zissulescu > > * config/arc/arc.md (bic_f): Use cc_set_register predicate. > (bic_cmp0_noout): New pattern. > (bic_cmp0): Likewise. > (neg_scc_insn): Remove pattern. > (not_scc_insn): Likewise. OK. FYI between e00e0f8c391779c8d6cc9ad2fff8056a73c765c2 (good) fcae029b424f0546ee0efe574dff150be41271ea (bad) I got the following regression. I haven't looked into it yet at all, but figured passing it along might be helpful. > # Comparing 4 common sum files > ## /bin/sh gcc/contrib/compare_tests /tmp/gxx-sum1.60617 /tmp/gxx-sum2.60617 > Tests that now fail, but worked before (1 tests): > > arc-sim: gcc.dg/pr86991.c (test for excess errors) Jeff
Re: v2 [PATCH 0/X] Introduce HWASAN sanitizer to GCC
On 20/11/2019 14:29, Martin Liška wrote: > On 11/7/19 7:37 PM, Matthew Malcomson wrote: >> I have rebased this series onto Martin Liska's patches that take the most >> recent libhwasan from upstream LLVM. >> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00340.html >> >> I've also cleared up some nomenclature (I had previously used the word >> 'colour' >> a few times instead of the word 'tag' and that clashes with other >> descriptions) >> and based the patch series off a more recent GCC revision (r277678). >> >> There's an ongoing discussion on whether to have >> __SANITIZER_ADDRESS__, or >> __SANITIZER_HWADDRESS__, but I'm keeping that discussion to the existing >> thread. >> >> Similarly there's still the question around C++ exceptions that I'm >> keeping to >> the existing thread (on the first patch series). >> >> >> NOTE: >> Unfortunately, there's a bug in the more recent version of GCC I >> rebased >> onto. >> Hwasan catches this when bootstrapping, which means bootstrapping >> with hwasan >> fails. >> I'm working on tracking the bug down now, but sending this series >> upstream >> for visibility while that happens. >> >> Bugzilla link: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92410 >> >> Entire patch series attached to cover letter.= >> > > Hello. > > Thank you very much for the patch set, I've just wrote some inline replies > and I have also some questions/suggestions that I'll summarize here. One > caveat, > I'm not maintainer in any of the ideas and I must confirm that I haven't > made > a deep review of the part which relates to new RTL hooks and stack > expansion. > I expect Jakub can help us here. > > Questions/notes: > a) For ASAN we do have bunch of parameters: > > gcc --help=param | grep asan > asan-stack Enable asan stack protection. > asan-instrument-allocas Enable asan allocas/VLAs protection. > asan-globals Enable asan globals protection. > asan-instrument-writes Enable asan store operations protection. > asan-instrument-reads Enable asan load operations protection. > asan-memintrin Enable asan builtin functions protection. > asan-use-after-return Enable asan detection of use-after-return > bugs. > asan-instrumentation-with-call-threshold Use callbacks instead of > inline code if number of accesses in function becomes greater or equal > to this number. > > We can probably use some of these in order to drive hwaddress in a > similar way. Most of them makes sense for hwaddress as well I would prefer to keep the options different but would be happy to share them if others want. I figure that there will be some parameters that make sense for hwasan but not for asan. For example hwasan-random-frame-tag doesn't have any analogue for asan. Re-using `asan-stack` for hwasan would mean we would then need to decide between having options with either `hwasan` or `asan` prefix being able to affect hwasan, or introducing a parameter `asan-random-frame-tag` that has no meaning on asan. > > b) Apparently clang prints also value of all registers. Do we want to do > the same? > I believe this only happens on clang for inline checking (try testing clang using the parameter `-mllvm --hwasan-instrument-with-calls` to see without). This would be nice to have, but I'm not planning on looking at it any time soon. This is currently implemented in clang on top of two not-yet implemented features: Inline instrumentation, and generated checking functions with special calling conventions. It's certainly not something I'm aiming to get into GCC 10. > > c) I'm a bit confused by the pointer tags, where clang does: > Yeah -- I left out a description of short-tags. Hopefully my explanation in the below email helped. https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01968.html > > d) Are you planning to come up with inline stack variable > tagging/untagging for GCC 10? To make sure I understand the question correctly: I think you're asking about writing tags into the shadow memory. I wasn't planning on this. What I've posted has all the functionality I'm aiming to get in. My stretch goal at the moment is to handle exceptions (where I currently have a fundamental problem I'm trying to resolve). > e) In hwasan_increment_tag, shouldn't you skip HWASAN_STACK_BACKGROUND > color? Hopefully answered in https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01968.html > f) I would rename ASAN_MARK in tree dumps to HWASAN_MARK, similarly to > what you did > for HWASAN_CHECK? This is an artifact of a shortcut I made (and tried but probably failed to explain well in https://gcc.gnu.org/ml/gcc-patches/2019-09/msg00392.html). For HWASAN_CHECK I introduced a new internal function that takes the same arguments as ASAN_CHECK, hence this new function is printed differently. For HWASAN_MARK, I used a trick to avoid adding "almost duplicate" code everywhere in
Re: [PATCH] Update comment in libsanitizer/*/libtool-version files.
On 11/20/19 4:02 AM, Martin Liška wrote: > Hi. > > The patch is about removal of an unused libtool-version file, > and comments in other libtool-version files are legacy. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > I'm going to install the patch if there are no objections. > > Thanks, > Martin > > libsanitizer/ChangeLog: > > 2019-11-20 Martin Liska > > * libtool-version: Remove. > * lsan/libtool-version: Upate comment to not mention libmudflap. > * tsan/libtool-version: Likewise. > * ubsan/libtool-version: Likewise. OK jeff
Re: Adjust expected output for bb-slp-21.c (PR 92527)
On 11/20/19 5:46 AM, Richard Sandiford wrote: > After r278246, we can try building the out[] store value from scalars > if the target has no multiplication support. That's not necessarily > a good thing, but like most of vect/, this test is run with the cost > model disabled. > > Tested on aarch64-linux-gnu, x86_64-linux-gnu, powerpc64-linux-gnu > (Power 7) and sparc-sun-solaris2.11. OK to install? > > Richard > > > 2019-11-20 Richard Sandiford > > gcc/testsuite/ > PR testsuite/92527 > * gcc.dg/vect/bb-slp-21.c: Expect both SLP groups to be vectorized, > regardless of whether the target supports multiplication. OK. Jeff
Re: Restrict bb-slp-40.c to targets with VnQI addition (PR 92366)
On 11/20/19 8:15 AM, Richard Sandiford wrote: > bb-slp-40.c fails on SPARC targets without VIS4 because it > requires addition on vectors of bytes. There doesn't seem to be > an existing target selector for this, so I added vect_char_add. > (Wasn't sure whether to use vect_char_add, for consistency > with vect_no_int_add/vect_int_mult etc., or vect_add_char for > consistency with vect_shift_char etc.) > > I took the target list from vect_int and removed targets that didn't > seem to support the operation (namely sparc*, since we don't seem to > have any test for VIS4, niagara7 or m8, and alpha*-*-*.) > > Tested on aarch64-linux-gnu, x86_64-linux-gnu, powerpc64-linux-gnu > (Power 7) and sparc-sun-solaris2.11. OK to install? > > Richard > > > 2019-11-20 Richard Sandiford > > gcc/ > PR testsuite/92366 > * doc/sourcebuild.texi (vect_char_add): Document. > > gcc/testsuite/ > PR testsuite/92366 > * lib/target-supports.exp (check_effective_target_vect_char_add): > New proc. > * gcc.dg/vect/bb-slp-40.c: Require vect_char_add instead of vect_int. OK jeff
Re: Reverting r278411
On Wed, Nov 20, 2019 at 04:59:49PM +, Richard Sandiford wrote: > Segher Boessenkool writes: > > On Wed, Nov 20, 2019 at 05:30:48PM +0100, Jakub Jelinek wrote: > >> On Wed, Nov 20, 2019 at 10:17:38AM -0600, Segher Boessenkool wrote: > >> > On Wed, Nov 20, 2019 at 03:46:29PM +, Richard Sandiford wrote: > >> > > Segher Boessenkool writes: > >> > > > UNLT & ORDERED is always LT. When would it not be true? > >> > > > >> > > LT traps on quiet NaNs for -ftrapping-math, UNLT and ORDERED don't. > >> > > >> > No? -ftrapping-math makes nothing trap. The only thing it does is to > >> > not do optimisations that are not valid if traps are considered to be > >> > a user-visible thing. > >> > > >> > Almost nothing ever traps on quiet NaNs. > >> > >> A lot traps even with quiet NaNs, assuming exceptions are enabled. > >> E.g. for x86 https://www.felixcloutier.com/x86/cmppd#tbl-3-1 lists the > >> details which operations rise FE_INVALID and which don't if it is enabled. > > > > That is what ordered comparisons (aka signaling comparisons) do, sure. > > This is part of "almost nothing" in my count ;-) > > > > Ordered comparisons should trap both with and without -ftrapping-math. > > The difference is that with -fno-trapping math GCC can ignore that and > > just optimise code how it wants to. > > Sure, no-one was disputing that. I think you're arguing against > a strawman here. No, I don't understand the question I guess? Everything in that table that traps on quiet NaNs is a signaling comparison. We should simply not do simplify_logical_relational_operation for floating point types and signaling comparisons. There is no "only for some codes", etc. Segher
Re: Reverting r278411
On Wed, Nov 20, 2019 at 04:35:24PM +, Richard Sandiford wrote:
> Actually, this doesn't work because *_operators want rtxes rather
> than codes. I can get around that by passing op0 and op1 for
> the existing rtxes. For the conversion at the end, I can do:
>
> machine_mode compared_mode = GET_MODE (XEXP (op0, 0));
>
> if (code == ORDERED && INTEGRAL_MODE_P (compared_mode))
> return const_true_rtx;
This should be all !HONOR_NANS? Also LTGT should be turned into NE,
under that same condition. So something like
if (!HONOR_NANS (mode))
{
/* UNORDERED cannot happen without NaNs. */
mask &= ~1;
/* LTGT is written as NE, and ORDERED just is always true,
without NaNs. */
if (mask == 12 || mask == 14)
mask |= 1;
}
before returning true for 15.
> if (is_unsigned)
> code = unsigned_condition (code);
>
> Or I can add signed_comparison_p and unsigned_comparison_p functions
> that take codes instead of rtxes.
That may be easier, dunno.
Thanks,
Segher
Re: Reverting r278411
Segher Boessenkool writes: > On Wed, Nov 20, 2019 at 05:30:48PM +0100, Jakub Jelinek wrote: >> On Wed, Nov 20, 2019 at 10:17:38AM -0600, Segher Boessenkool wrote: >> > On Wed, Nov 20, 2019 at 03:46:29PM +, Richard Sandiford wrote: >> > > Segher Boessenkool writes: >> > > > UNLT & ORDERED is always LT. When would it not be true? >> > > >> > > LT traps on quiet NaNs for -ftrapping-math, UNLT and ORDERED don't. >> > >> > No? -ftrapping-math makes nothing trap. The only thing it does is to >> > not do optimisations that are not valid if traps are considered to be >> > a user-visible thing. >> > >> > Almost nothing ever traps on quiet NaNs. >> >> A lot traps even with quiet NaNs, assuming exceptions are enabled. >> E.g. for x86 https://www.felixcloutier.com/x86/cmppd#tbl-3-1 lists the >> details which operations rise FE_INVALID and which don't if it is enabled. > > That is what ordered comparisons (aka signaling comparisons) do, sure. > This is part of "almost nothing" in my count ;-) > > Ordered comparisons should trap both with and without -ftrapping-math. > The difference is that with -fno-trapping math GCC can ignore that and > just optimise code how it wants to. Sure, no-one was disputing that. I think you're arguing against a strawman here. Richard
Re: [PATCH 0/4] Eliminate cc0 from m68k
On 11/20/19 7:16 AM, Bernd Schmidt wrote: > On 11/20/19 2:50 PM, Mikael Pettersson wrote: >> On Mon, Nov 18, 2019 at 9:57 PM Mikael Pettersson >> wrote: >>> >>> On Mon, Nov 18, 2019 at 8:31 PM Bernd Schmidt >>> wrote: Hi Mikael, > This fixed the problem, thanks. Could you also run the testsuite to see if you can reproduce the g++.old-deja failures Andreas reported? >>> >>> Yes, but it will probably take another week before the native >>> bootstrap (on aranym) and test suite run is finished. It's currently >>> in stage 2. > > Ugh, that suggests the stage2 compiler was miscompiled. That would be > nasty to track down. > > Probably best to just run tests on stage1 and hope something shows up. > > What distro are you using for native builds? The m68k debian I'm using > does not have an installable gcc package. Definitely agreed, best place to start is with teh stage1 testresults and see if we can nail it down that way. Debugging user mode qemu bits is *painful*. For that we may ultimately be better off with Aranym. jeff
Re: Reverting r278411
On Wed, Nov 20, 2019 at 05:30:48PM +0100, Jakub Jelinek wrote: > On Wed, Nov 20, 2019 at 10:17:38AM -0600, Segher Boessenkool wrote: > > On Wed, Nov 20, 2019 at 03:46:29PM +, Richard Sandiford wrote: > > > Segher Boessenkool writes: > > > > UNLT & ORDERED is always LT. When would it not be true? > > > > > > LT traps on quiet NaNs for -ftrapping-math, UNLT and ORDERED don't. > > > > No? -ftrapping-math makes nothing trap. The only thing it does is to > > not do optimisations that are not valid if traps are considered to be > > a user-visible thing. > > > > Almost nothing ever traps on quiet NaNs. > > A lot traps even with quiet NaNs, assuming exceptions are enabled. > E.g. for x86 https://www.felixcloutier.com/x86/cmppd#tbl-3-1 lists the > details which operations rise FE_INVALID and which don't if it is enabled. That is what ordered comparisons (aka signaling comparisons) do, sure. This is part of "almost nothing" in my count ;-) Ordered comparisons should trap both with and without -ftrapping-math. The difference is that with -fno-trapping math GCC can ignore that and just optimise code how it wants to. Segher
Fix (most of) nonlinearity in update_callee_keys
Hi,
this patch makes inliner to update callee keys only in function within
newly inlined clone rather than in the whole function it is inlined to.
This is possible only when the remaining edges are not becoming more hot
for inlining and on this I rely on monotonocity of the badness function:
if caller gets bigger and slower the call is not becoming hotter.
This is not true when wrapper penalty is applied and thus a special case
is needed.
Bootstrapped/regtested x86_64-linux, comitted.
Honza
* ipa-inline.c (wrapper_heuristics_may_apply): Break out from ...
(edge_badness): ... here.
(inline_small_functions): Use monotonicity of badness calculation
to avoid redundant updates.
Index: ipa-inline.c
===
--- ipa-inline.c(revision 278459)
+++ ipa-inline.c(working copy)
@@ -1097,6 +1097,17 @@ want_inline_function_to_all_callers_p (s
return true;
}
+/* Return true if WHERE of SIZE is a possible candidate for wrapper heuristics
+ in estimate_edge_badness. */
+
+static bool
+wrapper_heuristics_may_apply (struct cgraph_node *where, int size)
+{
+ return size < (DECL_DECLARED_INLINE_P (where->decl)
+? inline_insns_single (where, false)
+: inline_insns_auto (where, false));
+}
+
/* A cost model driving the inlining heuristics in a way so the edges with
smallest badness are inlined first. After each inlining is performed
the costs of all caller edges of nodes affected are recomputed so the
@@ -1227,10 +1238,8 @@ edge_badness (struct cgraph_edge *edge,
and it is not called once. */
if (!caller_info->single_caller && overall_growth < caller_growth
&& caller_info->inlinable
- && ipa_size_summaries->get (caller)->size
-< (DECL_DECLARED_INLINE_P (caller->decl)
- ? inline_insns_single (caller, false)
- : inline_insns_auto (caller, false)))
+ && wrapper_heuristics_may_apply
+(caller, ipa_size_summaries->get (caller)->size))
{
if (dump)
fprintf (dump_file,
@@ -2158,11 +2167,24 @@ inline_small_functions (void)
fprintf (dump_file, " Peeling recursion with depth %i\n", depth);
gcc_checking_assert (!callee->inlined_to);
+
+ int old_size = ipa_size_summaries->get (where)->size;
+ sreal old_time = ipa_fn_summaries->get (where)->time;
+
inline_call (edge, true, _indirect_edges, _size, true);
reset_edge_caches (edge->callee);
add_new_edges_to_heap (_heap, new_indirect_edges);
- update_callee_keys (_heap, where, updated_nodes);
+ /* If caller's size and time increased we do not need to update
+all edges becuase badness is not going to decrease. */
+ if (old_size <= ipa_size_summaries->get (where)->size
+ && old_time <= ipa_fn_summaries->get (where)->time
+ /* Wrapper penalty may be non-monotonous in this respect.
+Fortunately it only affects small functions. */
+ && !wrapper_heuristics_may_apply (where, old_size))
+ update_callee_keys (_heap, edge->callee, updated_nodes);
+ else
+ update_callee_keys (_heap, where, updated_nodes);
}
where = edge->caller;
if (where->inlined_to)
[C++ Patch] Avoid redundant error messages from build_x_arrow
Hi,
while working on improving the locations of cp_build_indirect_ref_1 &
co, I noticed this nit which seems a separate issue.
In a nutshell, at variance with many other cases, in build_x_arrow we
don't immediately check for error_mark_node the return value of
decay_conversion. Then, for the testcase, after a sensible:
error: invalid use of member function ‘C* C::f()’ (did you forget the
‘()’ ?)
we also issue:
error: base operand of ‘->’ is not a pointer
which is certainly redundant and a bit misleading, is talking about the
'f' mentioned in the first message. The amended behavior is also
consistent with EDG and CLANG.
Tested x86_64-linux, as usual.
Thanks, Paolo.
//
/gcc
2019-11-20 Paolo Carlini
* typeck2.c (build_x_arrow): Early return if decay_conversion
returns error_mark_node.
/testsuite
2019-11-20 Paolo Carlini
* g++.dg/parse/error43.C: Adjust expected error.
Index: cp/typeck2.c
===
--- cp/typeck2.c(revision 278499)
+++ cp/typeck2.c(working copy)
@@ -2044,7 +2044,11 @@ build_x_arrow (location_t loc, tree expr, tsubst_f
last_rval = convert_from_reference (last_rval);
}
else
-last_rval = decay_conversion (expr, complain);
+{
+ last_rval = decay_conversion (expr, complain);
+ if (last_rval == error_mark_node)
+ return error_mark_node;
+}
if (TYPE_PTR_P (TREE_TYPE (last_rval)))
{
Index: testsuite/g++.dg/parse/error43.C
===
--- testsuite/g++.dg/parse/error43.C(revision 278499)
+++ testsuite/g++.dg/parse/error43.C(working copy)
@@ -2,4 +2,4 @@
// { dg-options "" }
class C { public: C* f(); int get(); };
-int f(C* p) { return p->f->get(); } // { dg-error "forget the '\\(\\)'|base
operand" }
+int f(C* p) { return p->f->get(); } // { dg-error "25:invalid use of member
function" }
Document -Wc11-c2x-compat
My patch that added initial C2X support and associated command-line
options missed documenting -Wc11-c2x-compat although the other options
were properly documented. This patch adds the missing documentation.
Tested with "make info" and "make pdf". Applied to mainline. Will apply
to GCC 9 branch after reducing the list of features covered to reflect the
more limited C2X support in GCC 9.
2019-11-20 Joseph Myers
* doc/invoke.texi (-Wc11-c2x-compat): Document.
Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 278507)
+++ gcc/doc/invoke.texi (working copy)
@@ -295,6 +295,7 @@
-Wbool-compare -Wbool-operation @gol
-Wno-builtin-declaration-mismatch @gol
-Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol
+-Wc11-c2x-compat @gol
-Wc++-compat -Wc++11-compat -Wc++14-compat -Wc++17-compat @gol
-Wc++20-compat @gol
-Wcast-align -Wcast-align=strict -Wcast-function-type -Wcast-qual @gol
@@ -6830,6 +6831,16 @@
and so on. This option is independent of the standards mode. Warnings are
disabled in the expression that follows @code{__extension__}.
+@item -Wc11-c2x-compat @r{(C and Objective-C only)}
+@opindex Wc11-c2x-compat
+@opindex Wno-c11-c2x-compat
+Warn about features not present in ISO C11, but present in ISO C2X.
+For instance, warn about omitting the string in @code{_Static_assert},
+use of @samp{[[]]} syntax for attributes, use of decimal
+floating-point types, and so on. This option is independent of the
+standards mode. Warnings are disabled in the expression that follows
+@code{__extension__}.
+
@item -Wc++-compat @r{(C and Objective-C only)}
@opindex Wc++-compat
@opindex Wno-c++-compat
--
Joseph S. Myers
jos...@codesourcery.com
Re: RFA; patch to fix PR90007
On Wed, 20 Nov 2019, Alexander Monakov wrote: > On Wed, 20 Nov 2019, Richard Biener wrote: > > > On Tue, Nov 19, 2019 at 5:07 PM Vladimir Makarov > > wrote: > > > > > > The following patch fixes > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90007 > > > > > > Sometime ago a code which permits LRA to reload hard register into > > > memory as it did for pseudo were added. But this LRA possibility was > > > not reflected in recog.c. The following patch fixes this discrepancy > > > and as a result fixes PR90007. > > > > > > OK to commit? > > > > I guess the change is OK but for the bug itself it sounds like > > selective scheduling doesn't properly recognize insns it > > proagates into (and avoids doing that then)? That is, > > selective scheduling creates invalid RTL? > > We validate the substitution by invoking validate_replace_rtx_part_nosimplify > from substitute_reg_in_expr. I think that should be sufficient? I see > similar > code in haifa-sched uses attempt_change, which also ultimately uses > apply_change_group. Although looking at this more, I see that we specifically arrange for a call to constrain_operands in replace_src_with_reg_ok_p (with a comment), but here in substitute_reg_in_expr we have a '???' comment that seems to mention that theoretically there might be a problem, but it never came up in practice. So there's an inconsistency in sel-sched as well. Alexander
Re: Reverting r278411
Richard Sandiford writes: >>> I'd actually considered converting to signed and back instead of adding >>> extra cases, but I thought that would be rejected as too inefficient. >>> (That was a concern with my patch above.) It seemed like one of the selling >>> points of doing it your way was that everything was handled by one switch >>> statement "in" and one switch statement "out", and I was trying to >>> preserve that. >>> >>> signed_condition and unsigned_condition assert on unordered comparisons, >>> so if we're going to go that route, we either need to filter them out >>> first or add maybe_* versions of the routines that return UNKNOWN. >> >> Yeah. rs6000 has >> >> (define_predicate "unsigned_comparison_operator" >> (match_code "ltu,gtu,leu,geu")) >> (define_predicate "signed_comparison_operator" >> (match_code "lt,gt,le,ge")) >> >> Maybe we should have those be for every target? >> >> bool is_signed = (signed_comparison_operator (code0) >> || signed_comparison_operator (code1)); >> bool is_unsigned = (unsigned_comparison_operator (code0) >> || unsigned_comparison_operator (code1)); >> >> /* Don't allow mixing signed and unsigned comparisons. */ >> if (is_signed && is_unsigned) >> return 0; >> >> (this takes care of your EQ/NE concern automatically, btw) >> >> if (unsigned_comparison_operator (code0)) >> code0 = signed_condition (code0); >> if (unsigned_comparison_operator (code1)) >> code1 = signed_condition (code1); >> >> and at the end >> >> if (is_unsigned && signed_comparison_operator (code)) >> code = unsigned_condition (code); > > OK, thanks, I'll do it this way. Actually, this doesn't work because *_operators want rtxes rather than codes. I can get around that by passing op0 and op1 for the existing rtxes. For the conversion at the end, I can do: machine_mode compared_mode = GET_MODE (XEXP (op0, 0)); if (code == ORDERED && INTEGRAL_MODE_P (compared_mode)) return const_true_rtx; if (is_unsigned) code = unsigned_condition (code); Or I can add signed_comparison_p and unsigned_comparison_p functions that take codes instead of rtxes. Do either of those sound OK, or would you prefer something else? Richard
Re: Reverting r278411
On Wed, Nov 20, 2019 at 10:17:38AM -0600, Segher Boessenkool wrote: > On Wed, Nov 20, 2019 at 03:46:29PM +, Richard Sandiford wrote: > > Segher Boessenkool writes: > > > UNLT & ORDERED is always LT. When would it not be true? > > > > LT traps on quiet NaNs for -ftrapping-math, UNLT and ORDERED don't. > > No? -ftrapping-math makes nothing trap. The only thing it does is to > not do optimisations that are not valid if traps are considered to be > a user-visible thing. > > Almost nothing ever traps on quiet NaNs. A lot traps even with quiet NaNs, assuming exceptions are enabled. E.g. for x86 https://www.felixcloutier.com/x86/cmppd#tbl-3-1 lists the details which operations rise FE_INVALID and which don't if it is enabled. Jakub
Optimize allocations for evaluate_properties_for_edges
Hi,
this patch avoids malloc traffic in evaluate_properties_for_edge which
allocates vectors holding known values, aggregates and polyorphic contextes.
I made the vector allocated by a caller who can place them at stack using
auto_vec.
The patch also adds logic to avoid calculation and clearing of these vectors
for parameters which are not used.
I realize that API for evaluate_properties_for_edge became somewhat ugly
with adding a lot of additional stuff. I will clean it up next stage1.
With this patch we still do a lot of allocations to hold
ipa_agg_value_set::items which are vectors within vectors. I wonder how much
of this work would go away if we would further track what parameters are being
used as aggregates?
Bootstrapped/regtested x86_64-linux, will commit it shortly.
Honza
* ipa-fnsummary.c (evaluate_conditions_for_known_args): Be
ready for some vectors to not be allocated.
(evaluate_properties_for_edge): Document better; make
known_vals and known_aggs caller allocated; avoid determining
values of parameters which are not used.
(ipa_merge_fn_summary_after_inlining): Pre allocate known_vals and
known_aggs.
* ipa-inline-analysis.c (do_estimate_edge_time): Likewise.
(do_estimate_edge_size): Likewise.
(do_estimate_edge_hints): Likewise.
Index: ipa-fnsummary.c
===
--- ipa-fnsummary.c (revision 278464)
+++ ipa-fnsummary.c (working copy)
@@ -329,7 +329,7 @@ evaluate_conditions_for_known_args (stru
for (i = 0; vec_safe_iterate (info->conds, i, ); i++)
{
- tree val;
+ tree val = NULL;
tree res;
int j;
struct expr_eval_op *op;
@@ -338,14 +338,8 @@ evaluate_conditions_for_known_args (stru
(especially for K style programs). So bound check here (we assume
known_aggs vector, if non-NULL, has the same length as
known_vals). */
- gcc_checking_assert (!known_aggs.exists ()
+ gcc_checking_assert (!known_aggs.length () || !known_vals.length ()
|| (known_vals.length () == known_aggs.length ()));
- if (c->operand_num >= (int) known_vals.length ())
- {
- clause |= 1 << (i + predicate::first_dynamic_condition);
- nonspec_clause |= 1 << (i + predicate::first_dynamic_condition);
- continue;
- }
if (c->agg_contents)
{
@@ -353,19 +347,24 @@ evaluate_conditions_for_known_args (stru
if (c->code == predicate::changed
&& !c->by_ref
+ && c->operand_num < (int)known_vals.length ()
&& (known_vals[c->operand_num] == error_mark_node))
continue;
- if (known_aggs.exists ())
+ if (c->operand_num < (int)known_aggs.length ())
{
agg = _aggs[c->operand_num];
- val = ipa_find_agg_cst_for_param (agg, known_vals[c->operand_num],
+ val = ipa_find_agg_cst_for_param (agg,
+ c->operand_num
+ < (int) known_vals.length ()
+ ? known_vals[c->operand_num]
+ : NULL,
c->offset, c->by_ref);
}
else
val = NULL_TREE;
}
- else
+ else if (c->operand_num < (int) known_vals.length ())
{
val = known_vals[c->operand_num];
if (val == error_mark_node && c->code != predicate::changed)
@@ -491,7 +490,18 @@ evaluate_conditions_for_known_args (stru
}
-/* Work out what conditions might be true at invocation of E. */
+/* Work out what conditions might be true at invocation of E.
+ Compute costs for inlined edge if INLINE_P is true.
+
+ Return in CLAUSE_PTR the evaluated condistions and in NONSPEC_CLAUSE_PTR
+ (if non-NULL) conditions evaluated for nonspecialized clone called
+ in a given context.
+
+ KNOWN_VALS_PTR and KNOWN_AGGS_PTR must be non-NULL and will be filled by
+ known canstant and aggregate values of parameters.
+
+ KNOWN_CONTEXT_PTR, if non-NULL, will be filled by polymorphic call contexts
+ of parameter used by a polymorphic call. */
void
evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
@@ -504,113 +514,141 @@ evaluate_properties_for_edge (struct cgr
{
struct cgraph_node *callee = e->callee->ultimate_alias_target ();
class ipa_fn_summary *info = ipa_fn_summaries->get (callee);
- vec known_vals = vNULL;
auto_vec known_value_ranges;
- vec known_aggs = vNULL;
class ipa_edge_args *args;
if (clause_ptr)
*clause_ptr = inline_p ? 0 : 1 << predicate::not_inlined_condition;
- if (known_vals_ptr)
-known_vals_ptr->create (0);
- if (known_contexts_ptr)
-known_contexts_ptr->create (0);
if (ipa_node_params_sum
&&
Re: Reverting r278411
On Wed, Nov 20, 2019 at 03:46:29PM +, Richard Sandiford wrote: > Segher Boessenkool writes: > > UNLT & ORDERED is always LT. When would it not be true? > > LT traps on quiet NaNs for -ftrapping-math, UNLT and ORDERED don't. No? -ftrapping-math makes nothing trap. The only thing it does is to not do optimisations that are not valid if traps are considered to be a user-visible thing. Almost nothing ever traps on quiet NaNs. And indeed the existing code is wrong for this already, as I mentioned before (in the context of IEEE SNaNs). But yeah, we can do almost no optimisation if trapping math is true, so we should skip everything in that case. I wonder how much of existing GCC code breaks this as well though, hrm. Segher
Fix nonlinearity in estimate_edge_growth
Hi,
this patch treads ages old problem that compile time needed to estimate call
size is not constant, but a function of a number of calls of the callee.
If there is a function with many callers and many callees this triggers
quadratic behaviour.
This patch adds summary info for all callees of a node which is of same format
as size_time_table used to account normal code. It is a different table since,
unlike normal code, call statemetns are changing thorough the IPA ooptimization
queue. In a followup patch I will add incremental update of this summary which
will solve the second traditional quadratic bottleneck of greedy inliner
(with inlinining very many calls into large function).
This has became problem for compiling cc1 itself because with enablement of
-finline-functions at -O2 we end up inlining a lot into the auto-generated
insn-* pattern matchers (since a lot of predicates are small).
Thus WPA inlining time regressed from 6s in gcc 9 to 65s in trunk two weeks
back.
The memory use of new tables is 64bytes per function summary and at most one
entry in the allocated vector per call (whole point of the change is to glob
calls with same predicates together and also cap number of predicates we care
about). Overal 10MB for cc1 (out of 900MB we need at WPA time).
It would be possible to merge both size_time_table and call_size_time_table to
one saving some of overhead. This would however make it impossible to
recalculate the data and do some sanity checking and I am affraid of making
that very hard to maintain
The profile of WPA cc1 shows:
- 47.85% 0.30% lto1-wpa lto1 [.]
inline_small_functions
- 47.55% inline_small_functions
- 24.20% update_callee_keys
- 6.95% can_inline_edge_p
1.32% sanitize_flags_p
+ 1.32% is_tm_pure
- 4.34% update_edge_key
3.48% edge_badness
4.24% want_inline_small_function_p
- 3.51% can_inline_edge_by_limits_p
0.61% estimate_size_after_inlining
0.89% cgraph_node::get_availability
- 13.44% inline_call
+ 10.57% ipa_update_overall_fn_summary
+ 1.23% clone_inlined_nodes
1.06% ipa_merge_fn_summary_after_inlining
- 5.27% update_caller_keys
+ 4.46% want_inline_small_function_p
0.62% can_inline_edge_p
1.34% fibonacci_heap::extract_minimum_node
+ 0.83% want_inline_small_function_p
+ 0.73% estimate_growth
ipa_update_overall_fn_summary is the nonlinearity of updating summaries after
inlining (each inline update is function of size of the inline tree of caller).
Honza
* ipa-fnsummary.c (ipa_fn_summary::account_size_time): Add CALL
parameter and update call_size_time_table.
(ipa_fn_summary::max_size_time_table_size): New constant.
(estimate_calls_size_and_time_1): Break out from ...
(estimate_calls_size_and_time): ... here; implement summary production.
(summarize_calls_size_and_time): New function.
(ipa_call_context::estimate_size_and_time): Bypass
estimate_calls_size_and_time for leaf functions.
(ipa_update_overall_fn_summary): Likewise.
* ipa-fnsummary.h (call_size_time_table): New.
(ipa_fn_summary::account_size_time): Update prototype.
Index: ipa-fnsummary.c
===
--- ipa-fnsummary.c (revision 278496)
+++ ipa-fnsummary.c (working copy)
@@ -146,17 +147,22 @@ ipa_dump_hints (FILE *f, ipa_hints hints
/* Record SIZE and TIME to SUMMARY.
The accounted code will be executed when EXEC_PRED is true.
When NONCONST_PRED is false the code will evaulate to constant and
- will get optimized out in specialized clones of the function. */
+ will get optimized out in specialized clones of the function.
+ If CALL is true account to call_size_time_table rather than
+ size_time_table. */
void
ipa_fn_summary::account_size_time (int size, sreal time,
const predicate _pred,
- const predicate _pred_in)
+ const predicate _pred_in,
+ bool call)
{
size_time_entry *e;
bool found = false;
int i;
predicate nonconst_pred;
+ vec *table = call
+ ? call_size_time_table : size_time_table;
if (exec_pred == false)
return;
@@ -168,23 +174,23 @@ ipa_fn_summary::account_size_time (int s
/* We need to create initial empty unconitional clause, but otherwie
we don't need to account empty times and sizes. */
- if (!size && time == 0 && size_time_table)
+ if (!size && time == 0 && table)
return;
gcc_assert (time >= 0);
- for (i = 0; vec_safe_iterate (size_time_table, i, ); i++)
+ for (i = 0; vec_safe_iterate (table, i, ); i++)
if (e->exec_predicate == exec_pred
&& e->nonconst_predicate ==
[PATCH] doc: Correct `--enable-version-specific-runtime-libs' support information
The `--enable-version-specific-runtime-libs' configuration option is now
supported throughout all of our target library subdirectories, so update
installation documentation accordingly and also mention that the default
for the option is `yes' for libada and `no' for the remaining libraries.
gcc/
* doc/install.texi (Options specification): Remove the list of
target library subdirectories supporting
`--enable-version-specific-runtime-libs'. Document defaults for
the option.
---
gcc/doc/install.texi |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
gcc-doc-version-specific-runtime-libs.diff
Index: gcc/gcc/doc/install.texi
===
--- gcc.orig/gcc/doc/install.texi
+++ gcc/gcc/doc/install.texi
@@ -1560,8 +1560,8 @@ addition, @samp{libstdc++}'s include fil
@file{@var{libdir}} unless you overruled it by using
@option{--with-gxx-include-dir=@var{dirname}}. Using this option is
particularly useful if you intend to use several versions of GCC in
-parallel. This is currently supported by @samp{libgfortran},
-@samp{libstdc++}, and @samp{libobjc}.
+parallel. The default is @samp{yes} for @samp{libada}, and @samp{no} for
+the remaining libraries.
@item @anchor{WithAixSoname}--with-aix-soname=@samp{aix}, @samp{svr4} or
@samp{both}
Traditional AIX shared library versioning (versioned @code{Shared Object}
Re: [PATCH 2/2] PR 92463 MPFR modernization: Revert r269139
LGTM.
Thanks,
Tobias
PS: For reference, mpfr_regular_p was added in MPFR 3.0.0 (as stated);
acting as follows:
mpfr_number_p = returns nonzero if ordinary number (i.e., neither NaN
nor an infinity),
mpfr_regular_p = returns nonzero if regular number (i.e., neither NaN,
nor an infinity nor zero)
On 11/18/19 9:34 PM, Janne Blomqvist wrote:
Commit r269139 fixed an accidental dependency on MPFR 3.0. As we now
require at least MPFR 3.1.0+ we can revert it and instead use the
simpler MPFR 3.0+ code.
ChangeLog entry of the original commit was:
2019-02-23 David Malcolm
Jakub Jelinek
PR middle-end/88074
* simplify.c (norm2_do_sqrt, gfc_simplify_norm2): Use
mpfr_number_p && !mpfr_zero_p instead of mpfr_regular_p.
(norm2_add_squared): Likewise. Use mp_exp_t rather than mpfr_exp_t.
---
gcc/fortran/simplify.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c
index a5c940ca2d5..b48bf014121 100644
--- a/gcc/fortran/simplify.c
+++ b/gcc/fortran/simplify.c
@@ -6023,8 +6023,8 @@ norm2_add_squared (gfc_expr *result, gfc_expr *e)
gfc_set_model_kind (result->ts.kind);
int index = gfc_validate_kind (BT_REAL, result->ts.kind, false);
- mp_exp_t exp;
- if (mpfr_number_p (result->value.real) && !mpfr_zero_p (result->value.real))
+ mpfr_exp_t exp;
+ if (mpfr_regular_p (result->value.real))
{
exp = mpfr_get_exp (result->value.real);
/* If result is getting close to overflowing, scale down. */
@@ -6038,7 +6038,7 @@ norm2_add_squared (gfc_expr *result, gfc_expr *e)
}
mpfr_init (tmp);
- if (mpfr_number_p (e->value.real) && !mpfr_zero_p (e->value.real))
+ if (mpfr_regular_p (e->value.real))
{
exp = mpfr_get_exp (e->value.real);
/* If e**2 would overflow or close to overflowing, scale down. */
@@ -6079,9 +6079,7 @@ norm2_do_sqrt (gfc_expr *result, gfc_expr *e)
if (result != e)
mpfr_set (result->value.real, e->value.real, GFC_RND_MODE);
mpfr_sqrt (result->value.real, result->value.real, GFC_RND_MODE);
- if (norm2_scale
- && mpfr_number_p (result->value.real)
- && !mpfr_zero_p (result->value.real))
+ if (norm2_scale && mpfr_regular_p (result->value.real))
{
mpfr_t tmp;
mpfr_init (tmp);
@@ -6120,9 +6118,7 @@ gfc_simplify_norm2 (gfc_expr *e, gfc_expr *dim)
result = simplify_transformation_to_scalar (result, e, NULL,
norm2_add_squared);
mpfr_sqrt (result->value.real, result->value.real, GFC_RND_MODE);
- if (norm2_scale
- && mpfr_number_p (result->value.real)
- && !mpfr_zero_p (result->value.real))
+ if (norm2_scale && mpfr_regular_p (result->value.real))
{
mpfr_t tmp;
mpfr_init (tmp);
Re: [PATCH v3] libgomp/test: Add flags to find libatomic in build-tree testing
On Wed, 20 Nov 2019, Jakub Jelinek wrote: > Ok with appropriate ChangeLog entry. I've used one included with the submission. Change applied now, thanks for your review. Maciej
Re: Reverting r278411
Segher Boessenkool writes: > Hi! > > On Wed, Nov 20, 2019 at 09:42:46AM +, Richard Sandiford wrote: >> Segher Boessenkool writes: >> >> Before I resubmit, why is the simplify-rtx.c part all wrong? >> > >> > It was nice and simple, and it isn't anymore. 8 4 2 1 for the four of >> > lt gt eq un are hardly worth documenting or making symbolic constants >> > for, because a) they are familiar to all powerpc users anyway (the >> > assembler has those as predefined constants!), admittedly this isn't a >> > strong argument for most people; >> >> Ah, OK. I was totally unaware of the connection. > > Yeah, I should have documented it. Time pressure was high the last N > weeks, with everyone else putting stuff in before end of stage 1 things > broke left and right for me. Normally I would just not update trunk the > last two months or so of stage 1 (for development -- for testing you > always need current trunk of course), but I needed some stuff from it. > Oh well, I finally dug myself out. > >> > but also b) they were only used in two short and obvious routines. >> > After your patch the routines are no longer short or obvious. >> > >> > A comparison result is exactly one of four things: lt, gt, eq, or un. >> > So we can express testing for any subset of those with just an OR of >> > the masks for the individual conditions. Whether a comparison is >> > floating point, floating point no-nans, signed, or unsigned, is just >> > a property of the comparison, not of the result, in this representation. >> >> Yeah, this works for OR because we never lose the unorderdness. > > It works for everything, including things with a NOT. > > If the mode does not allow unordered, you mask that away all the way at > the end, and nothing else needs to be done. This doesn't have to be done > for IOR because you can never end up with unordered in the mask if you > didn't start out with some code that allows unordered. > > You also can encode NE as not allowing unordered, if you have a mode > where that does not exist, but that is purely cosmetic. > > 8421 "full" "no-nan" > false false > 1000 lt lt > 0100 gt gt > 1100 ltgt ne > 0010 eq eq > 1010 le le > 0110 ge ge > 1110 orderedtrue > 0001 unordered > 1001 unlt > 0101 ungt > 1101 ne > 0011 uneq > 1011 unle > 0111 unge > true > > So for !HONOR_NANS modes we need to output LTGT as NE, and ORDERED as > true, and that is all. > >> There were two aspects to my patch. One was adding AND, and had: >> >> /* We only handle AND if we can ignore unordered cases. */ >> bool honor_nans_p = HONOR_NANS (GET_MODE (op0)); >> if (code != IOR && (code != AND || honor_nans_p)) >> return 0; >> >> This is needed because e.g. UNLT & ORDERED -> LT is only conditionally >> valid. It doesn't sound like you're objecting to that bit, is that right? >> Or was this what you had in mind with the reference to no-nans? > > UNLT & ORDERED is always LT. When would it not be true? LT traps on quiet NaNs for -ftrapping-math, UNLT and ORDERED don't. >> As mentioned in the covering note, I punted for this because handling >> trapping FP comparisons correctly for AND would need its own set of >> testcases. > > This isn't trapping arithmetic. Unordered is a perfectly normal result. > As IEEE 754 says: > Four mutually exclusive relations are possible: less than, equal, > greater than, and unordered. > This same is true when !HONOR_NANS, for signed integer comparisons, and > for unsigned integer comparisons: just UNORDERED never happens. > >> > If you do not mix signed and unsigned comparisons (they make not much >> > sense mixed anyway(*)), you need no changes at all: just translate >> > ltu/gtu/leu/geu to/from lt/gt/le/ge on the way in and out of this >> > function (there already are helper functions for that, signed_condition >> > and unsigned_condition). >> >> So this all seems to come down to whether unsigned comparisons are handled >> as separate mask bits or whether they're dealt with by removing the >> unsignedness and then adding it back. ISTM both are legitimate ways >> of doing it. I don't think one of them is "all wrong". > > It violates the whole design of the thing left and right. I never > documented that well (or at all), of course :-/ > >> I was very belatedly getting around to dealing with Joseph's comment >> when you sent your patch and had it approved. Since that patch seemed >> to be more favourably received in general, I was trying to work within >> the existing style of your version. And without the powerpc background, >> I just assumed that the mask values were "documented" by the first block >> of case statements: >> >> case LT: >> return 8; >> case GT: >> return 4; >> case EQ: >> return 2; >> case UNORDERED: >> return 1; > > Yeah, but it may not be obvious that exactly one of those is true for > any comparison, and you can combine them
[PATCH] PR c++/92236: Improve static assertions of concepts
This patch builds on https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01034.html.
Tie static assertion diagnostics into constraint diagnostic. For
example, this program:
template
concept iterator =
requires (I i)
{
++i;
*i;
};
static_assert(iterator);
Yields these errors:
x.cpp:11:15: error: static assertion failed
11 | static_assert(iterator);
| ^
x.cpp:11:15: note: constraints not satisfied
x.cpp:4:9: required by the constraints of ‘template concept iterator’
x.cpp:5:3: in requirements with ‘int i’
x.cpp:8:5: note: the required expression ‘* i’ is invalid
8 | *i;
| ^~
Andrew Sutton
0001-Emit-detailed-diagnostics-for-static-assertion-failu.patch
Description: Binary data
Re: [PATCH 1/2] PR 92463 MPFR modernization in GFortran
Hi Janne, On 11/18/19 9:34 PM, Janne Blomqvist wrote: Now that we require a minimum of MPFR 3.1.0+ to build GCC, we can do some modernization of the MPFR usage in the GFortran frontend. OK – thanks for the cleanup. [For reference, those changes were introduced in MPFR 3.0.0, cf. https://www.mpfr.org/mpfr-3.0.0/#changes ; 3.0 and 3.1 also added a bunch of new functions, MPFR_RNDA and did remove some undefined behaviour.] Do you intent update the other places as well? Namely: This patch replaces 1) GMP_RND* with MPFR_RND* Also used by: configure.ac, gcc/builtins.c, gcc/fold-const-call.c, gcc/gimple-ssa-sprintf.c, gcc/go/…, gcc/real.c, gcc/realmpfr.h, gcc/ubsan.c. 2) mp_exp_t with mpfr_exp_t gcc/realmpfr.c + gcc/go/… 3) mp_prec_t with mpfr_prec_t (only used by gcc/fortran) 4) mp_rnd_t with mpfr_rnd_t gcc/builtins.c, gcc/fold-const-call.c, gcc/realmpfr.c Cheers, Tobias gcc/fortran/ChangeLog: 2019-11-18 Janne Blomqvist PR fortran/92463 * arith.c (gfc_mpfr_to_mpz): Change mp_exp_t to mpfr_exp_t. (gfc_check_real_range): Likewise. * gfortran.h (GFC_RND_MODE): Change GMP_RNDN to MPFR_RNDN. * module.c (mio_gmp_real): Change mp_exp_t to mpfr_exp_t. * simplify.c (degrees_f): Change mp_rnd_t to mpfr_rnd_t. (radians_f): Likewise. (fullprec_erfc_scaled): Change mp_prec_t to mpfr_prec_t. (asympt_erfc_scaled): Likewise. (gfc_simplify_nearest): Change mp_exp_t to mpfr_exp_t, and GMP_RND* to MPFR_RND*. --- gcc/fortran/arith.c| 8 gcc/fortran/gfortran.h | 2 +- gcc/fortran/module.c | 2 +- gcc/fortran/simplify.c | 20 ++-- 4 files changed, 16 insertions(+), 16 deletions(-)
Re: [PATCH 3/X] [libsanitizer] Add option to bootstrap using HWASAN
On 20/11/2019 14:33, Martin Liška wrote: > On 11/13/19 4:24 PM, Matthew Malcomson wrote: >> On 12/11/2019 12:08, Martin Liška wrote: >>> On 11/11/19 5:03 PM, Matthew Malcomson wrote: Ah! My apologies -- I sent up a series with a few documentation mistakes. > >> >> b) Marking 'ptr' and 'mem' in the dump sounds like a good idea to me. >> Exactly how I'm not sure -- maybe with a colourscheme? Do you >> have a >> marking in mind? > > Libsanitizer is capable of using colors for report printing. > I can help with that and come up with a patch for upstream. > >> >> Uninitialised shadow space has the zero tag, however, there are a >> few >> extra details that help understanding these traces: >> >> On the stack, zero is both uninitialized and "the background" (i.e. >> the tag for anything not specially instrumented, like register >> spills >> and parameters passed on the stack). >> However, accessible tagged objects can be given a zero tag. > > Question here would be if we should use non-zero tags here? Maybe related > to my comment about skipping of HWASAN_STACK_BACKGROUND tag? Unfortunately we can't skip non-zero tags at compile time when using a random frame tag. This is because we don't know at compile time what the random frame tag will be. On each entry to a frame a "base tag" is generated randomly at runtime. Each local object in the frame has a compile-time offset that's what gets calculated in `hwasan_increment_tag` -- the offset from this random tag. The tag assigned to a local object is the runtime random frame tag plus the compile-time constant offset. I could avoid HWASAN_STACK_BACKGROUND as a tag when the parameter `hwasan-random-frame-tag` is false, since then there is no runtime random base tag (instead I start with zero). I'll be happy to add that in if you'd like -- I decided against it since it would only matter when a function has 256 or more variables, but I flip-flopped on the decision a few times. > >> We allow this to avoid runtime checks when incrementing the random >> frame tag to get the tag for a new local variable. >> We can easily avoid the zero tag at compile-time if we don't use a >> random tag for each frame. I had this in development at one point >> and found it quite useful for verification. I already have an >> option >> to disable random tags for each frame that this ability could go >> under. >> I don't believe (but am not 100% certain) this option is in LLVM. >> >> On the heap uninitialised is tag zero, but memory that has been >> `free`d is given a random tag, so non-zero in a dump does not mean a >> valid object. >> >> c) Is there an alternate notation you have in mind? >> I would guess the "dots" are there to say "this granule has no >> short-tag information", and I'm not sure what would be a better >> way to demonstrate that. > > Now I've got it here. Dot means that top-byte of a pointer equals to zero. > Right? Ah! I think I never described the "short-tag" functionality, and the fact it's in the debug output is getting confusing. This will also be part of answering your question "c)", and question "h", in the other email https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01950.html . -- The main tagging behaviour as described has a natural limitation. Invalid accesses that do not cross a 16 byte boundary are not caught, since each shadow-memory tag applies to a 16 byte chunk. To account for this, HWASAN has a "short-tag" functionality. This functionality was introduced in llvm-svn revision 365551. Usually, a shadow-memory byte records the *tag* that is valid for access to the relevant 16 byte granule in normal memory. When using short-tags, if an object fills only part of a 16 byte granule in normal memory, the corresponding shadow-memory byte stores the *length (in bytes) into this granule that is valid*. The *tag* is then stored in the last byte of the 16 byte granule in normal memory. (We know that last byte is unused, since this is a "short" granule tag). Now, checking a memory access consists of two parts. 1) A normal tag comparison. 2) A fallback in the tag-mismatch case. This fallback checks if the accessing pointer is accessing less bytes into the granule than the length given in shadow-memory. Then if that's the case it also checks the pointers tag matches the last byte in this 16 byte granule. That is a little difficult to explain clearly in text, so I apologise if the above doesn't make sense. -- The hwasan error-reporting output lists both memory tags *and* short-tags. These are the two sections under the titles of "Memory tags ..." and "Tags for short granules ...". The first printed section shows what is stored in shadow-memory. This is usually the tag, but can be a length if using "short" tags. The second section contains the "last byte of a granule" for
Re: Optimize fibonacci heap allocations
Hi,
sadly this patch seems to trigger false positive
../../gcc/vec.h:311:10: error: attempt to free a non-heap object �@Xa�@Y
[-Werror=free-nonheap-object]
which is about vec destructor freeing heap allocated memory if the
vector was ever resized from hits stack allocated place. In this case
it never is.
I am testing the following workaround which turns it into simple array
(only benefit of auto_vec here is the bounds check that I implemented by
hand).
I will commit it once x86_64-linux boostrap finishes unless someone
comes with better alternative. Sorry for this - it did not show up with
LTO build for me.
Honza
* fibonacci_heap.h (fibonacci_heap::consolidate): Turn auto_vec
to ordinary array.
Index: fibonacci_heap.h
===
--- fibonacci_heap.h(revision 278501)
+++ fibonacci_heap.h(working copy)
@@ -648,17 +648,18 @@ template
void fibonacci_heap::consolidate ()
{
const int D = 1 + 8 * sizeof (long);
- auto_vec *, D> a;
+ fibonacci_node *a[D];
fibonacci_node *w, *x, *y;
int i, d;
- a.quick_grow_cleared (D);
+ memset (a, 0, sizeof (a));
while ((w = m_root) != NULL)
{
x = w;
remove_root (w);
d = x->m_degree;
+ gcc_checking_assert (d < D);
while (a[d] != NULL)
{
y = a[d];
Reject versioning for alignment with different masks (PR 92526)
Allowing mixed vector sizes broke the assumption in the following assert,
since it's now possible for different accesses to require different
levels of alignment:
/* FORNOW: use the same mask to test all potentially unaligned
references in the loop. The vectorizer currently supports
a single vector size, see the reference to
GET_MODE_NUNITS (TYPE_MODE (vectype)) where the
vectorization factor is computed. */
gcc_assert (!LOOP_VINFO_PTR_MASK (loop_vinfo)
|| LOOP_VINFO_PTR_MASK (loop_vinfo) == mask);
I guess we could try to over-align smaller accesses so that all
of them are consistent, or try to support multiple alignment masks,
but for now the easiest fix seems to be to turn the assert into a
bail-out check.
Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
Richard
2019-11-20 Richard Sandiford
gcc/
PR tree-optimization/92526
* tree-vect-data-refs.c (vect_enhance_data_refs_alignment): Reject
versioning for alignment if the accesses do not have a consistent
mask, rather than asserting that the masks are consistent.
gcc/testsuite/
PR tree-optimization/92526
* gcc.target/aarch64/pr92526.c: New test.
Index: gcc/tree-vect-data-refs.c
===
--- gcc/tree-vect-data-refs.c 2019-11-16 11:40:19.105159717 +
+++ gcc/tree-vect-data-refs.c 2019-11-20 15:27:49.385346722 +
@@ -2266,13 +2266,15 @@ vect_enhance_data_refs_alignment (loop_v
mask must be 15 = 0xf. */
mask = size - 1;
- /* FORNOW: use the same mask to test all potentially unaligned
- references in the loop. The vectorizer currently supports
- a single vector size, see the reference to
- GET_MODE_NUNITS (TYPE_MODE (vectype)) where the
- vectorization factor is computed. */
- gcc_assert (!LOOP_VINFO_PTR_MASK (loop_vinfo)
- || LOOP_VINFO_PTR_MASK (loop_vinfo) == mask);
+ /* FORNOW: use the same mask to test all potentially unaligned
+references in the loop. */
+ if (LOOP_VINFO_PTR_MASK (loop_vinfo)
+ && LOOP_VINFO_PTR_MASK (loop_vinfo) != mask)
+ {
+ do_versioning = false;
+ break;
+ }
+
LOOP_VINFO_PTR_MASK (loop_vinfo) = mask;
LOOP_VINFO_MAY_MISALIGN_STMTS (loop_vinfo).safe_push (stmt_info);
}
Index: gcc/testsuite/gcc.target/aarch64/pr92526.c
===
--- /dev/null 2019-09-17 11:41:18.176664108 +0100
+++ gcc/testsuite/gcc.target/aarch64/pr92526.c 2019-11-20 15:27:49.385346722
+
@@ -0,0 +1,9 @@
+/* { dg-options "-O3 -mstrict-align" } */
+
+void
+f (unsigned int *restrict x, unsigned int *restrict y,
+ unsigned char *restrict z, unsigned int n)
+{
+ for (unsigned int i = 0; i < n % 4; ++i)
+x[i] = x[i] + y[i] + z[i];
+}
Re: Reverting r278411
Hi! On Wed, Nov 20, 2019 at 09:42:46AM +, Richard Sandiford wrote: > Segher Boessenkool writes: > >> Before I resubmit, why is the simplify-rtx.c part all wrong? > > > > It was nice and simple, and it isn't anymore. 8 4 2 1 for the four of > > lt gt eq un are hardly worth documenting or making symbolic constants > > for, because a) they are familiar to all powerpc users anyway (the > > assembler has those as predefined constants!), admittedly this isn't a > > strong argument for most people; > > Ah, OK. I was totally unaware of the connection. Yeah, I should have documented it. Time pressure was high the last N weeks, with everyone else putting stuff in before end of stage 1 things broke left and right for me. Normally I would just not update trunk the last two months or so of stage 1 (for development -- for testing you always need current trunk of course), but I needed some stuff from it. Oh well, I finally dug myself out. > > but also b) they were only used in two short and obvious routines. > > After your patch the routines are no longer short or obvious. > > > > A comparison result is exactly one of four things: lt, gt, eq, or un. > > So we can express testing for any subset of those with just an OR of > > the masks for the individual conditions. Whether a comparison is > > floating point, floating point no-nans, signed, or unsigned, is just > > a property of the comparison, not of the result, in this representation. > > Yeah, this works for OR because we never lose the unorderdness. It works for everything, including things with a NOT. If the mode does not allow unordered, you mask that away all the way at the end, and nothing else needs to be done. This doesn't have to be done for IOR because you can never end up with unordered in the mask if you didn't start out with some code that allows unordered. You also can encode NE as not allowing unordered, if you have a mode where that does not exist, but that is purely cosmetic. 8421 "full" "no-nan" false false 1000 lt lt 0100 gt gt 1100 ltgt ne 0010 eq eq 1010 le le 0110 ge ge 1110 orderedtrue 0001 unordered 1001 unlt 0101 ungt 1101 ne 0011 uneq 1011 unle 0111 unge true So for !HONOR_NANS modes we need to output LTGT as NE, and ORDERED as true, and that is all. > There were two aspects to my patch. One was adding AND, and had: > > /* We only handle AND if we can ignore unordered cases. */ > bool honor_nans_p = HONOR_NANS (GET_MODE (op0)); > if (code != IOR && (code != AND || honor_nans_p)) > return 0; > > This is needed because e.g. UNLT & ORDERED -> LT is only conditionally > valid. It doesn't sound like you're objecting to that bit, is that right? > Or was this what you had in mind with the reference to no-nans? UNLT & ORDERED is always LT. When would it not be true? > As mentioned in the covering note, I punted for this because handling > trapping FP comparisons correctly for AND would need its own set of > testcases. This isn't trapping arithmetic. Unordered is a perfectly normal result. As IEEE 754 says: Four mutually exclusive relations are possible: less than, equal, greater than, and unordered. This same is true when !HONOR_NANS, for signed integer comparisons, and for unsigned integer comparisons: just UNORDERED never happens. > > If you do not mix signed and unsigned comparisons (they make not much > > sense mixed anyway(*)), you need no changes at all: just translate > > ltu/gtu/leu/geu to/from lt/gt/le/ge on the way in and out of this > > function (there already are helper functions for that, signed_condition > > and unsigned_condition). > > So this all seems to come down to whether unsigned comparisons are handled > as separate mask bits or whether they're dealt with by removing the > unsignedness and then adding it back. ISTM both are legitimate ways > of doing it. I don't think one of them is "all wrong". It violates the whole design of the thing left and right. I never documented that well (or at all), of course :-/ > I was very belatedly getting around to dealing with Joseph's comment > when you sent your patch and had it approved. Since that patch seemed > to be more favourably received in general, I was trying to work within > the existing style of your version. And without the powerpc background, > I just assumed that the mask values were "documented" by the first block > of case statements: > > case LT: > return 8; > case GT: > return 4; > case EQ: > return 2; > case UNORDERED: > return 1; Yeah, but it may not be obvious that exactly one of those is true for any comparison, and you can combine them to a mask and do any logical operations on it. > Adding two more cases here didn't seem to make things any more unclear. > But maybe it is more jarring if you've memorised the powerpc mapping. It's also that 2**4 is small, but 2**6 is not. The 2**4 cases
Restrict bb-slp-40.c to targets with VnQI addition (PR 92366)
bb-slp-40.c fails on SPARC targets without VIS4 because it
requires addition on vectors of bytes. There doesn't seem to be
an existing target selector for this, so I added vect_char_add.
(Wasn't sure whether to use vect_char_add, for consistency
with vect_no_int_add/vect_int_mult etc., or vect_add_char for
consistency with vect_shift_char etc.)
I took the target list from vect_int and removed targets that didn't
seem to support the operation (namely sparc*, since we don't seem to
have any test for VIS4, niagara7 or m8, and alpha*-*-*.)
Tested on aarch64-linux-gnu, x86_64-linux-gnu, powerpc64-linux-gnu
(Power 7) and sparc-sun-solaris2.11. OK to install?
Richard
2019-11-20 Richard Sandiford
gcc/
PR testsuite/92366
* doc/sourcebuild.texi (vect_char_add): Document.
gcc/testsuite/
PR testsuite/92366
* lib/target-supports.exp (check_effective_target_vect_char_add):
New proc.
* gcc.dg/vect/bb-slp-40.c: Require vect_char_add instead of vect_int.
Index: gcc/doc/sourcebuild.texi
===
--- gcc/doc/sourcebuild.texi2019-11-18 15:36:04.865884928 +
+++ gcc/doc/sourcebuild.texi2019-11-20 15:11:52.572010515 +
@@ -1522,6 +1522,10 @@ Target does not support a vector add ins
@item vect_no_bitwise
Target does not support vector bitwise instructions.
+@item vect_char_add
+Target supports addition of @code{char} vectors for at least one
+vector length.
+
@item vect_char_mult
Target supports @code{vector char} multiplication.
Index: gcc/testsuite/lib/target-supports.exp
===
--- gcc/testsuite/lib/target-supports.exp 2019-11-18 15:36:04.869884903
+
+++ gcc/testsuite/lib/target-supports.exp 2019-11-20 15:11:52.580010459
+
@@ -5749,6 +5749,27 @@ proc check_effective_target_vect_bswap {
|| [istarget amdgcn-*-*] }}]
}
+# Return 1 if the target supports addition of char vectors for at least
+# one vector length.
+
+proc check_effective_target_vect_char_add { } {
+return [check_cached_effective_target_indexed vect_int {
+ expr {
+ [istarget i?86-*-*] || [istarget x86_64-*-*]
+ || ([istarget powerpc*-*-*]
+&& ![istarget powerpc-*-linux*paired*])
+|| [istarget amdgcn-*-*]
+|| [istarget ia64-*-*]
+|| [istarget aarch64*-*-*]
+|| [is-effective-target arm_neon]
+|| ([istarget mips*-*-*]
+&& ([et-is-effective-target mips_loongson_mmi]
+|| [et-is-effective-target mips_msa]))
+|| ([istarget s390*-*-*]
+&& [check_effective_target_s390_vx])
+ }}]
+}
+
# Return 1 if the target supports hardware vector shift operation for char.
proc check_effective_target_vect_shift_char { } {
Index: gcc/testsuite/gcc.dg/vect/bb-slp-40.c
===
--- gcc/testsuite/gcc.dg/vect/bb-slp-40.c 2019-11-19 16:25:49.0
+
+++ gcc/testsuite/gcc.dg/vect/bb-slp-40.c 2019-11-20 15:11:52.572010515
+
@@ -1,6 +1,6 @@
/* { dg-do compile } */
/* { dg-additional-options "-fvect-cost-model=dynamic" } */
-/* { dg-require-effective-target vect_int } */
+/* { dg-require-effective-target vect_char_add } */
char g_d[1024], g_s1[1024], g_s2[1024];
void foo(void)
