Re: [PATCH V3][gcc] libgccjit: introduce version entry points
On Mon, 2020-03-30 at 12:09 -0400, David Malcolm via Gcc-patches wrote: > On Sun, 2020-03-29 at 21:31 +0100, Andrea Corallo wrote: > > David Malcolm writes: > > > > > On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote: > > > Please add the new test to the header in its alphabetical > > > location, > > > i.e. between: > > > > > > /* test-vector-types.cc: We don't use this, since it's C++. */ > > > > > > and > > > > > > /* test-volatile.c */ > > > > > > An entry also needs to be added to the "testcases" array at the > > > end > > > of > > > the header (again, in the alphabetical-sorted location). > > > > I tried adding the test into the "testcases" array but this makes > > the > > threads test failing. > > > > I think this has nothing to do with this patch and happen just > > because > > this test does not define any code. Infact I see the same > > happening > > just adding "test-empty.c" to the "testcases" array on the current > > master. > > > > The error is not very reproducible, I tried a run under valgrind > > but > > have found nothing so far :/ > > > > Dave do you recall if there was a specific reason not to have > > "test-empty.c" into the "testcases" array? > > > > Andrea > > I replied to this as: > > [PATCH] lra: set insn_code_data to NULL when freeing > https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542929.html > > but forgot to set the reply-to in git send-email; sorry. > Andrea: I've pushed my proposed fix for the above to master as 3809bcd6c0ee324cbd855c68cee104c8bf134dbe. Does this fix the issue you were seeing? Thanks Dave
Re: [PATCH], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2
Hi! On Mon, Mar 30, 2020 at 12:50:43PM -0500, will schmidt wrote: > On Fri, 2020-03-27 at 21:31 -0400, Michael Meissner via Gcc-patches > > * config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): New macro. > > (rs6000_option_override_internal): Set the -mprefixed and > > -mpcrel > > options for -mcpu=future if these options can be used. > > > s/can be used/are supported by the platform/ ? The code says /* Enable -mprefixed by default on 64-bit 'future' systems. */ /* If the OS has support for PC-relative relocations, enable it now. */ and something like that should go in the changelog as well (two lines in changelog is fine -- they are two hunks of patch as well, anyway!) > > +/* Enable default support for PC-relative addressing on the 'future' > > system if > > + we can use the PC-relative instructions. Currently this support > > only exits > > exists > > > + for the ELF v2 object file format using the medium code > > model. */ > > should that be "s/object file format/ABI/" ? Yes. > > -/* Support for a future processor's features. Do not enable -mpcrel > > until it > > - is fully functional. */ > > +/* Support for a future processor's features. We do not set -mpcrel > > or > > + -mprefixed here. These bits are set in rs6000_option_override if > > the system > > + supports those options. */ > > I'm still not sure the comment here is actually necessary, there are > many other places where we also do not set -mpcrel or -mprefixed. If > history of the code here requires a hint to point at those options > being set in rs6000_option_override, then it's fine. If you really need to say you do *not* do something, you should say why not. Without that it only leaves more questions to the reader :-) Hopefully that then also explains why the reader should care about this. Segher
Re: [PATCH] lra: set insn_code_data to NULL when freeing
On 2020-03-30 12:06 p.m., David Malcolm wrote:
It's a double-free bug in lra.c, albeit one that requires being used
in a multithreaded way from libgccjit to be triggered.
libgccjit's test-threads.c repeatedly compiles and runs numerous tests,
each in a separate thread.
Attempting to add an empty test that generates no code leads to a
double-free ICE within that thread, within lra.c's
finish_insn_code_data_once.
The root cause is that the insn_code_data array is cleared in
init_insn_code_data_once, but this is only called the first time
a cgraph_node is expanded [1], whereas the "loop-over-all-elements
and free them" is unconditionally called in finalize [2]. Hence
if there are no functions:
* the array is not re-initialized for the empty context
* when finish_insn_code_data_once is called for the empty context
it still contains the freed pointers from the previous context
that held the jit mutex, and hence the free is a double-free.
This patch sets the pointers to NULL after freeing them, fixing
the ICE. The calls to free are still guarded by a check for NULL,
which is redundant, but maybe there's a reason for not wanting to
call "free" on a possibly-NULL value many times on process exit?
(it makes the diff cleaner, at least)
Fixes the issue in jit.dg.
Full bootstrap & regression test in progress.
Is it OK for master if it passes?
Sure, David. Thank you for the patch.
gcc/ChangeLog:
* lra.c (finish_insn_code_data_once): Set the array elements
to NULL after freeing them.
gcc/testsuite/ChangeLog:
* jit.dg/all-non-failing-tests.h: Add test-empty.c
---
gcc/lra.c| 5 -
gcc/testsuite/jit.dg/all-non-failing-tests.h | 10 ++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/gcc/lra.c b/gcc/lra.c
index d5ea3622686..5e8b75b1fda 100644
--- a/gcc/lra.c
+++ b/gcc/lra.c
@@ -653,7 +653,10 @@ finish_insn_code_data_once (void)
for (unsigned int i = 0; i < NUM_INSN_CODES; i++)
{
if (insn_code_data[i] != NULL)
- free (insn_code_data[i]);
+ {
+ free (insn_code_data[i]);
+ insn_code_data[i] = NULL;
+ }
}
}
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index b2acc74ae95..af744192a73 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -116,6 +116,13 @@
#undef create_code
#undef verify_code
+/* test-empty.c */
+#define create_code create_code_empty
+#define verify_code verify_code_empty
+#include "test-empty.c"
+#undef create_code
+#undef verify_code
+
/* test-error-*.c: We don't use these test cases, since they deliberately
introduce errors, which we don't want here. */
@@ -328,6 +335,9 @@ const struct testcase testcases[] = {
{"expressions",
create_code_expressions,
verify_code_expressions},
+ {"empty",
+ create_code_empty,
+ verify_code_empty},
{"factorial",
create_code_factorial,
verify_code_factorial},
Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]
On Mon, 30 Mar 2020, Jason Merrill wrote:
> On 3/30/20 3:58 PM, Patrick Palka wrote:
> > On Thu, 26 Mar 2020, Jason Merrill wrote:
> >
> > > On 3/22/20 9:21 PM, Patrick Palka wrote:
> > > > This patch relaxes an assertion in tsubst_default_argument that exposes
> > > > a
> > > > latent
> > > > bug in how we substitute an array type into a cv-qualified wildcard
> > > > function
> > > > parameter type. Concretely, the latent bug is that given the function
> > > > template
> > > >
> > > > template void foo(const T t);
> > > >
> > > > one would expect the type of foo to be void(const int*), but we
> > > > (seemingly prematurely) strip function parameter types of their
> > > > top-level
> > > > cv-qualifiers when building the function's TYPE_ARG_TYPES, and instead
> > > > end
> > > > up
> > > > obtaining void(int*) as the type of foo after substitution and
> > > > decaying.
> > > >
> > > > We still however correctly substitute into and decay the formal
> > > > parameter
> > > > type,
> > > > obtaining const int* as the type of t after substitution. But this then
> > > > leads
> > > > to us tripping over the assert in tsubst_default_argument that verifies
> > > > the
> > > > formal parameter type and the function type are consistent.
> > > >
> > > > Assuming it's too late at this stage to fix the substitution bug, we can
> > > > still
> > > > relax the assertion like so. Tested on x86_64-pc-linux-gnu, does this
> > > > look
> > > > OK?
> > >
> > > This is core issues 1001/1322, which have not been resolved. Clang does
> > > the
> > > substitution the way you suggest; EDG rejects the testcase because the two
> > > substitutions produce different results. I think it would make sense to
> > > follow the EDG behavior until this issue is actually resolved.
> >
> > Here is what I have so far towards that end. When substituting into the
> > PARM_DECLs of a function decl, we now additionally check if the
> > aforementioned Core issues are relevant and issue a (fatal) diagnostic
> > if so. This patch checks this in tsubst_decl rather
> > than in tsubst_function_decl for efficiency reasons, so that we don't
> > have to perform another traversal over the DECL_ARGUMENTS /
> > TYPE_ARG_TYPES just to implement this check.
>
> Hmm, this seems like writing more complicated code for a very marginal
> optimization; how many function templates have so many parameters that walking
> over them once to compare types will have any effect on compile time?
Good point... though I just tried implementing this check in
tsubst_function_decl, and it seems it might be just as complicated to
implement it there instead, at least if we want to handle function
parameter packs correctly.
If we were to implement this check in tsubst_function_decl, then since
we have access to the instantiated function, it would presumably suffice
to compare its substituted DECL_ARGUMENTS with its substituted
TYPE_ARG_TYPES to see if they're consistent. Doing so would certainly
catch the original testcase, i.e.
template
void foo(const T);
int main() { foo(0); }
because the DECL_ARGUMENTS of foo would be {const int*} and its
TYPE_ARG_TYPES would be {int*}. But apparently it doesn't catch the
corresponding testcase that uses a function parameter pack, i.e.
template
void foo(const Ts...);
int main() { foo(0); }
because it turns out we don't strip top-level cv-qualifiers from
function parameter packs from TYPE_ARG_TYPES at declaration time, as we
do with regular function parameters. So in this second testcase both
DECL_ARGUMENTS and TYPE_ARG_TYPES of foo would be {const int*},
and yet we would (presumably) want to reject this instantiation too.
So it seems comparing TYPE_ARG_TYPES and DECL_ARGUMENTS from
tsubst_function_decl would not suffice, and we would still need to do a
variant of the trick that's done in this patch, i.e. substitute into
each dependent parameter type stripped of its top-level cv-qualifiers,
to see if these cv-qualifiers make a material difference in the
resulting function type. Or maybe there's yet another way to detect
this?
>
> > Is something like this what you have in mind?
> >
> > -- >8 --
> >
> > Subject: [PATCH] c++: Reject some instantiations that depend on resolution
> > of
> > Core issues 1001/1322
> >
> > gcc/cp/ChangeLog:
> >
> > Core issues 1001 and 1322
> > PR c++/92010
> > * pt.c (tsubst_decl) : Detect and reject the case
> > where
> > the function type depends on whether we strip top-level qualifiers
> > from
> > the type of T before or after substitution.
> >
> > gcc/testsuite/ChangeLog:
> >
> > Core issues 1001 and 1322
> > PR c++/92010
> > * g++.dg/template/array33.C: New test.
> > * g++.dg/template/array34.C: New test.
> > ---
> > gcc/cp/pt.c | 70 -
> > gcc/testsuite/g++.dg/template/array33.C | 39 ++
> > gcc/testsuite/g++.dg/template/array34.C | 63
Re: PATCH -- Fix degree trignometric functions
On Mon, Mar 30, 2020 at 4:53 PM Tobias Burnus wrote: > > Hi Fritz, > > On 3/30/20 10:20 PM, Fritz Reese via Fortran wrote: > > >>> * All included files need dependency; I do not quickly > >>>see whether that' the case. > > If one looks at the build, the dependency is automatically > obtained and tracked in …/.deps/*.Po in the build directory. > Hence, no action needed. > > Cheers, > > Tobias Awesome, thanks! Fritz
[pushed] c++: Fix comparison of fn() and ns::fn() [PR90711]
The resolution of CWG issue 1321 clarified that when deciding whether two
expressions involving template parameters are equivalent, two dependent
function calls where the function is named with an unqualified-id are
considered to be equivalent if the name is the same, even if unqualified
lookup finds different sets of functions. We were wrongly treating
qualified-ids the same way, so that EXISTS and test::EXISTS were considered
to be equivalent even though they are looking up the name in different
scopes. This also causes a mangling bug, but I don't think it's safe to fix
that for GCC 10; this patch just fixes the comparison.
Tested x86_64-pc-linux-gnu, applying to trunk/9.
gcc/cp/ChangeLog
2020-03-30 Jason Merrill
PR c++/90711
* tree.c (cp_tree_equal) [CALL_EXPR]: Compare KOENIG_LOOKUP_P.
(called_fns_equal): Check DECL_CONTEXT.
---
gcc/cp/tree.c | 14 ++-
.../g++.dg/template/dependent-name14.C| 38 +++
2 files changed, 51 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/template/dependent-name14.C
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index b85967e1bfa..a2172dea0d8 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -2464,6 +2464,8 @@ is_overloaded_fn (tree x)
tree
dependent_name (tree x)
{
+ /* FIXME a dependent name must be unqualified, but this function doesn't
+ distinguish between qualified and unqualified identifiers. */
if (identifier_p (x))
return x;
if (TREE_CODE (x) == TEMPLATE_ID_EXPR)
@@ -3581,6 +3583,15 @@ called_fns_equal (tree t1, tree t2)
if (name1 != name2)
return false;
+ /* FIXME dependent_name currently returns an unqualified name regardless
+of whether the function was named with a qualified- or unqualified-id.
+Until that's fixed, check that we aren't looking at overload sets from
+different scopes. */
+ if (is_overloaded_fn (t1) && is_overloaded_fn (t2)
+ && (DECL_CONTEXT (get_first_fn (t1))
+ != DECL_CONTEXT (get_first_fn (t2
+ return false;
+
if (TREE_CODE (t1) == TEMPLATE_ID_EXPR)
targs1 = TREE_OPERAND (t1, 1);
if (TREE_CODE (t2) == TEMPLATE_ID_EXPR)
@@ -3677,7 +3688,8 @@ cp_tree_equal (tree t1, tree t2)
{
tree arg1, arg2;
call_expr_arg_iterator iter1, iter2;
- if (!called_fns_equal (CALL_EXPR_FN (t1), CALL_EXPR_FN (t2)))
+ if (KOENIG_LOOKUP_P (t1) != KOENIG_LOOKUP_P (t2)
+ || !called_fns_equal (CALL_EXPR_FN (t1), CALL_EXPR_FN (t2)))
return false;
for (arg1 = first_call_expr_arg (t1, ),
arg2 = first_call_expr_arg (t2, );
diff --git a/gcc/testsuite/g++.dg/template/dependent-name14.C
b/gcc/testsuite/g++.dg/template/dependent-name14.C
new file mode 100644
index 000..52d2e72be35
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name14.C
@@ -0,0 +1,38 @@
+// PR c++/90711
+// { dg-do compile { target c++11 } }
+
+namespace test {
+void EXISTS(int);
+}
+
+template
+struct stub_void {
+typedef void type;
+};
+template
+using stub_void_t = typename stub_void::type;
+
+#if !defined(SUPPRESS)
+template
+struct has_to_string {
+static constexpr bool value = false;
+};
+
+template
+struct has_to_string> {
+static constexpr bool value = true;
+};
+#endif
+
+template
+struct has_std_to_string {
+static constexpr bool value = false;
+};
+
+template
+struct has_std_to_string> {
+static constexpr bool value = true;
+};
+
+static_assert (has_std_to_string::value, "");
+
base-commit: 1cb1986cb596336e688c079b821205ec212a46a3
--
2.18.1
Re: [committed] [P1][PR target/94238] Don't create invalid comparisons
On Tue, 2020-03-24 at 10:50 +0100, Jakub Jelinek wrote:
> On Mon, Mar 23, 2020 at 06:00:06PM -0600, Jeff Law via Gcc-patches wrote:
> > +/* Return true if CODE is valid for comparisons of mode MODE, false
> > + otherwise.
> > +
> > + It is always safe to return false, even if the code was valid for the
> > + given mode as that will merely suppress optimizations. */
> > +
> > +static bool
> > +comparison_code_valid_for_mode (enum rtx_code code, enum machine_mode mode)
> > +{
> > + switch (code)
> > +{
> > + /* These are valid for integral, floating and vector modes. */
> > + case NE:
> > + case EQ:
> > + case GE:
> > + case GT:
> > + case LE:
> > + case LT:
> > + return (INTEGRAL_MODE_P (mode)
> > + || FLOAT_MODE_P (mode)
> > + || VECTOR_MODE_P (mode));
>
> I'm not sure I understand the VECTOR_MODE_P cases.
> INTEGRAL_MODE_P or FLOAT_MODE_P already do include vector modes with the
> corresponding element types.
> And if you want the {,u}{fract,accum} modes for some of these, shouldn't
> they apply to both scalar and vector modes thereof?
> So, either drop the || VECTOR_MODE_P (mode), or use
> > > ALL_FRACT_MODE_P (mode) || ALL_ACCUM_MODE_P (mode)
> instead?
It's unclear to me as well. I tried to follow rtl.def as closely as possible
for
that reason. rtl.def doesn't call out the fractional modes, accumulator modes,
etc. It speaks strictly in terms of integral, float, vector mode classes.
+ /* These are filtered out in simplify_logical_operation, but
> > +we check for them too as a matter of safety. They are valid
> > +for integral and vector modes. */
> > + case GEU:
> > + case GTU:
> > + case LEU:
> > + case LTU:
> > + return INTEGRAL_MODE_P (mode) || VECTOR_MODE_P (mode);
>
> This doesn't look right. I don't know what the fract/accum modes want, but
> you don't want to return true for GET_MODE_CLASS (mode) ==
> MODE_VECTOR_FLOAT, do you?
Again, this is a direct translation from the documentation in rtl.def.
I certainly don't mind changing it if there's a concrete proposal.
jeff
Re: PATCH -- Fix degree trignometric functions
Hi Fritz, On 3/30/20 10:20 PM, Fritz Reese via Fortran wrote: * All included files need dependency; I do not quickly see whether that' the case. If one looks at the build, the dependency is automatically obtained and tracked in …/.deps/*.Po in the build directory. Hence, no action needed. Cheers, Tobias - Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Re: [PATCH] c++: Fix handling of internal fn calls in statement expressions [PR94385]
On 3/29/20 6:42 AM, Jakub Jelinek wrote:
Hi!
The following testcase ICEs, because the FE when processing the statement
expression changes the .VEC_CONVERT internal fn CALL_EXPR into .PHI call.
That is because the internal fn call is recorded in the base.u.ifn
field, which overlaps base.u.bits.lang_flag_1 which is used for
STMT_IS_FULL_EXPR_P, so this essentially does ifn |= 2 on little-endian.
STMT_IS_FULL_EXPR_P bit is used in:
cp-gimplify.c- if (STATEMENT_CODE_P (code))
cp-gimplify.c-{
cp-gimplify.c- saved_stmts_are_full_exprs_p = stmts_are_full_exprs_p ();
cp-gimplify.c- current_stmt_tree ()->stmts_are_full_exprs_p
cp-gimplify.c:= STMT_IS_FULL_EXPR_P (*expr_p);
cp-gimplify.c-}
and
pt.c- if (STATEMENT_CODE_P (TREE_CODE (t)))
pt.c:current_stmt_tree ()->stmts_are_full_exprs_p = STMT_IS_FULL_EXPR_P (t);
so besides being wrong on some other codes, it actually isn't beneficial at
all to set it on anything else, so the following patch restricts it to
trees with STATEMENT_CODE_P TREE_CODE.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
2020-03-29 Jakub Jelinek
PR c++/94385
* semantics.c (add_stmt): Only set STMT_IS_FULL_EXPR_P on trees with
STATEMENT_CODE_P code.
--- gcc/cp/semantics.c.jj 2020-03-28 10:19:14.898349472 +0100
+++ gcc/cp/semantics.c 2020-03-29 00:02:40.648258781 +0100
@@ -380,7 +380,8 @@ add_stmt (tree t)
/* When we expand a statement-tree, we must know whether or not the
statements are full-expressions. We record that fact here. */
- STMT_IS_FULL_EXPR_P (t) = stmts_are_full_exprs_p ();
+ if (STATEMENT_CODE_P (TREE_CODE (t)))
+ STMT_IS_FULL_EXPR_P (t) = stmts_are_full_exprs_p ();
}
if (code == LABEL_EXPR || code == CASE_LABEL_EXPR)
--- gcc/testsuite/c-c++-common/pr94385.c.jj 2020-03-29 00:05:23.402823607
+0100
+++ gcc/testsuite/c-c++-common/pr94385.c2020-03-29 00:05:08.149051828
+0100
@@ -0,0 +1,12 @@
+/* PR c++/94385 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+typedef int V __attribute__((__vector_size__(16)));
+typedef float W __attribute__((__vector_size__(16)));
+
+void
+foo (W *x, V *y)
+{
+ *y = (({ __builtin_convertvector (*x, V); }));
+}
Jakub
Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]
On 3/30/20 3:58 PM, Patrick Palka wrote:
On Thu, 26 Mar 2020, Jason Merrill wrote:
On 3/22/20 9:21 PM, Patrick Palka wrote:
This patch relaxes an assertion in tsubst_default_argument that exposes a
latent
bug in how we substitute an array type into a cv-qualified wildcard function
parameter type. Concretely, the latent bug is that given the function
template
template void foo(const T t);
one would expect the type of foo to be void(const int*), but we
(seemingly prematurely) strip function parameter types of their top-level
cv-qualifiers when building the function's TYPE_ARG_TYPES, and instead end
up
obtaining void(int*) as the type of foo after substitution and
decaying.
We still however correctly substitute into and decay the formal parameter
type,
obtaining const int* as the type of t after substitution. But this then
leads
to us tripping over the assert in tsubst_default_argument that verifies the
formal parameter type and the function type are consistent.
Assuming it's too late at this stage to fix the substitution bug, we can
still
relax the assertion like so. Tested on x86_64-pc-linux-gnu, does this look
OK?
This is core issues 1001/1322, which have not been resolved. Clang does the
substitution the way you suggest; EDG rejects the testcase because the two
substitutions produce different results. I think it would make sense to
follow the EDG behavior until this issue is actually resolved.
Here is what I have so far towards that end. When substituting into the
PARM_DECLs of a function decl, we now additionally check if the
aforementioned Core issues are relevant and issue a (fatal) diagnostic
if so. This patch checks this in tsubst_decl rather
than in tsubst_function_decl for efficiency reasons, so that we don't
have to perform another traversal over the DECL_ARGUMENTS /
TYPE_ARG_TYPES just to implement this check.
Hmm, this seems like writing more complicated code for a very marginal
optimization; how many function templates have so many parameters that
walking over them once to compare types will have any effect on compile
time?
Is something like this what you have in mind?
-- >8 --
Subject: [PATCH] c++: Reject some instantiations that depend on resolution of
Core issues 1001/1322
gcc/cp/ChangeLog:
Core issues 1001 and 1322
PR c++/92010
* pt.c (tsubst_decl) : Detect and reject the case where
the function type depends on whether we strip top-level qualifiers from
the type of T before or after substitution.
gcc/testsuite/ChangeLog:
Core issues 1001 and 1322
PR c++/92010
* g++.dg/template/array33.C: New test.
* g++.dg/template/array34.C: New test.
---
gcc/cp/pt.c | 70 -
gcc/testsuite/g++.dg/template/array33.C | 39 ++
gcc/testsuite/g++.dg/template/array34.C | 63 ++
3 files changed, 171 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/template/array33.C
create mode 100644 gcc/testsuite/g++.dg/template/array34.C
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 8564eb11df4..fd99053df36 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -14072,9 +14072,77 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
complain)
/* We're dealing with a normal parameter. */
type = tsubst (TREE_TYPE (t), args, complain, in_decl);
+ const int type_quals = cp_type_quals (type);
+
+ /* Determine whether the function type after substitution into the
+ type of the function parameter T depends on the resolution of
+ Core issues 1001/1322, which have not been resolved. In
+ particular, the following detects the case where the resulting
+ function type depends on whether we strip top-level qualifiers
+ from the type of T before or after substitution.
+
+ This can happen only when the dependent type of T is a
+ cv-qualified wildcard type, and substitution yields a
+ (cv-qualified) array type before array-to-pointer conversion. */
+ tree unqual_expanded_types = NULL_TREE;
+ if (TREE_CODE (type) == ARRAY_TYPE
+ && (type_quals & (TYPE_QUAL_CONST|TYPE_QUAL_VOLATILE))
+ && DECL_FUNCTION_SCOPE_P (t)
+ && !DECL_TEMPLATE_PARM_P (t))
+ {
+ tree type_pattern = (PACK_EXPANSION_P (TREE_TYPE (t))
+? PACK_EXPANSION_PATTERN (TREE_TYPE (t))
+: TREE_TYPE (t));
+ if (WILDCARD_TYPE_P (type_pattern)
+ && cv_qualified_p (type_pattern))
+ {
+ /* Substitute into the corresponding wildcard type that is
+ stripped of its own top-level cv-qualifiers. */
+ tree unqual_type;
+ if (PACK_EXPANSION_P (TREE_TYPE
[PATCH] c++: Fix crash in gimplifier with paren init of aggregates [PR94155]
Here we crash in the gimplifier because gimplify_init_ctor_eval doesn't
expect null indexes for a constructor:
/* ??? Here's to hoping the front end fills in all of the indices,
so we don't have to figure out what's missing ourselves. */
gcc_assert (purpose);
The indexes weren't filled because we never called reshape_init: for
a constructor that represents parenthesized initialization of an
aggregate we don't allow brace elision or designated initializers. So
fill in the indexes manually, here we have an array, and we can simply
assign indexes starting from 0.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
PR c++/94155 - crash in gimplifier with paren init of aggregates.
* decl.c (check_initializer): Fill in constructor indexes.
* g++.dg/cpp2a/paren-init22.C: New test.
---
gcc/cp/decl.c | 6 ++
gcc/testsuite/g++.dg/cpp2a/paren-init22.C | 15 +++
2 files changed, 21 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/paren-init22.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 69a238997b4..80dd2d8b931 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -6754,6 +6754,12 @@ check_initializer (tree decl, tree init, int flags,
vec **cleanups)
init = build_constructor_from_list (init_list_type_node, init);
CONSTRUCTOR_IS_DIRECT_INIT (init) = true;
CONSTRUCTOR_IS_PAREN_INIT (init) = true;
+ /* The gimplifier expects that the front end fills in all of the
+indices. Normally, reshape_init_array fills these in, but we
+don't call reshape_init because that does nothing when it gets
+CONSTRUCTOR_IS_PAREN_INIT. */
+ for (unsigned int i = 0; i < CONSTRUCTOR_NELTS (init); i++)
+ CONSTRUCTOR_ELT (init, i)->index = size_int (i);
}
}
else if (TREE_CODE (init) == TREE_LIST
diff --git a/gcc/testsuite/g++.dg/cpp2a/paren-init22.C
b/gcc/testsuite/g++.dg/cpp2a/paren-init22.C
new file mode 100644
index 000..1b2959e7731
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/paren-init22.C
@@ -0,0 +1,15 @@
+// PR c++/94155 - crash in gimplifier with paren init of aggregates.
+// { dg-do compile { target c++2a } }
+
+struct S { int i, j; };
+
+struct A {
+ S s;
+ constexpr A(S e) : s(e) {}
+};
+
+void
+f()
+{
+ A g[1]({{1, 1}});
+}
base-commit: 48e331d63827a0500670d685c0fe7d609e0a807a
--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Re: PATCH -- Fix degree trignometric functions
On Sat, Mar 28, 2020 at 12:37 PM Steve Kargl wrote: > > On Sat, Mar 28, 2020 at 08:10:38AM +0100, Tobias Burnus wrote: > > Two remarks: > > > > * As the file trigd_lib.inc is shared between libgfortran > > and gcc/fortran, I wonder whether it should be placed > > under include/ (under the GCC toplevel directroy) > > The original name and location were chosen to match > intrinsics/erfc_scaled_inc.c, which is included in > intrinsics/erfc_scaled.c. I think Fritz's re-use > in simplification makes since given the ugly nested > if-elseif-else constructs in the file. As this is > Fortran specific, I do not believe it should be moved > up to a toplevel diretory. I'm ambivalent about the location, though it is Fortran-specific. > > * All included files need dependency; I do not quickly > > see whether that' the case. Yes, I'd like to add a dependency for them as rebuilding after a change is a pain. I've not yet decoded the Make-fu involved, but I will be sure to add them for the final patch. Thanks for the feedback, Tobias. --- Fritz
Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]
On Mon, 30 Mar 2020, Patrick Palka wrote:
> On Thu, 26 Mar 2020, Jason Merrill wrote:
>
> > On 3/22/20 9:21 PM, Patrick Palka wrote:
> > > This patch relaxes an assertion in tsubst_default_argument that exposes a
> > > latent
> > > bug in how we substitute an array type into a cv-qualified wildcard
> > > function
> > > parameter type. Concretely, the latent bug is that given the function
> > > template
> > >
> > >template void foo(const T t);
> > >
> > > one would expect the type of foo to be void(const int*), but we
> > > (seemingly prematurely) strip function parameter types of their top-level
> > > cv-qualifiers when building the function's TYPE_ARG_TYPES, and instead end
> > > up
> > > obtaining void(int*) as the type of foo after substitution and
> > > decaying.
> > >
> > > We still however correctly substitute into and decay the formal parameter
> > > type,
> > > obtaining const int* as the type of t after substitution. But this then
> > > leads
> > > to us tripping over the assert in tsubst_default_argument that verifies
> > > the
> > > formal parameter type and the function type are consistent.
> > >
> > > Assuming it's too late at this stage to fix the substitution bug, we can
> > > still
> > > relax the assertion like so. Tested on x86_64-pc-linux-gnu, does this
> > > look
> > > OK?
> >
> > This is core issues 1001/1322, which have not been resolved. Clang does the
> > substitution the way you suggest; EDG rejects the testcase because the two
> > substitutions produce different results. I think it would make sense to
> > follow the EDG behavior until this issue is actually resolved.
>
> Here is what I have so far towards that end. When substituting into the
> PARM_DECLs of a function decl, we now additionally check if the
> aforementioned Core issues are relevant and issue a (fatal) diagnostic
> if so. This patch checks this in tsubst_decl rather
> than in tsubst_function_decl for efficiency reasons, so that we don't
> have to perform another traversal over the DECL_ARGUMENTS /
> TYPE_ARG_TYPES just to implement this check.
>
> Is something like this what you have in mind?
Oops, noticed a few bugs in the patch as soon as I hit send. Please
consider this patch instead, which compared to the previous patch
declares unqual_expanded_types in the correct scope and refrains from
doing a potentially incorrect CSE of "cp_type_quals (type)".
-- >8 --
gcc/cp/ChangeLog:
Core issues 1001 and 1322
PR c++/92010
* pt.c (tsubst_decl) : Detect and reject the case where
the function type depends on whether we strip top-level qualifiers from
the type of T before or after substitution.
gcc/testsuite/ChangeLog:
Core issues 1001 and 1322
PR c++/92010
* g++.dg/template/array33.C: New test.
* g++.dg/template/array34.C: New test.
---
gcc/cp/pt.c | 68 +
gcc/testsuite/g++.dg/template/array33.C | 39 ++
gcc/testsuite/g++.dg/template/array34.C | 63 +++
3 files changed, 170 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/template/array33.C
create mode 100644 gcc/testsuite/g++.dg/template/array34.C
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 8564eb11df4..a1efaef2c1e 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -14046,6 +14046,8 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
}
}
+ tree unqual_expanded_types = NULL_TREE;
+
/* Loop through all of the parameters we'll build. When T is
a function parameter pack, LEN is the number of expanded
types in EXPANDED_TYPES; otherwise, LEN is 1. */
@@ -14072,6 +14074,72 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
complain)
/* We're dealing with a normal parameter. */
type = tsubst (TREE_TYPE (t), args, complain, in_decl);
+ /* Determine whether the function type after substitution into the
+ type of the function parameter T depends on the resolution of
+ Core issues 1001/1322, which have not been resolved. In
+ particular, the following detects when the resulting function
+ type depends on whether we strip top-level qualifiers from the
+ type of T before or after substitution.
+
+ This can happen only when the dependent type of T is a
+ cv-qualified wildcard type, and substitution yields a
+ (cv-qualified) array type before array-to-pointer conversion. */
+ if (TREE_CODE (type) == ARRAY_TYPE
+ && cv_qualified_p (type)
+ && DECL_FUNCTION_SCOPE_P (t)
+ && !DECL_TEMPLATE_PARM_P (t))
+ {
+ tree type_pattern = (PACK_EXPANSION_P (TREE_TYPE (t))
+? PACK_EXPANSION_PATTERN (TREE_TYPE (t))
+: TREE_TYPE (t));
+
Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]
On Thu, 26 Mar 2020, Jason Merrill wrote:
> On 3/22/20 9:21 PM, Patrick Palka wrote:
> > This patch relaxes an assertion in tsubst_default_argument that exposes a
> > latent
> > bug in how we substitute an array type into a cv-qualified wildcard function
> > parameter type. Concretely, the latent bug is that given the function
> > template
> >
> >template void foo(const T t);
> >
> > one would expect the type of foo to be void(const int*), but we
> > (seemingly prematurely) strip function parameter types of their top-level
> > cv-qualifiers when building the function's TYPE_ARG_TYPES, and instead end
> > up
> > obtaining void(int*) as the type of foo after substitution and
> > decaying.
> >
> > We still however correctly substitute into and decay the formal parameter
> > type,
> > obtaining const int* as the type of t after substitution. But this then
> > leads
> > to us tripping over the assert in tsubst_default_argument that verifies the
> > formal parameter type and the function type are consistent.
> >
> > Assuming it's too late at this stage to fix the substitution bug, we can
> > still
> > relax the assertion like so. Tested on x86_64-pc-linux-gnu, does this look
> > OK?
>
> This is core issues 1001/1322, which have not been resolved. Clang does the
> substitution the way you suggest; EDG rejects the testcase because the two
> substitutions produce different results. I think it would make sense to
> follow the EDG behavior until this issue is actually resolved.
Here is what I have so far towards that end. When substituting into the
PARM_DECLs of a function decl, we now additionally check if the
aforementioned Core issues are relevant and issue a (fatal) diagnostic
if so. This patch checks this in tsubst_decl rather
than in tsubst_function_decl for efficiency reasons, so that we don't
have to perform another traversal over the DECL_ARGUMENTS /
TYPE_ARG_TYPES just to implement this check.
Is something like this what you have in mind?
-- >8 --
Subject: [PATCH] c++: Reject some instantiations that depend on resolution of
Core issues 1001/1322
gcc/cp/ChangeLog:
Core issues 1001 and 1322
PR c++/92010
* pt.c (tsubst_decl) : Detect and reject the case where
the function type depends on whether we strip top-level qualifiers from
the type of T before or after substitution.
gcc/testsuite/ChangeLog:
Core issues 1001 and 1322
PR c++/92010
* g++.dg/template/array33.C: New test.
* g++.dg/template/array34.C: New test.
---
gcc/cp/pt.c | 70 -
gcc/testsuite/g++.dg/template/array33.C | 39 ++
gcc/testsuite/g++.dg/template/array34.C | 63 ++
3 files changed, 171 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/template/array33.C
create mode 100644 gcc/testsuite/g++.dg/template/array34.C
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 8564eb11df4..fd99053df36 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -14072,9 +14072,77 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
complain)
/* We're dealing with a normal parameter. */
type = tsubst (TREE_TYPE (t), args, complain, in_decl);
+ const int type_quals = cp_type_quals (type);
+
+ /* Determine whether the function type after substitution into the
+ type of the function parameter T depends on the resolution of
+ Core issues 1001/1322, which have not been resolved. In
+ particular, the following detects the case where the resulting
+ function type depends on whether we strip top-level qualifiers
+ from the type of T before or after substitution.
+
+ This can happen only when the dependent type of T is a
+ cv-qualified wildcard type, and substitution yields a
+ (cv-qualified) array type before array-to-pointer conversion. */
+ tree unqual_expanded_types = NULL_TREE;
+ if (TREE_CODE (type) == ARRAY_TYPE
+ && (type_quals & (TYPE_QUAL_CONST|TYPE_QUAL_VOLATILE))
+ && DECL_FUNCTION_SCOPE_P (t)
+ && !DECL_TEMPLATE_PARM_P (t))
+ {
+ tree type_pattern = (PACK_EXPANSION_P (TREE_TYPE (t))
+? PACK_EXPANSION_PATTERN (TREE_TYPE (t))
+: TREE_TYPE (t));
+ if (WILDCARD_TYPE_P (type_pattern)
+ && cv_qualified_p (type_pattern))
+ {
+ /* Substitute into the corresponding wildcard type that is
+ stripped of its own top-level cv-qualifiers. */
+ tree unqual_type;
+ if (PACK_EXPANSION_P (TREE_TYPE (t)))
+ {
+ if (unqual_expanded_types == NULL_TREE)
+ {
+ tree
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Mar 26, 2020, at 3:00 PM, Maciej W. Rozycki wrote: > > I have actually considered extracting the bits already, but I hesitated > putting that forward that as having looked at the part that we require I > have thought it to be very messy: Yeah, sometimes it's like that. I glanced at the work, if you think it's a step forward, I'd support importing it, my take, import from upstream isn't a bad way to go. > So I am in favour of retaining the mechanism rather than using my earlier > proposal, however I'm in two minds as to how to proceed. Integrating the > change as it is will make us having clutter left in the tree after `make > distclean', but we can do it right away. I'd support this. distclean is one rm -rf away from being clean enough. I'd not let that gate or hold up the import. If there is work that we want that's more to do with in tree building and testing (sys root fun, multilibs), while not ideal, we can deviate from upstream to support that. Though, if there is a way to naturally support that, that upstream can accept, that'd be better.
Re: [PATCH] Define TRY_EMPTY_VM_SPACE for riscv64-linux
On Sun, Mar 29, 2020 at 3:03 PM Andreas Schwab wrote: > * config/host-linux.c (TRY_EMPTY_VM_SPACE) [__riscv && __LP64__]: > Define. Looks like the same address already used by the aarch64 and ia64 ports, so it seems safe. OK. Jim
Re: [PATCH], Make PowerPC -mcpu=future enable -mpcrel on linux ELFv2
On Fri, 2020-03-27 at 21:31 -0400, Michael Meissner via Gcc-patches
wrote:
Hi,
A few cosmetic nits and comments sprinkled in below. I defer to
Segher for his approvals and comments. thanks,
-Will
> This is a revised version of the patch I posted on March 23rd. The
> changes are
> to update the comments and improve the ChangeLog.
Missing the description of what the patch actually does.
(From Mar 23):
> It makes
> -mpcrel the default on Linux 64-bit systems that use ELF v2, use the medium
> code mode, and if the user did not disable prefixed load/store instructions
> for
> -mcpu=future.
> There were no regressions when I did the bootstrap and make check
> steps. I
> verified that -mcpu=future does turn on -mprecl if you are targeting
> a Linux
-mpcrel
> ELF v2 system and use the medium code model. Can I check this into
> the master
> branch?
>
> 2020-03-27 Michael Meissner
>
> * config/rs6000/linux64.h (PCREL_SUPPORTED_BY_OS): New macro.
> * config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Do
> not
> set -mprefixed here.
> * config/rs6000/rs6000.c (PCREL_SUPPORTED_BY_OS): New macro.
> (rs6000_option_override_internal): Set the -mprefixed and
> -mpcrel
> options for -mcpu=future if these options can be used.
>
s/can be used/are supported by the platform/ ?
> --- /tmp/JVBhAf_linux64.h 2020-03-27 16:27:05.478619500 -0400
> +++ gcc/config/rs6000/linux64.h 2020-03-27 16:21:56.268876616
> -0400
> @@ -640,3 +640,11 @@ extern int dot_symbols;
> enabling the __float128 keyword. */
> #undef TARGET_FLOAT128_ENABLE_TYPE
> #define TARGET_FLOAT128_ENABLE_TYPE 1
> +
> +/* Enable default support for PC-relative addressing on the 'future'
> system if
> + we can use the PC-relative instructions. Currently this support
> only exits
exists
> + for the ELF v2 object file format using the medium code
> model. */
should that be "s/object file format/ABI/" ?
> +#undef PCREL_SUPPORTED_BY_OS
> +#define PCREL_SUPPORTED_BY_OS(TARGET_FUTURE &&
> TARGET_PREFIXED \
> + && ELFv2_ABI_CHECK
> \
> + && (TARGET_CMODEL == CMODEL_MEDIUM))
> --- /tmp/KyQOUN_rs6000-cpus.def 2020-03-27 16:27:05.488619427
> -0400
> +++ gcc/config/rs6000/rs6000-cpus.def 2020-03-27 16:23:51.780030238
> -0400
> @@ -75,11 +75,11 @@
>| OPTION_MASK_P8_VECTOR\
>| OPTION_MASK_P9_VECTOR)
>
> -/* Support for a future processor's features. Do not enable -mpcrel
> until it
> - is fully functional. */
> +/* Support for a future processor's features. We do not set -mpcrel
> or
> + -mprefixed here. These bits are set in rs6000_option_override if
> the system
> + supports those options. */
I'm still not sure the comment here is actually necessary, there are
many other places where we also do not set -mpcrel or -mprefixed. If
history of the code here requires a hint to point at those options
being set in rs6000_option_override, then it's fine.
> #define ISA_FUTURE_MASKS_SERVER (ISA_3_0_MASKS_SERVER \
> - | OPTION_MASK_FUTURE \
> - | OPTION_MASK_PREFIXED)
> + | OPTION_MASK_FUTURE)
>
> /* Flags that need to be turned off if -mno-future. */
> #define OTHER_FUTURE_MASKS (OPTION_MASK_PCREL
> \
> --- /tmp/z4Mwhm_rs6000.c 2020-03-27 16:27:05.500619340 -0400
> +++ gcc/config/rs6000/rs6000.c2020-03-27 16:20:13.066641659
> -0400
> @@ -98,6 +98,12 @@
> #endif
> #endif
>
> +/* Set up the defaults for whether PC-relative addressing is
> supported by the
> + target system. */
> +#ifndef PCREL_SUPPORTED_BY_OS
> +#define PCREL_SUPPORTED_BY_OS0
> +#endif
> +
> /* Support targetm.vectorize.builtin_mask_for_load. */
> tree altivec_builtin_mask_for_load;
>
> @@ -4014,6 +4020,11 @@ rs6000_option_override_internal (bool gl
>rs6000_isa_flags &= ~OPTION_MASK_FLOAT128_HW;
> }
>
> + /* Enable -mprefixed by default on 64-bit 'future' systems. */
> + if (TARGET_FUTURE && TARGET_POWERPC64
> + && (rs6000_isa_flags_explicit & OPTION_MASK_PREFIXED) == 0)
> +rs6000_isa_flags |= OPTION_MASK_PREFIXED;
> +
>/* -mprefixed (and hence -mpcrel) requires -mcpu=future. */
>if (TARGET_PREFIXED && !TARGET_FUTURE)
> {
> @@ -4175,6 +4186,11 @@ rs6000_option_override_internal (bool gl
>rs6000_isa_flags &= ~OPTION_MASK_PCREL;
> }
>
> + /* If the OS has support for PC-relative relocations, enable it now. */
> + if (PCREL_SUPPORTED_BY_OS
> + && (rs6000_isa_flags_explicit & OPTION_MASK_PCREL) == 0)
> +rs6000_isa_flags |= OPTION_MASK_PCREL;
> +
>if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET)
> rs6000_print_isa_options (stderr, 0, "after subtarget",
> rs6000_isa_flags);
>
Ok.
Re: [PATCH] lower-subreg: PR94123, SVN r273240, causes gcc.target/powerpc/pr87507.c to fail
On Mon, Mar 30, 2020 at 11:23:12AM -0500, Peter Bergner wrote: > On 3/30/20 6:26 AM, Segher Boessenkool wrote: > > On Mon, Mar 30, 2020 at 09:50:05AM +0100, Richard Sandiford wrote: > >> Peter Bergner via Gcc-patches writes: > >>> - if (HARD_REGISTER_NUM_P (rd) || HARD_REGISTER_NUM_P (rs)) > >>> + if (HARD_REGISTER_NUM_P (rd)) > >>> return false; > >>> > >>>b = reg_copy_graph[rs]; > >> > >> I guess this would also work if we dropped the rd check instead. > >> So how about s/||/&&/ instead, to avoid the assymetry? > >> > >> I agree something like this is a better fix long-term, since we > >> shouldn't be relying on make_more_copies outside combine. > > > > Yes; on the other hand, most RTL passes should do something to not have > > hard registers forwarded into non-move instructions (where they can > > cause problems later). (make_more_copies itself is a technicality > > specific to how combine works, and we might be able to drop it in the > > future). > > I kind of agree with Richard above on making it more applicable/symmetric, > but why can't we just remove the HARD_REGISTER_NUM_P() tests altogether? > It's not like lower-subreg is extending hard register lifetime usage than > what is already there in the rtl. We're just decomposing what's already > there into smaller register sized chunks. Is there a problem with that > I'm not aware of? The function comment is (since day 1): /* If SET is a copy from one multi-word pseudo-register to another, record that in reg_copy_graph. Return whether it is such a copy. */ so a) that needs fixing; and b) what does this change for the (single) caller of find_pseudo_copy? The comment there isn't very enlightening: /* We mark pseudo-to-pseudo copies as decomposable during the second pass only. The first pass is so early that there is good chance such moves will be optimized away completely by subsequent optimizations anyway. However, we call find_pseudo_copy even during the first pass so as to properly set up the reg_copy_graph. */ (The function *name* should change as well, if you make this change). Segher
Re: [PATCH] lower-subreg: PR94123, SVN r273240, causes gcc.target/powerpc/pr87507.c to fail
On 3/30/20 11:23 AM, Peter Bergner wrote: > I kind of agree with Richard above on making it more applicable/symmetric, > but why can't we just remove the HARD_REGISTER_NUM_P() tests altogether? > It's not like lower-subreg is extending hard register lifetime usage than > what is already there in the rtl. We're just decomposing what's already > there into smaller register sized chunks. Is there a problem with that > I'm not aware of? ...or maybe there was an issue when combine used to extend hard register lifetimes (which it doesn't anymore) and the test above was just a workaround? Peter
Re: [PATCH] lower-subreg: PR94123, SVN r273240, causes gcc.target/powerpc/pr87507.c to fail
On 3/30/20 6:26 AM, Segher Boessenkool wrote: > On Mon, Mar 30, 2020 at 09:50:05AM +0100, Richard Sandiford wrote: >> Peter Bergner via Gcc-patches writes: >>> - if (HARD_REGISTER_NUM_P (rd) || HARD_REGISTER_NUM_P (rs)) >>> + if (HARD_REGISTER_NUM_P (rd)) >>> return false; >>> >>>b = reg_copy_graph[rs]; >> >> I guess this would also work if we dropped the rd check instead. >> So how about s/||/&&/ instead, to avoid the assymetry? >> >> I agree something like this is a better fix long-term, since we >> shouldn't be relying on make_more_copies outside combine. > > Yes; on the other hand, most RTL passes should do something to not have > hard registers forwarded into non-move instructions (where they can > cause problems later). (make_more_copies itself is a technicality > specific to how combine works, and we might be able to drop it in the > future). I kind of agree with Richard above on making it more applicable/symmetric, but why can't we just remove the HARD_REGISTER_NUM_P() tests altogether? It's not like lower-subreg is extending hard register lifetime usage than what is already there in the rtl. We're just decomposing what's already there into smaller register sized chunks. Is there a problem with that I'm not aware of? Peter
Re: [PATCH V3][gcc] libgccjit: introduce version entry points
On Sun, 2020-03-29 at 21:31 +0100, Andrea Corallo wrote: > David Malcolm writes: > > > On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote: > > Please add the new test to the header in its alphabetical location, > > i.e. between: > > > > /* test-vector-types.cc: We don't use this, since it's C++. */ > > > > and > > > > /* test-volatile.c */ > > > > An entry also needs to be added to the "testcases" array at the end > > of > > the header (again, in the alphabetical-sorted location). > > I tried adding the test into the "testcases" array but this makes the > threads test failing. > > I think this has nothing to do with this patch and happen just > because > this test does not define any code. Infact I see the same happening > just adding "test-empty.c" to the "testcases" array on the current > master. > > The error is not very reproducible, I tried a run under valgrind but > have found nothing so far :/ > > Dave do you recall if there was a specific reason not to have > "test-empty.c" into the "testcases" array? > > Andrea I replied to this as: [PATCH] lra: set insn_code_data to NULL when freeing https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542929.html but forgot to set the reply-to in git send-email; sorry. Dave
Re: [PATCH] lower-subreg: PR94123, SVN r273240, causes gcc.target/powerpc/pr87507.c to fail
Hi!
On Mon, Mar 30, 2020 at 09:50:05AM +0100, Richard Sandiford wrote:
> With this change, the only remaining function of -fsplit-wide-types-early
> is to act as a double lock on one pass. IMO it'd make more sense to remove
> that double lock and make -fsplit-wide-types-early and -fsplit-wide-types
> act as independent options, a bit like -fschedule-insns{,2}.
But then, -fsplit-wide-types would control two passes, one of which is
identical to -fsplit-wide-types-early, just in a different spot in the
pass pipeline. That is bad enough on its own, already :-/
I still think all three passes should be controlled by -fsplit-wide-types,
just as they already are (and the "old" two have been for years).
Segher
[PATCH] lra: set insn_code_data to NULL when freeing
On Sun, 2020-03-29 at 21:31 +0100, Andrea Corallo wrote:
> David Malcolm writes:
>
> > On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote:
> > Please add the new test to the header in its alphabetical location,
> > i.e. between:
> >
> > /* test-vector-types.cc: We don't use this, since it's C++. */
> >
> > and
> >
> > /* test-volatile.c */
> >
> > An entry also needs to be added to the "testcases" array at the end
> > of
> > the header (again, in the alphabetical-sorted location).
>
> I tried adding the test into the "testcases" array but this makes the
> threads test failing.
>
> I think this has nothing to do with this patch and happen just
> because
> this test does not define any code. Infact I see the same happening
> just adding "test-empty.c" to the "testcases" array on the current
> master.
>
> The error is not very reproducible, I tried a run under valgrind but
> have found nothing so far :/
>
> Dave do you recall if there was a specific reason not to have
> "test-empty.c" into the "testcases" array?
>
> Andrea
It's a double-free bug in lra.c, albeit one that requires being used
in a multithreaded way from libgccjit to be triggered.
libgccjit's test-threads.c repeatedly compiles and runs numerous tests,
each in a separate thread.
Attempting to add an empty test that generates no code leads to a
double-free ICE within that thread, within lra.c's
finish_insn_code_data_once.
The root cause is that the insn_code_data array is cleared in
init_insn_code_data_once, but this is only called the first time
a cgraph_node is expanded [1], whereas the "loop-over-all-elements
and free them" is unconditionally called in finalize [2]. Hence
if there are no functions:
* the array is not re-initialized for the empty context
* when finish_insn_code_data_once is called for the empty context
it still contains the freed pointers from the previous context
that held the jit mutex, and hence the free is a double-free.
This patch sets the pointers to NULL after freeing them, fixing
the ICE. The calls to free are still guarded by a check for NULL,
which is redundant, but maybe there's a reason for not wanting to
call "free" on a possibly-NULL value many times on process exit?
(it makes the diff cleaner, at least)
Fixes the issue in jit.dg.
Full bootstrap & regression test in progress.
Is it OK for master if it passes?
Thanks
Dave
[1]
init_insn_code_data_once is called via
lra_init_once called by
ira_init_once called by
initialize_rtl, via:
if (!rtl_initialized)
ira_init_once ();
called by init_function_start
called by cgraph_node::expand
[2]:
finish_insn_code_data_once is called by:
lra_finish_once called by
finalize
gcc/ChangeLog:
* lra.c (finish_insn_code_data_once): Set the array elements
to NULL after freeing them.
gcc/testsuite/ChangeLog:
* jit.dg/all-non-failing-tests.h: Add test-empty.c
---
gcc/lra.c| 5 -
gcc/testsuite/jit.dg/all-non-failing-tests.h | 10 ++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/gcc/lra.c b/gcc/lra.c
index d5ea3622686..5e8b75b1fda 100644
--- a/gcc/lra.c
+++ b/gcc/lra.c
@@ -653,7 +653,10 @@ finish_insn_code_data_once (void)
for (unsigned int i = 0; i < NUM_INSN_CODES; i++)
{
if (insn_code_data[i] != NULL)
- free (insn_code_data[i]);
+ {
+ free (insn_code_data[i]);
+ insn_code_data[i] = NULL;
+ }
}
}
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index b2acc74ae95..af744192a73 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -116,6 +116,13 @@
#undef create_code
#undef verify_code
+/* test-empty.c */
+#define create_code create_code_empty
+#define verify_code verify_code_empty
+#include "test-empty.c"
+#undef create_code
+#undef verify_code
+
/* test-error-*.c: We don't use these test cases, since they deliberately
introduce errors, which we don't want here. */
@@ -328,6 +335,9 @@ const struct testcase testcases[] = {
{"expressions",
create_code_expressions,
verify_code_expressions},
+ {"empty",
+ create_code_empty,
+ verify_code_empty},
{"factorial",
create_code_factorial,
verify_code_factorial},
--
2.21.0
Re: [PATCH V2][wwwdocs] Document GNU-stack support added to GCC 10 for MIPS
On Mon, 30 Mar 2020, Dragan Mladjenovic wrote: > Thanks. I forgot to mention. I would need someone to commit this for me. I'll take care. Gerald
Re: [PATCH V2][wwwdocs] Document GNU-stack support added to GCC 10 for MIPS
On 3/30/20 5:49 PM, Dragan Mladjenovic wrote: I won't be able to access to my sourceware account for some time. Hi. Done as 47f1af75a082fe6e4d2bd4b289d982abd749c824. Martin
Re: [PATCH] i386: Fix up *one_cmplv*2* insn with avx512f [PR94343]
On Fri, 2020-03-27 at 00:46 +0100, Jakub Jelinek wrote:
> Hi!
>
> This define_insn has two issues.
> One is that with -mavx512f -mno-avx512vl it can emit an AVX512VL-only insn
> - 128-bit or 256-bit EVEX encoded vpternlog{d,q}.
> Another one is that because there is no vpternlog{b,w}, we emit vpternlogd
> instead, but then we shouldn't pretend we support masking of that, because
> we don't.
> The first one can be fixed by forcing the use of %zmm* registers instead of
> %xmm* or %ymm* if AVX512F but not AVX512VL, like we do for a couple of other
> insns (although that is primarily done in order to support %xmm16+ regs).
> But we need to make sure that in that case the input operand isn't memory,
> because while we can read and store the higher bits of registers, we don't
> want to read from memory more bytes than what we should read.
>
> A variant to these two if_then_else set attrs, condition in the output and
> larger condition would be 4 different define_insns (one with something like
> VI48_AVX512VL iterator, masking, no g modifiers and "vm" input constraint,
> another one with VI48_AVX iterator, !TARGET_AVX512VL in condition,
> no masking, g modifiers and "v" input constraint, one with VI12_AVX512VL
> iterator, no masking, no g modifiers and "vm" input constraint and last one
> with
> VI12_AVX2 iterator, !TARGET_AVX512VL in condition, no masking, g modifiers
> and "v" input constraint, but I think having one pattern is shorter than
> that.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2020-03-26 Jakub Jelinek
>
> PR target/94343
> * config/i386/sse.md (one_cmpl2): If
> !TARGET_AVX512VL, use 512-bit vpternlog and make sure the input
> operand is a register. Don't enable masked variants for V*[QH]Imode.
>
> * gcc.target/i386/avx512f-pr94343.c: New test.
> * gcc.target/i386/avx512vl-pr94343.c: New test.
OK
jeff
Re: [PATCH V2][wwwdocs] Document GNU-stack support added to GCC 10 for MIPS
On 3/30/2020 17:39, Jeff Law wrote: On Sun, 2020-03-29 at 22:33 +0200, Dragan Mladjenovic wrote: diff --git a/htdocs/gcc-10/changes.html b/htdocs/gcc-10/changes.html index b5cbcebf..1e1eaf43 100644 --- a/htdocs/gcc-10/changes.html +++ b/htdocs/gcc-10/changes.html @@ -692,7 +692,17 @@ a work-in-progress. - + MIPS + +The mips*-*-linux* targets now mark object files with + appropriate GNU-stack note, facilitating use of non-executable stack + hardening on GNU/Linux. + The soft-float targets have this feature enabled by default, while + for hard-float targets it is required for GCC to be configured with + --with-glibc-version=2.31 + against glibc 2.31 or later. + + OK jeff Thanks. I forgot to mention. I would need someone to commit this for me. I won't be able to access to my sourceware account for some time. Thanks in advance.
Re: [PATCH V2][wwwdocs] Document GNU-stack support added to GCC 10 for MIPS
On Sun, 2020-03-29 at 22:33 +0200, Dragan Mladjenovic wrote: > diff --git a/htdocs/gcc-10/changes.html b/htdocs/gcc-10/changes.html > index b5cbcebf..1e1eaf43 100644 > --- a/htdocs/gcc-10/changes.html > +++ b/htdocs/gcc-10/changes.html > @@ -692,7 +692,17 @@ a work-in-progress. > > > > - > + MIPS > + > +The mips*-*-linux* targets now mark object files with > + appropriate GNU-stack note, facilitating use of non-executable stack > + hardening on GNU/Linux. > + The soft-float targets have this feature enabled by default, while > + for hard-float targets it is required for GCC to be configured with > + --with-glibc-version=2.31 > + against glibc 2.31 or later. > + > + OK jeff >
Re: [PATCH v2] Fix vextract* masked patterns (PR target/93069)
On Fri, 2020-03-27 at 00:26 +0100, Jakub Jelinek wrote:
> On Wed, Mar 25, 2020 at 05:59:36PM -0600, Jeff Law via Gcc-patches wrote:
> > Sorry. I know you asked me to look at this eons ago, but ever time I just
> > get
> > lost.
> >
> > I get the distinct impression that we could do something much simpler (the
> > patch
> > you initially proposed for backporting to the release branches). Perhaps we
> > go
> > with that now and the full patch for gcc-11?
>
> So like this?
> Bootstrapped/regtested on x86_64-linux and i686-linux.
>
> 2020-03-26 Jakub Jelinek
>
> PR target/93069
> * config/i386/sse.md (vec_extract_lo_): Use
>instead of m in output operand constraint.
> (vec_extract_hi_): Use instead of
> %{%3%}.
>
> * gcc.target/i386/avx512vl-pr93069.c: New test.
> * gcc.dg/vect/pr93069.c: New test.
Yea. And consider the more complete fix approved for gcc-11.
jeff
>
Re: [PATCH] Fix scan pattern of vect-8.f90 dump.
On Mon, 2020-03-30 at 16:06 +0200, Martin Liška wrote: > Hi. > > I would like to relax scanned pattern, see the PR. > > Ready to be installed? > Thanks, > Martin > > gcc/testsuite/ChangeLog: > > 2020-03-30 Martin Liska > > PR testsuite/94402 > * gfortran.dg/vect/vect-8.f90: Allow 22 or 23 loops > to be vectorized (based on libmvec presence). OK jeff >
Re: [PATCH] XFAIL pr57193.c test-case.
On Mon, 2020-03-30 at 14:10 +0200, Martin Liška wrote: > Hi. > > I would like to XFAIL the mentioned test-case. It fails > for quite some time and it has PR created. > > Ready to be installed? > Thanks, > Martin > > gcc/testsuite/ChangeLog: > > 2020-03-30 Martin Liska > > PR rtl-optimization/87716 > * gcc.target/i386/pr57193.c: XFAIL a test-case. Yea, it's been failing for over a year and as you note, we've got a BZ for it. OK jeff
[PATCH] Fix scan pattern of vect-8.f90 dump.
Hi.
I would like to relax scanned pattern, see the PR.
Ready to be installed?
Thanks,
Martin
gcc/testsuite/ChangeLog:
2020-03-30 Martin Liska
PR testsuite/94402
* gfortran.dg/vect/vect-8.f90: Allow 22 or 23 loops
to be vectorized (based on libmvec presence).
---
gcc/testsuite/gfortran.dg/vect/vect-8.f90 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/testsuite/gfortran.dg/vect/vect-8.f90 b/gcc/testsuite/gfortran.dg/vect/vect-8.f90
index 6f10c7e586f..b10c4fc0a97 100644
--- a/gcc/testsuite/gfortran.dg/vect/vect-8.f90
+++ b/gcc/testsuite/gfortran.dg/vect/vect-8.f90
@@ -705,5 +705,5 @@ RETURN
END SUBROUTINE kernel
! { dg-final { scan-tree-dump-times "vectorized 23 loops" 1 "vect" { target aarch64*-*-* } } }
-! { dg-final { scan-tree-dump-times "vectorized 22 loops" 1 "vect" { target { vect_intdouble_cvt && { ! aarch64*-*-* } } } } }
+! { dg-final { scan-tree-dump-times "vectorized 2\[23\] loops" 1 "vect" { target { vect_intdouble_cvt && { ! aarch64*-*-* } } } } }
! { dg-final { scan-tree-dump-times "vectorized 17 loops" 1 "vect" { target { { ! vect_intdouble_cvt } && { ! aarch64*-*-* } } } } }
Re: [RFC] Fix for pr64706 testcase faulre
Hello, On Thu, 26 Mar 2020, Jan Hubicka wrote: > > I think we should continue to try to model ELF semantics re weak and > > aliases. If so, then yes, LTO gets it wrong and the above testcase should > > not abort. Weak doesn't enter the picture for creating aliases (the > > aliases is created with the declaration, and we require an available > > definition, and the alias is henceforth bound to _that_ very definition). > > I.e. the 'a.c:b' name should continue to refer to the same code snippet > > identified by the a.c:a name, not be redirected to the overriding b.c:a. > > > > I'm wondering about the amount of code necessary to fix this. I think > > that points towards a wrong level of representation somewhere, and perhaps > > _that_ should be changed instead of trying to work around it. > > > > For instance, right now aliases are different from non-aliases, whereas in > > reality there's no difference: there are simply names referring to code or > > data snippets, and it so happens that for some snippets there are multiple > > names, and it further just so happens that some names for a code snippet > > become overridden/removed by other names for other code snippets, which > > doesn't invalidate the fact that there still is another name for the first > > snippet. > > > > If it were modelled like that in cgraph/lto all the rest would naturally > > follow. (Of course you would need tracking of when some code/data > > snippets can't be referred to by name anymore (and hence are useless), > > because all names for them went away, or none of the existing names is > > used anymore, but I don't think that materially would change much in our > > internal data structures). > > Yep, the trouble is caused by fact that GCC representation is not quite > what linker does. I.e. we bind function bodies with their declarations > and declarations with symbols. We are bouit about assumption of 1to1 > correspondence between function bodies and their symbols. This is not > true because one body may have multiple aliases but also it may define > mutliple different symbols (alternative entry points & labels). > > Reorganizing this design is quite some work. > > We need to change trees/GIMPLE to use symbols instead of DECLs when > referring to them. So parameter of CALL_EXPR would not be decl but > symbol associated with it. > > We to move everyting that is property of symbol away from decl into > symbols (i.e. assembler name, visibility, section etc) and thus we would > need to change everything from frontends all the way to RTL. I'm not sure I agree that it's that complicated. With the above you essentially do away with the need of DECLs at all. calls would refer to your fat symbols, symbols would refer to code/data bodies. Data (e.g. vtables) would also refer to symbols. Nowhere would be a DECL in that chain. At that point you can just as well call your new symbols "DECLs". The crucial part is merely that the DECLs/fat-symbols can change their association with code/data blobs. (Well, I can see that some properties of code blobs, for instance the API, and hence the (function) type, is inherent to the code blob, and therefore should be 1to1 associated with it, some there's some argument to be made to retain some info of the DECL-as-now to be put tighly to the code blob representation). > I am gradually shuffling things in this direction (I plan to move > assembler names to symbols and gradually do that for visibilitie so we > can have cross-module referneces to labels and finally support static > initializers taking addresses of them), but the process is slow and I > think it makes sense to first fix correctness issues with what we have > rahter than waiting for major surgery to be finished. True. I was just surprised by the size of this change, many diff pluses, few minuses :) Ciao, Michael.
Re: [PATCH] Respect user align for local variable
Hi Jakub: Thanks for your correction, I read the doc for the aligned attribute again[1], it's minimum alignment not restricted alignment, I thought it should honor to the alignment attribute, Richard Biener suggested[2] should put an assertion here to make sure never decrease alignment here, so I'll send another patch for add assertion. [1] https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542808.html On Mon, Mar 30, 2020 at 7:21 PM Jakub Jelinek wrote: > > On Mon, Mar 30, 2020 at 06:31:27PM +0800, Kito Cheng wrote: > > gcc/ChangeLog > > > > * cfgexpand.c (align_local_variable): Check DECL_USER_ALIGN. > > Why? LOCAL_DECL_ALIGNMENT surely shouldn't decrease alignment of decls > with DECL_USER_ALIGN vars (but do you have evidence that it does), > but it is just fine to increase alignment of vars with explicit alignment > requirements if the compiler thinks it is beneficial. Larger alignment > still satisfies the user requirement... > > Jakub >
[PATCH] XFAIL pr57193.c test-case.
Hi.
I would like to XFAIL the mentioned test-case. It fails
for quite some time and it has PR created.
Ready to be installed?
Thanks,
Martin
gcc/testsuite/ChangeLog:
2020-03-30 Martin Liska
PR rtl-optimization/87716
* gcc.target/i386/pr57193.c: XFAIL a test-case.
---
gcc/testsuite/gcc.target/i386/pr57193.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gcc/testsuite/gcc.target/i386/pr57193.c b/gcc/testsuite/gcc.target/i386/pr57193.c
index 5c6d7e495de..25c69a2b024 100644
--- a/gcc/testsuite/gcc.target/i386/pr57193.c
+++ b/gcc/testsuite/gcc.target/i386/pr57193.c
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-options "-O2 -msse2 -mno-sse3 -mtune=generic" } */
-/* { dg-final { scan-assembler-times "movdqa" 2 } } */
+/* XFAIL the test due to PR87716. */
+/* { dg-final { scan-assembler-times "movdqa" 2 { xfail *-*-* } } } */
#include
Re: [PATCH] ICE: in vectorizable_load, at tree-vect-stmts.c:9173
"Yangfei (Felix)" writes: > Hi! >> -Original Message- >> From: Yangfei (Felix) >> Sent: Monday, March 30, 2020 5:28 PM >> To: gcc-patches@gcc.gnu.org >> Cc: 'rguent...@suse.de' >> Subject: [PATCH] ICE: in vectorizable_load, at tree-vect-stmts.c:9173 >> >> Hi, >> >> New bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94398 >> >> With -mstrict-align, aarch64_builtin_support_vector_misalignment will returns >> false when misalignment factor is unknown at compile time. >> Then vect_supportable_dr_alignment returns dr_unaligned_unsupported, >> which triggers the ICE. I have pasted the call trace on the bug report. >> >> vect_supportable_dr_alignment is expected to return either dr_aligned or >> dr_unaligned_supported for masked operations. >> But it seems that this function only catches internal_fn IFN_MASK_LOAD & >> IFN_MASK_STORE. >> We are emitting a mask gather load here for this test case. >> As backends have their own vector misalignment support policy, I am supposing >> this should be better handled in the auto-vect shared code. >> > > I can only reply to comment on the bug here as my account got locked by the > bugzilla system for now. Sorry to hear that. What was the reason? > The way Richard (rsandifo) suggests also works for me and I agree it should > be more consistent and better for compile time. > One question here: when will a IFN_MASK_LOAD/IFN_MASK_STORE be passed to > vect_supportable_dr_alignment? I'd expect that to happen in the same cases as for unmasked load/stores. It certainly will for unconditional loads and stores that get masked via full-loop masking. In principle, the same rules apply to both masked and unmasked accesses. But for SVE, both masked and unmasked accesses should support misalignment, which is why I think the current target hook is probably wrong for SVE + -mstrict-align. > @@ -8051,8 +8051,15 @@ vectorizable_store (stmt_vec_info stmt_info, > gimple_stmt_iterator *gsi, >auto_vec dr_chain (group_size); >oprnds.create (group_size); > > - alignment_support_scheme > -= vect_supportable_dr_alignment (first_dr_info, false); > + /* Strided accesses perform only component accesses, alignment > + is irrelevant for them. */ > + if (STMT_VINFO_STRIDED_P (first_dr_info->stmt) > + && !STMT_VINFO_GROUPED_ACCESS (first_dr_info->stmt)) I think this should be based on memory_access_type == VMAT_GATHER_SCATTER instead. At this point, STMT_VINFO_* describes properties of the original scalar access(es) while memory_access_type describes the vector implementation strategy. It's the latter that matters here. Same thing for loads. Thanks, Richard > +alignment_support_scheme = dr_unaligned_supported; > + else > +alignment_support_scheme > + = vect_supportable_dr_alignment (first_dr_info, false); > + >gcc_assert (alignment_support_scheme); >vec_loop_masks *loop_masks > = (loop_vinfo && LOOP_VINFO_FULLY_MASKED_P (loop_vinfo) > @@ -9168,8 +9175,15 @@ vectorizable_load (stmt_vec_info stmt_info, > gimple_stmt_iterator *gsi, >ref_type = reference_alias_ptr_type (DR_REF (first_dr_info->dr)); > } > > - alignment_support_scheme > -= vect_supportable_dr_alignment (first_dr_info, false); > + /* Strided accesses perform only component accesses, alignment > + is irrelevant for them. */ > + if (STMT_VINFO_STRIDED_P (first_dr_info->stmt) > + && !STMT_VINFO_GROUPED_ACCESS (first_dr_info->stmt)) > +alignment_support_scheme = dr_unaligned_supported; > + else > +alignment_support_scheme > + = vect_supportable_dr_alignment (first_dr_info, false); > + >gcc_assert (alignment_support_scheme); >vec_loop_masks *loop_masks > = (loop_vinfo && LOOP_VINFO_FULLY_MASKED_P (loop_vinfo)
RE: [PATCH] ICE: in vectorizable_load, at tree-vect-stmts.c:9173
Hi! > -Original Message- > From: Yangfei (Felix) > Sent: Monday, March 30, 2020 5:28 PM > To: gcc-patches@gcc.gnu.org > Cc: 'rguent...@suse.de' > Subject: [PATCH] ICE: in vectorizable_load, at tree-vect-stmts.c:9173 > > Hi, > > New bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94398 > > With -mstrict-align, aarch64_builtin_support_vector_misalignment will returns > false when misalignment factor is unknown at compile time. > Then vect_supportable_dr_alignment returns dr_unaligned_unsupported, > which triggers the ICE. I have pasted the call trace on the bug report. > > vect_supportable_dr_alignment is expected to return either dr_aligned or > dr_unaligned_supported for masked operations. > But it seems that this function only catches internal_fn IFN_MASK_LOAD & > IFN_MASK_STORE. > We are emitting a mask gather load here for this test case. > As backends have their own vector misalignment support policy, I am supposing > this should be better handled in the auto-vect shared code. > I can only reply to comment on the bug here as my account got locked by the bugzilla system for now. The way Richard (rsandifo) suggests also works for me and I agree it should be more consistent and better for compile time. One question here: when will a IFN_MASK_LOAD/IFN_MASK_STORE be passed to vect_supportable_dr_alignment? New patch: diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c index 12beef6..2825023 100644 --- a/gcc/tree-vect-stmts.c +++ b/gcc/tree-vect-stmts.c @@ -8051,8 +8051,15 @@ vectorizable_store (stmt_vec_info stmt_info, gimple_stmt_iterator *gsi, auto_vec dr_chain (group_size); oprnds.create (group_size); - alignment_support_scheme -= vect_supportable_dr_alignment (first_dr_info, false); + /* Strided accesses perform only component accesses, alignment + is irrelevant for them. */ + if (STMT_VINFO_STRIDED_P (first_dr_info->stmt) + && !STMT_VINFO_GROUPED_ACCESS (first_dr_info->stmt)) +alignment_support_scheme = dr_unaligned_supported; + else +alignment_support_scheme + = vect_supportable_dr_alignment (first_dr_info, false); + gcc_assert (alignment_support_scheme); vec_loop_masks *loop_masks = (loop_vinfo && LOOP_VINFO_FULLY_MASKED_P (loop_vinfo) @@ -9168,8 +9175,15 @@ vectorizable_load (stmt_vec_info stmt_info, gimple_stmt_iterator *gsi, ref_type = reference_alias_ptr_type (DR_REF (first_dr_info->dr)); } - alignment_support_scheme -= vect_supportable_dr_alignment (first_dr_info, false); + /* Strided accesses perform only component accesses, alignment + is irrelevant for them. */ + if (STMT_VINFO_STRIDED_P (first_dr_info->stmt) + && !STMT_VINFO_GROUPED_ACCESS (first_dr_info->stmt)) +alignment_support_scheme = dr_unaligned_supported; + else +alignment_support_scheme + = vect_supportable_dr_alignment (first_dr_info, false); + gcc_assert (alignment_support_scheme); vec_loop_masks *loop_masks = (loop_vinfo && LOOP_VINFO_FULLY_MASKED_P (loop_vinfo)
Re: [PATCH] lower-subreg: PR94123, SVN r273240, causes gcc.target/powerpc/pr87507.c to fail
Hi!
On Mon, Mar 30, 2020 at 09:50:05AM +0100, Richard Sandiford wrote:
> Peter Bergner via Gcc-patches writes:
> > - if (HARD_REGISTER_NUM_P (rd) || HARD_REGISTER_NUM_P (rs))
> > + if (HARD_REGISTER_NUM_P (rd))
> > return false;
> >
> >b = reg_copy_graph[rs];
>
> I guess this would also work if we dropped the rd check instead.
> So how about s/||/&&/ instead, to avoid the assymetry?
>
> I agree something like this is a better fix long-term, since we
> shouldn't be relying on make_more_copies outside combine.
Yes; on the other hand, most RTL passes should do something to not have
hard registers forwarded into non-move instructions (where they can
cause problems later). (make_more_copies itself is a technicality
specific to how combine works, and we might be able to drop it in the
future).
> With this change, the only remaining function of -fsplit-wide-types-early
> is to act as a double lock on one pass. IMO it'd make more sense to remove
> that double lock and make -fsplit-wide-types-early and -fsplit-wide-types
> act as independent options, a bit like -fschedule-insns{,2}.
Sure, that would simplify things a bit (at least conceptually).
With or without that change, the documentation could use some tweaking
as well, after this patch.
Segher
Re: [PATCH] Respect user align for local variable
On Mon, Mar 30, 2020 at 06:31:27PM +0800, Kito Cheng wrote: > gcc/ChangeLog > > * cfgexpand.c (align_local_variable): Check DECL_USER_ALIGN. Why? LOCAL_DECL_ALIGNMENT surely shouldn't decrease alignment of decls with DECL_USER_ALIGN vars (but do you have evidence that it does), but it is just fine to increase alignment of vars with explicit alignment requirements if the compiler thinks it is beneficial. Larger alignment still satisfies the user requirement... Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mon, Mar 30, 2020 at 01:09:41PM +0200, Richard Biener wrote: > Btw, does the above fallout already happen if you add -g? Because > all the affected stmt-lists should end up with some DEBUG_BEGIN_STMTs > in them and thus preserved? Such STATEMENT_LISTs were marked !TREE_SIDE_EFFECTS, which means e.g. save_expr doesn't save those etc. Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mon, 30 Mar 2020, Jakub Jelinek wrote:
> On Mon, Mar 30, 2020 at 12:44:40PM +0200, Richard Biener wrote:
> > >type = TREE_TYPE (stmt_expr);
> > > + /* For statement-expressions, force the STATEMENT_LIST
> > > + to be preserved with side-effects, even if it contains
> > > + just one statement or no statements. See PR93786. */
> > > + TREE_SIDE_EFFECTS (stmt_expr) = 0;
> > >result = pop_stmt_list (stmt_expr);
> > > + gcc_assert (result == stmt_expr);
> > > + TREE_SIDE_EFFECTS (result) = 1;
> >
> > The original code had !TREE_SIDE_EFFECT stmt lists (empty ones?) returned
> > directly which you above change to have side-effects - are you sure
> > the fallout is not due to that? Did you look into any of the fallout?
>
> Some of the fallout has been from missed optimizations e.g. in initializers,
> where the TREE_SIDE_EFFECTs (sometimes for empty statement exprs like
> ({ ; ; ; }), at other times for one with a single real statement in there
> which didn't have side-effects) is with the patch now set when it wasn't
> before, but there were ICEs too.
I see. I was thinking of
if (TREE_SIDE_EFFECTS (stmt_expr))
{
TREE_SIDE_EFFECTS (stmt_expr) = 0;
result = pop_stmt_list (stmt_expr);
gcc_assert (result == stmt_expr);
TREE_SIDE_EFFECTS (result) = 1;
}
else
result = pop_stmt_list (stmt_expr);
which should make ({ ; ; ; }) work again, but obviously not the
single-stmt case.
> I guess another option is what Alex wrote, just throw the DEBUG_BEGIN_STMTs
> away if there would be just those or those plus a single other statement,
> perhaps limited to statement expressions (i.e. do it in the above spot).
Sure, that's another option - but of course only short-term since we
then end up with "bad" debug info.
Btw, does the above fallout already happen if you add -g? Because
all the affected stmt-lists should end up with some DEBUG_BEGIN_STMTs
in them and thus preserved?
Richard.
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mon, Mar 30, 2020 at 12:44:40PM +0200, Richard Biener wrote:
> >type = TREE_TYPE (stmt_expr);
> > + /* For statement-expressions, force the STATEMENT_LIST
> > + to be preserved with side-effects, even if it contains
> > + just one statement or no statements. See PR93786. */
> > + TREE_SIDE_EFFECTS (stmt_expr) = 0;
> >result = pop_stmt_list (stmt_expr);
> > + gcc_assert (result == stmt_expr);
> > + TREE_SIDE_EFFECTS (result) = 1;
>
> The original code had !TREE_SIDE_EFFECT stmt lists (empty ones?) returned
> directly which you above change to have side-effects - are you sure
> the fallout is not due to that? Did you look into any of the fallout?
Some of the fallout has been from missed optimizations e.g. in initializers,
where the TREE_SIDE_EFFECTs (sometimes for empty statement exprs like
({ ; ; ; }), at other times for one with a single real statement in there
which didn't have side-effects) is with the patch now set when it wasn't
before, but there were ICEs too.
I guess another option is what Alex wrote, just throw the DEBUG_BEGIN_STMTs
away if there would be just those or those plus a single other statement,
perhaps limited to statement expressions (i.e. do it in the above spot).
Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mon, 30 Mar 2020, Jakub Jelinek wrote: > On Fri, Mar 27, 2020 at 09:09:42PM +0100, Richard Biener wrote: > > Sounds worth a try. > > Unfortunately that FAILed miserably: > +FAIL: g++.dg/cpp0x/constexpr-object1.C -std=c++11 (test for excess errors) > +FAIL: g++.dg/cpp0x/constexpr-object1.C -std=c++14 (test for excess errors) > +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++11 scan-assembler rodata > +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++14 scan-assembler rodata > +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++17 scan-assembler rodata > +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++2a scan-assembler rodata > +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++11 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++14 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++17 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++2a scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++11 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++14 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++17 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++2a scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/pr78765.C -std=c++11 (test for excess errors) > +FAIL: g++.dg/cpp0x/pr78765.C -std=c++14 (test for excess errors) > +FAIL: g++.dg/cpp0x/pr78765.C -std=c++17 (test for excess errors) > +FAIL: g++.dg/cpp0x/pr78765.C -std=c++2a (test for excess errors) > +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++14 scan-assembler-not > static_init > +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++17 scan-assembler-not > static_init > +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++2a scan-assembler-not > static_init > +FAIL: g++.dg/cpp1z/decomp3.C (test for excess errors) > +FAIL: g++.dg/cpp1z/elide2.C -std=c++11 execution test > +FAIL: g++.dg/cpp1z/elide2.C -std=c++14 execution test > +FAIL: g++.dg/cpp1z/elide2.C -std=c++17 execution test > +FAIL: g++.dg/cpp1z/elide2.C -std=c++2a execution test > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++11 (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++11 (test for excess errors) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++14 (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++14 (test for excess errors) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++17 (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++17 (test for excess errors) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++2a (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++2a (test for excess errors) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++98 (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++98 (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++11 (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++14 (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++17 (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++2a (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++98 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++11 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++11 17 (test for errors, line 6) > +FAIL: g++.dg/parse/error26.C -std=gnu++14 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++14 17 (test for errors, line 6) > +FAIL: g++.dg/parse/error26.C -std=gnu++17 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++17 17 (test for errors, line 6) > +FAIL: g++.dg/parse/error26.C -std=gnu++2a (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++2a 17 (test for errors, line 6) > +FAIL: g++.dg/parse/error26.C -std=gnu++98 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++98 17 (test for errors, line 6) > +FAIL: g++.dg/tree-ssa/pr69336.C (test for excess errors) > +UNRESOLVED: g++.dg/tree-ssa/pr69336.C scan-tree-dump-not optimized "cmap" > +FAIL: g++.dg/torture/pr45709-2.C -O1 (internal compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O1 (test for excess errors) > +FAIL: g++.dg/torture/pr45709-2.C -O2 (internal compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O2 (test for excess errors) > +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto (internal compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto (test for excess errors) > +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto -flto-partition=none (internal > compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto -flto-partition=none (test for > excess errors) > +FAIL: g++.dg/torture/pr45709-2.C -O3 -g (internal compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O3 -g (test for excess errors) > +FAIL:
Re: [PATCH] Fix PR94043 by making vect_live_op generate lc-phi
On Mon, Mar 30, 2020 at 12:24 PM Kewen.Lin wrote: > > Hi, > > As PR94043 shows, my commit r10-4524 exposed one issue in > vectorizable_live_operation, which inserts one extra BB > before the single exit, leading unexpected operand expansion > and unexpected loop depth assertion. As Richi suggested, > this patch is to teach vectorizable_live_operation to > generate loop closed phi for vec_lhs, it looks like: > loop; > # lhs' = PHI > => > loop; > # vec_lhs' = PHI > new_tree = BIT_FIELD_REF ; > lhs' = new_tree; > > I noticed that there are some SLP cases that have same lhs > and vec_lhs but different offsets, which can make us have > more PHIs for the same vec_lhs there. But I think it would > be fine since only one of them is actually live, the others > should be eliminated by the following dce. So the patch > doesn't check whether there is one phi for vec_lhs, just > create one directly instead. > > Bootstrapped/regtested on powerpc64le-linux-gnu (LE) P8. > > Is it ok for trunk? OK. Thanks, Richard. > BR, > Kewen > --- > > gcc/ChangeLog > > 2020-MM-DD Kewen Lin > > PR tree-optimization/94043 > * tree-vect-loop.c (vectorizable_live_operation): Generate loop-closed > phi for vec_lhs and use it for lane extraction. > > gcc/testsuite/ChangeLog > > 2020-MM-DD Kewen Lin > > PR tree-optimization/94043 > * gfortran.dg/graphite/vect-pr94043.f90: New test. >
[PATCH] Respect user align for local variable
gcc/ChangeLog
* cfgexpand.c (align_local_variable): Check DECL_USER_ALIGN.
---
gcc/cfgexpand.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index a7ec77d5c85..19a020b4b97 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -369,13 +369,19 @@ align_local_variable (tree decl, bool really_expand)
align = TYPE_ALIGN (TREE_TYPE (decl));
else
{
- align = LOCAL_DECL_ALIGNMENT (decl);
- /* Don't change DECL_ALIGN when called from estimated_stack_frame_size.
-That is done before IPA and could bump alignment based on host
-backend even for offloaded code which wants different
-LOCAL_DECL_ALIGNMENT. */
- if (really_expand)
- SET_DECL_ALIGN (decl, align);
+ if (DECL_USER_ALIGN (decl))
+ align = DECL_ALIGN (decl);
+ else
+ {
+ align = LOCAL_DECL_ALIGNMENT (decl);
+ /* Don't change DECL_ALIGN when called from
+estimated_stack_frame_size.
+That is done before IPA and could bump alignment based on host
+backend even for offloaded code which wants different
+LOCAL_DECL_ALIGNMENT. */
+ if (really_expand)
+ SET_DECL_ALIGN (decl, align);
+ }
}
return align / BITS_PER_UNIT;
}
--
2.25.2
Re: [PATCH 2/2] Fix alignment for local variable [PR90811]
On Mon, Mar 30, 2020 at 06:24:32PM +0800, Kito Cheng wrote: > Hi Jakub: > > I saw the omp and oacc related passes are in the head of all_passes, > so I plan added after pass_omp_target_link, does it late enough? Yes, that is ok. > diff --git a/gcc/passes.def b/gcc/passes.def > index 2bf2cb78fc5..92cbe587a8a 100644 > --- a/gcc/passes.def > +++ b/gcc/passes.def > @@ -183,6 +183,7 @@ along with GCC; see the file COPYING3. If not see > NEXT_PASS (pass_oacc_device_lower); > NEXT_PASS (pass_omp_device_lower); > NEXT_PASS (pass_omp_target_link); > + NEXT_PASS (pass_adjust_alignment); > NEXT_PASS (pass_all_optimizations); > PUSH_INSERT_PASSES_WITHIN (pass_all_optimizations) > NEXT_PASS (pass_remove_cgraph_callee_edges); Jakub
Re: [PATCH 2/2] Fix alignment for local variable [PR90811]
Hi Andrew:
> > + FOR_EACH_FUNCTION_WITH_GIMPLE_BODY (node)
> > +{
> > + function *fun = node->get_fun ();
> > + FOR_EACH_LOCAL_DECL (fun, i, var)
> > + {
> > + align = LOCAL_DECL_ALIGNMENT (var);
> > +
> > + SET_DECL_ALIGN (var, align);
>
>
> I think this is wrong if the user has set the alignment already.
> You need to check DECL_USER_ALIGN.
Good point, maybe we need to fix align_local_variable too :)
Thanks.
Re: [PATCH 2/2] Fix alignment for local variable [PR90811]
Hi Jakub: I saw the omp and oacc related passes are in the head of all_passes, so I plan added after pass_omp_target_link, does it late enough? diff --git a/gcc/passes.def b/gcc/passes.def index 2bf2cb78fc5..92cbe587a8a 100644 --- a/gcc/passes.def +++ b/gcc/passes.def @@ -183,6 +183,7 @@ along with GCC; see the file COPYING3. If not see NEXT_PASS (pass_oacc_device_lower); NEXT_PASS (pass_omp_device_lower); NEXT_PASS (pass_omp_target_link); + NEXT_PASS (pass_adjust_alignment); NEXT_PASS (pass_all_optimizations); PUSH_INSERT_PASSES_WITHIN (pass_all_optimizations) NEXT_PASS (pass_remove_cgraph_callee_edges); Thanks :) On Sat, Mar 28, 2020 at 2:27 AM Jakub Jelinek wrote: > > On Sat, Mar 28, 2020 at 02:06:56AM +0800, Kito Cheng wrote: > > PR target/90811 > > * ipa-increase-alignment.cc (increase_alignment_local_var): New. > > (increase_alignment_global_var): New. > > (pass_ipa_increase_alignment::gate): Remove. > > I'm afraid this is too early, pass_ipa_increase_alignment is part of > all_small_ipa_passes list, those are run before the LTO streaming. > See cgraphunit.c (ipa_passes). > For OpenMP/OpenACC offloading, this has to run after the streaming, > which means either a pass in the all_regular_ipa_passes list, or much better > (I don't see any advantage of this being an IPA pass) some new pass that is > run very early in the all_passes list. > > At which point I don't really see the benefit of moving this into a new file > either. > > Jakub >
[PATCH] Fix PR94043 by making vect_live_op generate lc-phi
Hi,
As PR94043 shows, my commit r10-4524 exposed one issue in
vectorizable_live_operation, which inserts one extra BB
before the single exit, leading unexpected operand expansion
and unexpected loop depth assertion. As Richi suggested,
this patch is to teach vectorizable_live_operation to
generate loop closed phi for vec_lhs, it looks like:
loop;
# lhs' = PHI
=>
loop;
# vec_lhs' = PHI
new_tree = BIT_FIELD_REF ;
lhs' = new_tree;
I noticed that there are some SLP cases that have same lhs
and vec_lhs but different offsets, which can make us have
more PHIs for the same vec_lhs there. But I think it would
be fine since only one of them is actually live, the others
should be eliminated by the following dce. So the patch
doesn't check whether there is one phi for vec_lhs, just
create one directly instead.
Bootstrapped/regtested on powerpc64le-linux-gnu (LE) P8.
Is it ok for trunk?
BR,
Kewen
---
gcc/ChangeLog
2020-MM-DD Kewen Lin
PR tree-optimization/94043
* tree-vect-loop.c (vectorizable_live_operation): Generate loop-closed
phi for vec_lhs and use it for lane extraction.
gcc/testsuite/ChangeLog
2020-MM-DD Kewen Lin
PR tree-optimization/94043
* gfortran.dg/graphite/vect-pr94043.f90: New test.
diff --git a/gcc/testsuite/gfortran.dg/graphite/vect-pr94043.f90
b/gcc/testsuite/gfortran.dg/graphite/vect-pr94043.f90
new file mode 100644
index 000..744c0f3042e
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/graphite/vect-pr94043.f90
@@ -0,0 +1,18 @@
+! { dg-do compile }
+! { dg-additional-options "-O3 -ftree-parallelize-loops=2 -fno-tree-dce" }
+
+! As PR94043, test it to be compiled successfully without ICE.
+
+program yw
+ integer :: hx(6, 6)
+ integer :: ps = 1, e2 = 1
+
+ do ps = 1, 6
+do e2 = 1, 6
+hx(e2, ps) = 0
+if (ps >= 5 .and. e2 >= 5) then
+hx(e2, ps) = hx(1, 1)
+end if
+end do
+ end do
+end program
diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 53fccb715ef..eb607f3aab0 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -7998,6 +7998,25 @@ vectorizable_live_operation (stmt_vec_info stmt_info,
bitstart = int_const_binop (MINUS_EXPR, vec_bitsize, bitsize);
}
+ /* Ensure the VEC_LHS for lane extraction stmts satisfy loop-closed PHI
+ requirement, insert one phi node for it. It looks like:
+loop;
+ BB:
+# lhs' = PHI
+ ==>
+loop;
+ BB:
+# vec_lhs' = PHI
+new_tree = lane_extract ;
+lhs' = new_tree; */
+
+ basic_block exit_bb = single_exit (loop)->dest;
+ gcc_assert (single_pred_p (exit_bb));
+
+ tree vec_lhs_phi = copy_ssa_name (vec_lhs);
+ gimple *phi = create_phi_node (vec_lhs_phi, exit_bb);
+ SET_PHI_ARG_DEF (phi, single_exit (loop)->dest_idx, vec_lhs);
+
gimple_seq stmts = NULL;
tree new_tree;
if (LOOP_VINFO_FULLY_MASKED_P (loop_vinfo))
@@ -8010,10 +8029,10 @@ vectorizable_live_operation (stmt_vec_info stmt_info,
the loop mask for the final iteration. */
gcc_assert (ncopies == 1 && !slp_node);
tree scalar_type = TREE_TYPE (STMT_VINFO_VECTYPE (stmt_info));
- tree mask = vect_get_loop_mask (gsi, _VINFO_MASKS (loop_vinfo),
- 1, vectype, 0);
- tree scalar_res = gimple_build (, CFN_EXTRACT_LAST,
- scalar_type, mask, vec_lhs);
+ tree mask = vect_get_loop_mask (gsi, _VINFO_MASKS (loop_vinfo), 1,
+ vectype, 0);
+ tree scalar_res = gimple_build (, CFN_EXTRACT_LAST, scalar_type,
+ mask, vec_lhs_phi);
/* Convert the extracted vector element to the required scalar type. */
new_tree = gimple_convert (, lhs_type, scalar_res);
@@ -8023,13 +8042,32 @@ vectorizable_live_operation (stmt_vec_info stmt_info,
tree bftype = TREE_TYPE (vectype);
if (VECTOR_BOOLEAN_TYPE_P (vectype))
bftype = build_nonstandard_integer_type (tree_to_uhwi (bitsize), 1);
- new_tree = build3 (BIT_FIELD_REF, bftype, vec_lhs, bitsize, bitstart);
+ new_tree = build3 (BIT_FIELD_REF, bftype, vec_lhs_phi, bitsize,
bitstart);
new_tree = force_gimple_operand (fold_convert (lhs_type, new_tree),
, true, NULL_TREE);
}
if (stmts)
-gsi_insert_seq_on_edge_immediate (single_exit (loop), stmts);
+{
+ gimple_stmt_iterator exit_gsi = gsi_after_labels (exit_bb);
+ gsi_insert_before (_gsi, stmts, GSI_CONTINUE_LINKING);
+
+ /* Remove existing phi from lhs and create one copy from new_tree. */
+ tree lhs_phi = NULL_TREE;
+ gimple_stmt_iterator gsi;
+ for (gsi = gsi_start_phis (exit_bb); !gsi_end_p (gsi); gsi_next ())
+ {
+ gimple *phi = gsi_stmt (gsi);
+ if ((gimple_phi_arg_def (phi, 0) == lhs))
+ {
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Fri, Mar 27, 2020 at 09:09:42PM +0100, Richard Biener wrote: > Sounds worth a try. Unfortunately that FAILed miserably: +FAIL: g++.dg/cpp0x/constexpr-object1.C -std=c++11 (test for excess errors) +FAIL: g++.dg/cpp0x/constexpr-object1.C -std=c++14 (test for excess errors) +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++11 scan-assembler rodata +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++14 scan-assembler rodata +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++17 scan-assembler rodata +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++2a scan-assembler rodata +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++11 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++14 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++17 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++2a scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++11 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++14 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++17 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++2a scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/pr78765.C -std=c++11 (test for excess errors) +FAIL: g++.dg/cpp0x/pr78765.C -std=c++14 (test for excess errors) +FAIL: g++.dg/cpp0x/pr78765.C -std=c++17 (test for excess errors) +FAIL: g++.dg/cpp0x/pr78765.C -std=c++2a (test for excess errors) +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++14 scan-assembler-not static_init +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++17 scan-assembler-not static_init +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++2a scan-assembler-not static_init +FAIL: g++.dg/cpp1z/decomp3.C (test for excess errors) +FAIL: g++.dg/cpp1z/elide2.C -std=c++11 execution test +FAIL: g++.dg/cpp1z/elide2.C -std=c++14 execution test +FAIL: g++.dg/cpp1z/elide2.C -std=c++17 execution test +FAIL: g++.dg/cpp1z/elide2.C -std=c++2a execution test +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++11 (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++11 (test for excess errors) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++14 (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++14 (test for excess errors) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++17 (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++17 (test for excess errors) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++2a (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++2a (test for excess errors) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++98 (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++98 (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++11 (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++14 (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++17 (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++2a (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++98 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++11 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++11 17 (test for errors, line 6) +FAIL: g++.dg/parse/error26.C -std=gnu++14 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++14 17 (test for errors, line 6) +FAIL: g++.dg/parse/error26.C -std=gnu++17 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++17 17 (test for errors, line 6) +FAIL: g++.dg/parse/error26.C -std=gnu++2a (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++2a 17 (test for errors, line 6) +FAIL: g++.dg/parse/error26.C -std=gnu++98 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++98 17 (test for errors, line 6) +FAIL: g++.dg/tree-ssa/pr69336.C (test for excess errors) +UNRESOLVED: g++.dg/tree-ssa/pr69336.C scan-tree-dump-not optimized "cmap" +FAIL: g++.dg/torture/pr45709-2.C -O1 (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O1 (test for excess errors) +FAIL: g++.dg/torture/pr45709-2.C -O2 (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O2 (test for excess errors) +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto (test for excess errors) +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto -flto-partition=none (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto -flto-partition=none (test for excess errors) +FAIL: g++.dg/torture/pr45709-2.C -O3 -g (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O3 -g (test for excess errors) +FAIL: g++.dg/torture/pr45709.C -O1 (internal compiler error) +FAIL: g++.dg/torture/pr45709.C -O1 (test for excess errors) +FAIL: g++.dg/torture/pr45709.C -O2 (internal compiler error) +FAIL: g++.dg/torture/pr45709.C -O2
Re: [PATCH 2/2] Fix alignment for local variable [PR90811]
> But I wonder if we can instead fix the memcpy inlining issue by > making the predicates involved honor LOCAL_ALIGNMENT > instead of relying on DECL_ALIGN? The memcpy inlining issue is not the only one affected by alignment issue, I guess? So I think it would be better to fix DECL_ALIGN? On Mon, Mar 30, 2020 at 5:15 PM Jakub Jelinek wrote: > > On Mon, Mar 30, 2020 at 11:09:40AM +0200, Richard Biener wrote: > > On Fri, Mar 27, 2020 at 7:27 PM Jakub Jelinek wrote: > > > > > > On Sat, Mar 28, 2020 at 02:06:56AM +0800, Kito Cheng wrote: > > > > PR target/90811 > > > > * ipa-increase-alignment.cc (increase_alignment_local_var): New. > > > > (increase_alignment_global_var): New. > > > > (pass_ipa_increase_alignment::gate): Remove. > > > > > > I'm afraid this is too early, pass_ipa_increase_alignment is part of > > > all_small_ipa_passes list, those are run before the LTO streaming. > > > See cgraphunit.c (ipa_passes). > > > For OpenMP/OpenACC offloading, this has to run after the streaming, > > > which means either a pass in the all_regular_ipa_passes list, or much > > > better > > > (I don't see any advantage of this being an IPA pass) some new pass that > > > is > > > run very early in the all_passes list. > > > > It's an IPA pass because we IPA propagate alignment. > > For global vars sure, but we are talking here about automatic vars, and the > offloading targets really have serious problems dealing with the 16/32 byte > alignment forced from x86 backend. > > Jakub >
[PATCH] ICE: in vectorizable_load, at tree-vect-stmts.c:9173
Hi,
New bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94398
With -mstrict-align, aarch64_builtin_support_vector_misalignment will returns
false when misalignment factor is unknown at compile time.
Then vect_supportable_dr_alignment returns dr_unaligned_unsupported, which
triggers the ICE. I have pasted the call trace on the bug report.
vect_supportable_dr_alignment is expected to return either dr_aligned or
dr_unaligned_supported for masked operations.
But it seems that this function only catches internal_fn IFN_MASK_LOAD &
IFN_MASK_STORE.
We are emitting a mask gather load here for this test case.
As backends have their own vector misalignment support policy, I am supposing
this should be better handled in the auto-vect shared code.
Proposed fix:
diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c
index 0192aa6..67d3345 100644
--- a/gcc/tree-vect-data-refs.c
+++ b/gcc/tree-vect-data-refs.c
@@ -6509,11 +6509,26 @@ vect_supportable_dr_alignment (dr_vec_info *dr_info,
/* For now assume all conditional loads/stores support unaligned
access without any special code. */
- if (gcall *stmt = dyn_cast (stmt_info->stmt))
-if (gimple_call_internal_p (stmt)
- && (gimple_call_internal_fn (stmt) == IFN_MASK_LOAD
- || gimple_call_internal_fn (stmt) == IFN_MASK_STORE))
- return dr_unaligned_supported;
+ gcall *call = dyn_cast (stmt_info->stmt);
+ if (call && gimple_call_internal_p (call))
+{
+ internal_fn ifn = gimple_call_internal_fn (call);
+ switch (ifn)
+ {
+ case IFN_MASK_LOAD:
+ case IFN_MASK_LOAD_LANES:
+ case IFN_MASK_GATHER_LOAD:
+ case IFN_MASK_STORE:
+ case IFN_MASK_STORE_LANES:
+ case IFN_MASK_SCATTER_STORE:
+ return dr_unaligned_supported;
+ default:
+ break;
+ }
+}
+
+ if (loop_vinfo && LOOP_VINFO_FULLY_MASKED_P (loop_vinfo))
+return dr_unaligned_supported;
if (loop_vinfo)
{
Suggestions?
Thanks,
Felix
Re: [PATCH] Check DECL_CONTEXT of new/delete operators.
On Mon, 30 Mar 2020, Martin Liška wrote: The patch ensures that a deleted new/delete pair has a same context. That will fix the issue presented in the PR. DECL_CONTEXT seems good for that example (assuming it is still available in the middle-end), but shouldn't we also check if both are array versions? I can build a similar example by implementing the global operator new[] in terms of operator new and operator delete[] in terms of operator delete. Checking if both are aligned versions could be nice as well. I don't know what others can be relevant, I think we are allowed to mix throw and nothrow, or sized and non-sized versions. For DECL_CONTEXT, if we use new on a derived class and delete on a base class, I guess it is sensible not to optimize unless devirt / inlining manage to show a matched pair of operators, so your patch looks good here. I understand this feature is getting rather painful, sorry... -- Marc Glisse
Re: [PATCH 2/2] Fix alignment for local variable [PR90811]
On Mon, Mar 30, 2020 at 11:09:40AM +0200, Richard Biener wrote: > On Fri, Mar 27, 2020 at 7:27 PM Jakub Jelinek wrote: > > > > On Sat, Mar 28, 2020 at 02:06:56AM +0800, Kito Cheng wrote: > > > PR target/90811 > > > * ipa-increase-alignment.cc (increase_alignment_local_var): New. > > > (increase_alignment_global_var): New. > > > (pass_ipa_increase_alignment::gate): Remove. > > > > I'm afraid this is too early, pass_ipa_increase_alignment is part of > > all_small_ipa_passes list, those are run before the LTO streaming. > > See cgraphunit.c (ipa_passes). > > For OpenMP/OpenACC offloading, this has to run after the streaming, > > which means either a pass in the all_regular_ipa_passes list, or much better > > (I don't see any advantage of this being an IPA pass) some new pass that is > > run very early in the all_passes list. > > It's an IPA pass because we IPA propagate alignment. For global vars sure, but we are talking here about automatic vars, and the offloading targets really have serious problems dealing with the 16/32 byte alignment forced from x86 backend. Jakub
Re: [PATCH 2/2] Fix alignment for local variable [PR90811]
On Fri, Mar 27, 2020 at 7:27 PM Jakub Jelinek wrote: > > On Sat, Mar 28, 2020 at 02:06:56AM +0800, Kito Cheng wrote: > > PR target/90811 > > * ipa-increase-alignment.cc (increase_alignment_local_var): New. > > (increase_alignment_global_var): New. > > (pass_ipa_increase_alignment::gate): Remove. > > I'm afraid this is too early, pass_ipa_increase_alignment is part of > all_small_ipa_passes list, those are run before the LTO streaming. > See cgraphunit.c (ipa_passes). > For OpenMP/OpenACC offloading, this has to run after the streaming, > which means either a pass in the all_regular_ipa_passes list, or much better > (I don't see any advantage of this being an IPA pass) some new pass that is > run very early in the all_passes list. It's an IPA pass because we IPA propagate alignment. I fear that offload targets have to agree on local variable alignment if we want to go this way (update DECL_ALIGN). But I wonder if we can instead fix the memcpy inlining issue by making the predicates involved honor LOCAL_ALIGNMENT instead of relying on DECL_ALIGN? > > At which point I don't really see the benefit of moving this into a new file > either. > > Jakub >
Re: [PATCH] Check DECL_CONTEXT of new/delete operators.
On Mon, Mar 30, 2020 at 10:41 AM Martin Liška wrote: > > Hi. > > The patch ensures that a deleted new/delete pair has a same context. > That will fix the issue presented in the PR. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. I think this will break the DCE with LTO since we strip quite some DECL_CONTEXT in free-lang-data. Honza - do you remember why we strip DECL_CONTEXT for methods which will most likely keep their containing record types live through their this arguments? Quoting: /* We need to keep field decls associated with their trees. Otherwise tree merging may merge some fileds and keep others disjoint wich in turn will not do well with TREE_CHAIN pointers linking them. Also do not drop containing types for virtual methods and tables because these are needed by devirtualization. C++ destructors are special because C++ frontends sometimes produces virtual destructor as an alias of non-virtual destructor. In devirutalization code we always walk through aliases and we need context to be preserved too. See PR89335 */ if (TREE_CODE (decl) != FIELD_DECL && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL) || (!DECL_VIRTUAL_P (decl) && (TREE_CODE (decl) != FUNCTION_DECL || !DECL_CXX_DESTRUCTOR_P (decl) DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl)); and fld_decl_context stripping up to the next non-TYPE context (unless VLA). Richard. > Ready to be installed? > Thanks, > Martin > > gcc/ChangeLog: > > 2020-03-30 Martin Liska > > PR c++/94314 > * tree-ssa-dce.c (propagate_necessity): Verify that > DECL_CONTEXT of a new/delete operators do match. > > gcc/testsuite/ChangeLog: > > 2020-03-30 Martin Liska > > PR c++/94314 > * g++.dg/pr94314.C: New test. > --- > gcc/testsuite/g++.dg/pr94314.C | 84 ++ > gcc/tree-ssa-dce.c | 31 + > 2 files changed, 105 insertions(+), 10 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/pr94314.C > >
Re: [PATCH] lower-subreg: PR94123, SVN r273240, causes gcc.target/powerpc/pr87507.c to fail
Peter Bergner via Gcc-patches writes:
> The pr87507.c test case regressed due to Segher's commit that added
> -fsplit-wide-types-early. The issue is that the lower-subreg pass only
> decomposes the TImode code in the example if there is a pseudo reg to pseudo
> reg copy. When the lower-subreg pass is called late (its old location),
> then combine changes the generated code by adding a TImode pseudo reg to
> pseudo reg copy and lower-subreg successfully decomposes it.
>
> When we run lower-subreg before combine, that copy isn't there so we
> do not decompose our TImode uses. I'm not sure why we require a pseudo
> to pseudo copy before we decompose things, but changing find_pseudo_copy
> to allow pseudo and hard regs to be copied into a pseudo like below fixes
> the issue.
>
> Ian, do you remember why we don't just decompose all wide types and instead
> require a pseudo to pseudo copy to exist?
>
> This patch survived bootstrap and regtesting on powerpc64le-linux with
> no regressions.
>
> * lower-subreg.c (find_pseudo_copy): Allow copies from hard registers
> too.
>
> diff --git a/gcc/lower-subreg.c b/gcc/lower-subreg.c
> index 4c8bc835f93..c6816a34489 100644
> --- a/gcc/lower-subreg.c
> +++ b/gcc/lower-subreg.c
> @@ -419,7 +419,7 @@ find_pseudo_copy (rtx set)
>
>rd = REGNO (dest);
>rs = REGNO (src);
> - if (HARD_REGISTER_NUM_P (rd) || HARD_REGISTER_NUM_P (rs))
> + if (HARD_REGISTER_NUM_P (rd))
> return false;
>
>b = reg_copy_graph[rs];
I guess this would also work if we dropped the rd check instead.
So how about s/||/&&/ instead, to avoid the assymetry?
I agree something like this is a better fix long-term, since we
shouldn't be relying on make_more_copies outside combine.
> Given we're late in stage4, the above patch (assuming it's ok) probably
> shouldn't go in until stage1, since it is changing lower-subreg's behaviour
> slightly.
>
> A different (ie, safer) approach would be to just rerun lower-subreg at
> its old location, regardless of whether we used -fsplit-wide-types-early.
> This way, we are not changing lower-subreg's behaviour, just running it an
> extra time (3 times instead of twice when using -fsplit-wide-types-early).
> I don't think lower-subreg is too expensive to run an extra time and we'd
> only do it when using -fsplit-wide-types-early. The only downside (if any)
> is that we don't decompose these TImode uses early like the patch above does,
> so combine, etc. can't see what they will eventually become. This does fix
> the bug and also survives bootstrap and regtesting on powerpc64le-linux
> with no regressions.
>
> * lower-subreg.c (pass_lower_subreg3::gate): Remove test for
> flag_split_wide_types_early.
>
> diff --git a/gcc/lower-subreg.c b/gcc/lower-subreg.c
> index 4c8bc835f93..807ad398b64 100644
> --- a/gcc/lower-subreg.c
> +++ b/gcc/lower-subreg.c
> @@ -1844,8 +1844,7 @@ public:
>{}
>
>/* opt_pass methods: */
> - virtual bool gate (function *) { return flag_split_wide_types
> - && !flag_split_wide_types_early; }
> + virtual bool gate (function *) { return flag_split_wide_types != 0; }
>virtual unsigned int execute (function *)
> {
>decompose_multiword_subregs (true);
Looks good to me with the s/ != 0// that Segher mentioned.
With this change, the only remaining function of -fsplit-wide-types-early
is to act as a double lock on one pass. IMO it'd make more sense to remove
that double lock and make -fsplit-wide-types-early and -fsplit-wide-types
act as independent options, a bit like -fschedule-insns{,2}.
Thanks,
Richard
[PATCH] Check DECL_CONTEXT of new/delete operators.
Hi.
The patch ensures that a deleted new/delete pair has a same context.
That will fix the issue presented in the PR.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
Thanks,
Martin
gcc/ChangeLog:
2020-03-30 Martin Liska
PR c++/94314
* tree-ssa-dce.c (propagate_necessity): Verify that
DECL_CONTEXT of a new/delete operators do match.
gcc/testsuite/ChangeLog:
2020-03-30 Martin Liska
PR c++/94314
* g++.dg/pr94314.C: New test.
---
gcc/testsuite/g++.dg/pr94314.C | 84 ++
gcc/tree-ssa-dce.c | 31 +
2 files changed, 105 insertions(+), 10 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/pr94314.C
diff --git a/gcc/testsuite/g++.dg/pr94314.C b/gcc/testsuite/g++.dg/pr94314.C
new file mode 100644
index 000..76cd9d8d2a4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314.C
@@ -0,0 +1,84 @@
+/* PR c++/94314. */
+/* { dg-options "-O2 -fdump-tree-cddce-details" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include
+
+struct A
+{
+ __attribute__((malloc,noinline))
+ static void* operator new(unsigned long sz)
+ {
+++count;
+return ::operator new(sz);
+ }
+
+ static void operator delete(void* ptr)
+ {
+--count;
+::operator delete(ptr);
+ }
+
+ static int count;
+};
+
+int A::count = 0;
+
+struct B
+{
+ __attribute__((malloc,noinline))
+ static void* operator new(unsigned long sz)
+ {
+++count;
+return ::operator new(sz);
+ }
+
+ __attribute__((noinline))
+ static void operator delete(void* ptr)
+ {
+--count;
+::operator delete(ptr);
+ }
+
+ static int count;
+};
+
+int B::count = 0;
+
+struct C
+{
+ static void* operator new(unsigned long sz)
+ {
+++count;
+return ::operator new(sz);
+ }
+
+ static void operator delete(void* ptr)
+ {
+--count;
+::operator delete(ptr);
+ }
+
+ static int count;
+};
+
+int C::count = 0;
+
+int main(){
+ delete new A;
+ if (A::count != 0)
+__builtin_abort ();
+
+ delete new B;
+ if (B::count != 0)
+__builtin_abort ();
+
+ delete new C;
+ if (C::count != 0)
+__builtin_abort ();
+
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Deleting : operator delete" 1 "cddce1"} } */
+/* { dg-final { scan-tree-dump-times "Deleting : B::operator delete" 1 "cddce1"} } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index e4077b58890..d86234ead23 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -824,16 +824,27 @@ propagate_necessity (bool aggressive)
|| DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
|| DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
{
- /* Delete operators can have alignment and (or) size as next
- arguments. When being a SSA_NAME, they must be marked
- as necessary. */
- if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
- for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
- {
- tree arg = gimple_call_arg (stmt, i);
- if (TREE_CODE (arg) == SSA_NAME)
- mark_operand_necessary (arg);
- }
+ if (is_delete_operator)
+ {
+ /* Verify that new and delete operators have the same
+ context. */
+ if (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)
+ && (DECL_CONTEXT (def_callee)
+ != DECL_CONTEXT (gimple_call_fndecl (stmt
+ mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+ /* Delete operators can have alignment and (or) size
+ as next arguments. When being a SSA_NAME, they
+ must be marked as necessary. */
+ if (gimple_call_num_args (stmt) >= 2)
+ for (unsigned i = 1; i < gimple_call_num_args (stmt);
+ i++)
+ {
+ tree arg = gimple_call_arg (stmt, i);
+ if (TREE_CODE (arg) == SSA_NAME)
+ mark_operand_necessary (arg);
+ }
+ }
continue;
}
Re: [Patch][Fortran] Fix error cleanup of select rank (PR93522)
Early *ping*. Tobias On 3/27/20 11:06 AM, Tobias Burnus wrote: Hi all, here, the reject_statement cleanup and the freeing of the namespace both remove the symbol. Solution: Remove it first, then clean the namespace – then the reject_statement has no (deleted) statement to cleanup. As select rank is new, that's again a GCC-10 only regression (of invalid code). OK? Tobias PS: valgrind shows ==71237==definitely lost: 0 bytes in 0 blocks ==71237==indirectly lost: 0 bytes in 0 blocks ==71237== possibly lost: 0 bytes in 0 blocks I did ignore: ==52255==still reachable: 500,682 bytes in 2,181 blocks which is the same also with 'select... end select' commented. - Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Re: [Patch, 9/10 Regression] fortran: ICE in build_entry_thunks PR93814
Hi Mark, the error message does not make sense – and I also currently do not see why that example should be invalid. Regarding the error message: "uses the same global identifier" In the test program (attached or PR) I do see one function "f" and one entry "g" – and both "f" and "g" is only used once. I also checked the F2018 standard and did not spot anything which restricts this example. Hence, I believe the test case is valid and, hence, the patch does not solve the actual problem. Cheers, Tobias On 3/23/20 3:55 PM, Mark Eggleston wrote: Apologies, 2nd attempt: gcc/fortran/ChangeLog: Steven G. Kargl PR fortran/93814 * resolve.c (gfc_verify_binding_labels): Handle symbols with the subroutine attribute separately from symbols with the function attribute. gcc/testsuite/ChanheLog: Mark Eggleston PR fortran/93814 * gfortran.dg/pr93814.f90: New test. OK to commit? On 22/03/2020 17:46, Thomas Koenig wrote: Hi Mark, Please find attached Steve Kargl's fix for PR93814. The attached patch does not match the ChangeLog; it seems to be for PR93484. Regards Thomas - Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Re: [PATCH, 8/9/10 Regression] fortran: ICE equivalence with an element of an array PR94030
OK. Thanks, Tobias On 3/30/20 9:30 AM, Mark Eggleston wrote: Please find attached patch for PR94030. OK to commit? gcc/fortran/ChangeLog: Steven G. Kargl PR fortran/94030 * resolve.c (resolve_equivalence): Correct formatting around the label "identical_types". Instead of using gfc_resolve_array_spec use is_non_constants_shape_array to determine whether the array can be used in a in an equivalence statement. gcc/testsuite/ChangeLog: Mark Eggleston Steven G. Kargl PR fortran/94030 * gfortran.dg/pr94030_1.f90: New test. * gfortran.dg/pr94030_2.f90: New test. - Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Re: [PATCH, 9/10 Regression] fortran : ICE in gfc_resolve_findloc PR93498
OK (both GCC 9 + 10). Thanks for the packaging the patch and to Steven for the patch. Tobias On 3/30/20 9:00 AM, Mark Eggleston wrote: Please find attached patch for PR93498. OK to commit? gcc/fortran/ChangeLog: Steven G. Kargl PR fortran/93498 * check.c (gfc_check_findloc): If the kinds of the arguments differ goto label "incompat". gcc/testsuite/ChangeLog: Mark Eggleston PR fortran/93498 * gfortran.dg/pr93498_1.f90: New test. * gfortran.dg/pr93498_2.f90: New test. - Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
[PATCH, 8/9/10 Regression] fortran: ICE equivalence with an element of an array PR94030
Please find attached patch for PR94030.
OK to commit?
gcc/fortran/ChangeLog:
Steven G. Kargl
PR fortran/94030
* resolve.c (resolve_equivalence): Correct formatting
around the label "identical_types". Instead of using
gfc_resolve_array_spec use is_non_constants_shape_array
to determine whether the array can be used in a in an
equivalence statement.
gcc/testsuite/ChangeLog:
Mark Eggleston
Steven G. Kargl
PR fortran/94030
* gfortran.dg/pr94030_1.f90: New test.
* gfortran.dg/pr94030_2.f90: New test.
--
https://www.codethink.co.uk/privacy.html
>From 28562d99f2bc7c9418baf707c602bbf7aefbeec3 Mon Sep 17 00:00:00 2001
From: Mark Eggleston
Date: Fri, 27 Mar 2020 11:30:40 +
Subject: [PATCH] fortran: ICE equivalence with an element of an array PR94030
Deferred size arrays can not be used in equivalance statements.
gcc/fortran/ChangeLog:
PR fortran/94030
* resolve.c (resolve_equivalence): Correct formatting
around the label "identical_types". Instead of using
gfc_resolve_array_spec use is_non_constants_shape_array
to determine whether the array can be used in a in an
equivalence statement.
gcc/testsuite/ChangeLog:
PR fortran/94030
* gfortran.dg/pr94030_1.f90
* gfortran.dg/pr94030_2.f90
---
gcc/fortran/resolve.c | 6 +++---
gcc/testsuite/gfortran.dg/pr94030_1.f90 | 11 +++
gcc/testsuite/gfortran.dg/pr94030_2.f90 | 33 +
3 files changed, 47 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/pr94030_1.f90
create mode 100644 gcc/testsuite/gfortran.dg/pr94030_2.f90
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 2dcb261fc71..bfafd3de07a 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -16855,7 +16855,8 @@ resolve_equivalence (gfc_equiv *eq)
&& !gfc_notify_std (GFC_STD_GNU, msg, sym->name, >where))
continue;
- identical_types:
+identical_types:
+
last_ts =>ts;
last_where = >where;
@@ -16863,8 +16864,7 @@ resolve_equivalence (gfc_equiv *eq)
continue;
/* Shall not be an automatic array. */
- if (e->ref->type == REF_ARRAY
- && !gfc_resolve_array_spec (e->ref->u.ar.as, 1))
+ if (e->ref->type == REF_ARRAY && is_non_constant_shape_array (sym))
{
gfc_error ("Array %qs at %L with non-constant bounds cannot be "
"an EQUIVALENCE object", sym->name, >where);
diff --git a/gcc/testsuite/gfortran.dg/pr94030_1.f90 b/gcc/testsuite/gfortran.dg/pr94030_1.f90
new file mode 100644
index 000..e63d3cc8da4
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr94030_1.f90
@@ -0,0 +1,11 @@
+! { dg-do compile }
+!
+
+subroutine f(n)
+ integer :: n
+ integer :: arr(n)
+ integer :: i
+ equivalence (i, arr(1))
+end
+
+! { dg-error "Array 'arr' at .1. with non-constant bounds cannot be an EQUIVALENCE object" " " { target *-*-* } 8 }
diff --git a/gcc/testsuite/gfortran.dg/pr94030_2.f90 b/gcc/testsuite/gfortran.dg/pr94030_2.f90
new file mode 100644
index 000..84bfdeaa819
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr94030_2.f90
@@ -0,0 +1,33 @@
+! { dg-do compile }
+!
+! Provided by Steve Kargl.
+
+subroutine foo(n,m)
+ integer, intent(in) :: n, m
+ integer a(n)
+ real b(n)
+ equivalence(a,b)
+ if (m /= 2) then
+ a = 1
+ print *, a(1)
+ else
+ b = 42.
+ print *, b(1)
+ end if
+end subroutine
+
+subroutine bar(m)
+ integer, intent(in) :: m
+ integer x(8)
+ real y(8)
+ equivalence(x,y)
+ if (m /= 2) then
+ x = 1
+ print *, x(1)
+ else
+ y = 42.
+ print *, y(1)
+ end if
+end subroutine
+
+! { dg-error "Array '.' at .1. with non-constant bounds cannot be an EQUIVALENCE object" " " { target *-*-* } 9 }
--
2.11.0
[PATCH, 9/10 Regression] fortran : ICE in gfc_resolve_findloc PR93498
Please find attached patch for PR93498.
OK to commit?
gcc/fortran/ChangeLog:
Steven G. Kargl
PR fortran/93498
* check.c (gfc_check_findloc): If the kinds of the arguments
differ goto label "incompat".
gcc/testsuite/ChangeLog:
Mark Eggleston
PR fortran/93498
* gfortran.dg/pr93498_1.f90: New test.
* gfortran.dg/pr93498_2.f90: New test.
--
https://www.codethink.co.uk/privacy.html
>From 38865feca36f0837f3fea8b401a2b42fb4f818ca Mon Sep 17 00:00:00 2001
From: Mark Eggleston
Date: Thu, 26 Mar 2020 14:07:09 +
Subject: [PATCH] fortran : ICE in gfc_resolve_findloc PR93498
ICE occurs when findloc is used with character arguments of different
kinds. If the character kinds are different reject the code.
Original patch provided by Steven G. Kargl .
gcc/fortran/ChangeLog:
PR fortran/93498
* check.c (gfc_check_findloc): If the kinds of the arguments
differ goto label "incompat".
gcc/testsuite/ChangeLog:
PR fortran/93498
* gfortran.dg/pr93498_1.f90: New test.
* gfortran.dg/pr93498_2.f90: New test.
---
gcc/fortran/check.c | 4
gcc/testsuite/gfortran.dg/pr93498_1.f90 | 11 +++
gcc/testsuite/gfortran.dg/pr93498_2.f90 | 12
3 files changed, 27 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/pr93498_1.f90
create mode 100644 gcc/testsuite/gfortran.dg/pr93498_2.f90
diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c
index 519aa8b8c2b..cdabbf5e12a 100644
--- a/gcc/fortran/check.c
+++ b/gcc/fortran/check.c
@@ -3947,6 +3947,10 @@ gfc_check_findloc (gfc_actual_arglist *ap)
v1 = v->ts.type == BT_CHARACTER;
if ((a1 && !v1) || (!a1 && v1))
goto incompat;
+
+ /* Check the kind of the characters argument match. */
+ if (a1 && v1 && a->ts.kind != v->ts.kind)
+goto incompat;
d = ap->next->next->expr;
m = ap->next->next->next->expr;
diff --git a/gcc/testsuite/gfortran.dg/pr93498_1.f90 b/gcc/testsuite/gfortran.dg/pr93498_1.f90
new file mode 100644
index 000..0210cc7951e
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr93498_1.f90
@@ -0,0 +1,11 @@
+! { dg-do compile }
+!
+! Test case by G. Steinmetz
+
+program p
+ character(len=1, kind=1) :: x(3) = ['a', 'b', 'c']
+ character(len=1, kind=4) :: y = 4_'b'
+ print *, findloc(x, y) ! { dg-error " must be in type conformance" }
+ print *, findloc(x, y, 1) ! { dg-error " must be in type conformance" }
+end
+
diff --git a/gcc/testsuite/gfortran.dg/pr93498_2.f90 b/gcc/testsuite/gfortran.dg/pr93498_2.f90
new file mode 100644
index 000..ee9238ffa24
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr93498_2.f90
@@ -0,0 +1,12 @@
+! { dg-do compile }
+!
+! Test case by G. Steinmetz
+
+program p
+ character(len=1, kind=4) :: x(3) = [4_'a', 4_'b', 4_'c']
+ character(len=1, kind=1) :: y = 'b'
+ print *, findloc(x, y) ! { dg-error " must be in type conformance" }
+ print *, findloc(x, y, 1) ! { dg-error " must be in type conformance" }
+end
+
+
--
2.11.0
Re: [Patch, 9/10 Regression] fortran: ICE in build_entry_thunks PR93814
**ping** On 23/03/2020 14:55, Mark Eggleston wrote: Apologies, 2nd attempt: gcc/fortran/ChangeLog: Steven G. Kargl PR fortran/93814 * resolve.c (gfc_verify_binding_labels): Handle symbols with the subroutine attribute separately from symbols with the function attribute. gcc/testsuite/ChanheLog: Mark Eggleston PR fortran/93814 * gfortran.dg/pr93814.f90: New test. OK to commit? On 22/03/2020 17:46, Thomas Koenig wrote: Hi Mark, Please find attached Steve Kargl's fix for PR93814. The attached patch does not match the ChangeLog; it seems to be for PR93484. Regards Thomas -- https://www.codethink.co.uk/privacy.html
