Re: [PATCH] Add value range info for affine combination to improve store motion (PR83403)
On Tue, 28 Apr 2020, Xionghu Luo wrote:
> From: Xionghu Luo
>
> Get and propagate value range info to convert expressions with convert
> operation on PLUS_EXPR/MINUS_EXPR/MULT_EXPR when not overflow. i.e.:
>
> (long unsigned int)((unsigned int)n * 10 + 1)
> =>
> (long unsigned int)((unsigned int) n * (long unsigned int)10 + (long
> unsigned int)1)
>
> With this patch for affine combination, load/store motion could detect
> more address refs independency and promote some memory expressions to
> registers within loop.
>
> PS: Replace the previous "(T1)(X + CST) as (T1)X - (T1)(-CST))"
> to "(T1)(X + CST) as (T1)X + (T1)(CST))" for wrapping overflow.
>
> Bootstrap and regression tested pass on Power8-LE. Any comments?
> Thanks.
So the core of the patch is adding handling of MULT_EXPR and the rest
is a no-op? It's not clear from the patch what passing down a
value-range does and your description doesn't say anything about this
either.
Richard.
> gcc/ChangeLog
>
> 2020-04-28 Xiong Hu Luo
>
> PR tree-optimization/83403
> * tree-affine.c (aff_combination_convert): New parameter
> value_range.
> (expr_to_aff_combination): Use range info to check CONVERT
> expr on PLUS_EXPR/MINUS_EXPR/MULT_EXPR.
> (tree_to_aff_combination): Likewise.
> (aff_combination_expand): Get and propagate range info.
> * tree-affine.h: Include value-range.h.
> (aff_combination_convert): New parameter value_range.
> (tree_to_aff_combination): Likewise.
> (aff_combination_expand): Likewise.
> ---
> gcc/tree-affine.c | 66 +++
> gcc/tree-affine.h | 8 +++---
> 2 files changed, 43 insertions(+), 31 deletions(-)
>
> diff --git a/gcc/tree-affine.c b/gcc/tree-affine.c
> index 0eb8db1b086..63f8acd4c73 100644
> --- a/gcc/tree-affine.c
> +++ b/gcc/tree-affine.c
> @@ -220,7 +220,7 @@ aff_combination_add (aff_tree *comb1, aff_tree *comb2)
> /* Converts affine combination COMB to TYPE. */
>
> void
> -aff_combination_convert (aff_tree *comb, tree type)
> +aff_combination_convert (aff_tree *comb, tree type, value_range *vr)
> {
>unsigned i, j;
>tree comb_type = comb->type;
> @@ -228,7 +228,7 @@ aff_combination_convert (aff_tree *comb, tree type)
>if (TYPE_PRECISION (type) > TYPE_PRECISION (comb_type))
> {
>tree val = fold_convert (type, aff_combination_to_tree (comb));
> - tree_to_aff_combination (val, type, comb);
> + tree_to_aff_combination (val, type, comb, vr);
>return;
> }
>
> @@ -263,8 +263,8 @@ aff_combination_convert (aff_tree *comb, tree type)
> true when that was successful and returns the combination in COMB. */
>
> static bool
> -expr_to_aff_combination (aff_tree *comb, tree_code code, tree type,
> - tree op0, tree op1 = NULL_TREE)
> +expr_to_aff_combination (aff_tree *comb, tree_code code, tree type, tree op0,
> + tree op1 = NULL_TREE, value_range *vr = NULL)
> {
>aff_tree tmp;
>poly_int64 bitpos, bitsize, bytepos;
> @@ -279,8 +279,8 @@ expr_to_aff_combination (aff_tree *comb, tree_code code,
> tree type,
>
> case PLUS_EXPR:
> case MINUS_EXPR:
> - tree_to_aff_combination (op0, type, comb);
> - tree_to_aff_combination (op1, type, &tmp);
> + tree_to_aff_combination (op0, type, comb, vr);
> + tree_to_aff_combination (op1, type, &tmp, vr);
>if (code == MINUS_EXPR)
> aff_combination_scale (&tmp, -1);
>aff_combination_add (comb, &tmp);
> @@ -289,7 +289,7 @@ expr_to_aff_combination (aff_tree *comb, tree_code code,
> tree type,
> case MULT_EXPR:
>if (TREE_CODE (op1) != INTEGER_CST)
> break;
> - tree_to_aff_combination (op0, type, comb);
> + tree_to_aff_combination (op0, type, comb, vr);
>aff_combination_scale (comb, wi::to_widest (op1));
>return true;
>
> @@ -340,27 +340,33 @@ expr_to_aff_combination (aff_tree *comb, tree_code
> code, tree type,
> op1 = fold_convert (otype, op1);
> return expr_to_aff_combination (comb, icode, otype, op0, op1);
> }
> - wide_int minv, maxv;
> +
> /* If inner type has wrapping overflow behavior, fold conversion
> -for below case:
> - (T1)(X - CST) -> (T1)X - (T1)CST
> -if X - CST doesn't overflow by range information. Also handle
> -(T1)(X + CST) as (T1)(X - (-CST)). */
> - if (TYPE_UNSIGNED (itype)
> +for below cases:
> + (T1)(X *+- CST) -> (T1)X *+- (T1)CST
> +when X *+- CST doesn't overflow by range information. */
> + if (vr && vr->kind () == VR_RANGE && TYPE_UNSIGNED (itype)
> && TYPE_OVERFLOW_WRAPS (itype)
> - && TREE_CODE (op0) == SSA_NAME
> - && TREE_CODE (op1) == INTEGER_CST
> - && icode != MULT_EXPR
> - && get_range_info (op0, &minv, &ma
[PATCH] toplev.c: Check for null argument to fprintf
Ensure that CF does not equal NULL in function output_stack_usage_1 before calling fprintf. This fixes the following warning/error: gcc/toplev.c:976:13: error: argument 1 null where non-null expected [-Werror=nonnull] 976 | fprintf (cf, "\\n" HOST_WIDE_INT_PRINT_DEC " bytes (%s)", | ^ 977 | stack_usage, | 978 | stack_usage_kind_str[stack_usage_kind]); An example call side where CF is NULL is in function output_stack_usage. Bootstrapped and regtested successfully on S/390. Ok for master? gcc/ChangeLog: 2020-04-28 Stefan Schulze Frielinghaus * toplev.c (output_stack_usage_1): Ensure that first argument to fprintf is not null. --- gcc/toplev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/toplev.c b/gcc/toplev.c index 4c8be502c71..5c026feece2 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -972,7 +972,7 @@ output_stack_usage_1 (FILE *cf) stack_usage += current_function_dynamic_stack_size; } - if (flag_callgraph_info & CALLGRAPH_INFO_STACK_USAGE) + if (cf && flag_callgraph_info & CALLGRAPH_INFO_STACK_USAGE) fprintf (cf, "\\n" HOST_WIDE_INT_PRINT_DEC " bytes (%s)", stack_usage, stack_usage_kind_str[stack_usage_kind]); -- 2.25.3
Re: [PATCH, vect] Check alignment for no peeling gaps handling
Hi, Gentle ping for this patch. https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543701.html BR, Kewen on 2020/4/10 下午5:28, Kewen.Lin via Gcc-patches wrote: > Hi, > > This is one fix following Richi's comments here: > https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542232.html > > I noticed the current half vector support for no peeling gaps > handled some cases which never check the half size vector > support. By further investigation, those cases are safe > to play without peeling gaps due to ideal alignment. It > means they don't require half vector handlings, we should > avoid to use half vector for them. > > The fix is to add alignment check as a part of conditions for > half vector support avoiding redundant half vector codes. > > Bootstrapped/regtested on powerpc64le-linux-gnu P8, while > aarch64-linux-gnu testing is ongoing. > > Is it ok for trunk if all testings are fine? > All testings were fine. > BR, > Kewen > > > gcc/ChangeLog > > 2020-MM-DD Kewen Lin > > * gcc/tree-vect-stmts.c (vectorizable_load): Check alignment to avoid > redundant half vector handlings for no peeling gaps. >
[PATCH] c-attribs.c: Fix use of uninitialized variable nunits
In function handle_vector_size_attribute local variable nunits is supposed to be initialized by function type_valid_for_vector_size. However, in case ARGS is null the function may return with a non-null value and leave nunits uninitialized. This results in warning/error: gcc/poly-int.h: In function 'tree_node* handle_vector_size_attribute(tree_node**, tree, tree, int, bool*)': gcc/poly-int.h:330:3: error: 'nunits' may be used uninitialized in this function [-Werror=maybe-uninitialized] 330 | ((void) (&(RES).coeffs[0] == (C *) 0), \ | ^ gcc/c-family/c-attribs.c:3695:26: note: 'nunits' was declared here 3695 | unsigned HOST_WIDE_INT nunits; | This is fixed by also checking whether ARGS is null or not. Bootstrapped and regtested on S/390. Ok for master? gcc/c-family/ChangeLog: 2020-04-28 Stefan Schulze Frielinghaus * c-attribs.c (handle_vector_size_attribute): Fix use of unintialized variable nunits. --- gcc/c-family/c-attribs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index ac936d5..a8992e76755 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -3694,7 +3694,7 @@ handle_vector_size_attribute (tree *node, tree name, tree args, the number of vector units. */ unsigned HOST_WIDE_INT nunits; type = type_valid_for_vector_size (type, name, args, &nunits); - if (!type) + if (!type || !args) return NULL_TREE; tree new_type = build_vector_type (type, nunits); -- 2.25.3
Re: [RFC] Clarify -ffinite-math-only documentation
On Mon, Apr 27, 2020 at 11:26 PM Matthias Kretz wrote:
>
> On Montag, 27. April 2020 21:39:17 CEST Richard Sandiford wrote:
> > "Dr. Matthias Kretz" writes:
> > > On Montag, 27. April 2020 18:59:08 CEST Richard Sandiford wrote:
> > >> Richard Biener via Gcc-patches writes:
> > >> > On Mon, Apr 27, 2020 at 6:09 PM Matthias Kretz wrote:
> > >> >> Hi,
> > >> >>
> > >> >> This documentation change clarifies the effect of -ffinite-math-only.
> > >> >> With the current documentation, it is unclear what the presence of NaN
> > >> >> and Inf representations means if (arithmetic) operations on such
> > >> >> values
> > >> >> are unspecified and even classification functions like isnan are
> > >> >> unreliable. If the hardware thinks a certain bit pattern is a NaN, but
> > >> >> the software assumes a NaN value cannot ever exist, it is questionable
> > >> >> whether, from a language viewpoint, a representation for NaNs really
> > >> >> exists. Because, a NaN is defined by its behavior. This change also
> > >> >> clarifies that isnan(nan) returning false is fine.
> > >> >>
> > >> >> This relates to PR84949.
> > >> >>
> > >> >> * doc/invoke.texi: Clarify the effects of -ffinite-math-only.
> > >> >>
> > >> >> ---
> > >> >>
> > >> >> gcc/doc/invoke.texi | 6 --
> > >> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> > >> >>
> > >> >> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > >> >> index a37a2ee9c19..9e76ab057a9 100644
> > >> >> --- a/gcc/doc/invoke.texi
> > >> >> +++ b/gcc/doc/invoke.texi
> > >> >> @@ -11619,8 +11619,10 @@ The default is @option{-fno-reciprocal-math}.
> > >> >>
> > >> >> @item -ffinite-math-only
> > >> >> @opindex ffinite-math-only
> > >> >>
> > >> >> -Allow optimizations for floating-point arithmetic that assume
> > >> >> -that arguments and results are not NaNs or +-Infs.
> > >> >> +Assume that floating-point types in the language do not have
> > >> >> representations for
> > >> >> +NaNs and +-Inf. Whether floating-point hardware supports and acts on
> > >> >> NaNs and ++-Inf is not affected. The behavior of a program that uses a
> > >> >> NaN or +-Inf value
> > >> >> +as function argument, macro argument, or operand is undefined.
> > >> >
> > >> > Minor nit here - I'd avoid the 'undefined' word which has bad
> > >> > connotation
> > >> > and use 'unspecified'. Maybe we can even use ISO C language
> > >> > specification
> > >> > terms but I'm not sure which one is most appropriate here.
> > >
> > > I'm an ISO C++ person, and unspecified sounds too reliable to me:
> > > https://wg21.link/intro.defs#defns.unspecified.
> > >
> > >> > Curiously __builtin_nan ("nan") still gets you a NaN representation
> > >> > but isnan(__builtin_nan("nan")) is resolved to false.
> > >
> > > Right, that's because only the hardware thinks __builtin_nan ("nan") is a
> > > NaN representation. With -ffinite-math-only, the double data type in
> > > C/C++ can either hold a finite real value, or an invalid value (i.e. a
> > > value that the optimizer unconditionally excludes as a possible value for
> > > any object of floating-point type). FWIW, with -ffinite-math-only, ubsan
> > > should flag isnan(__builtin_nan("nan")) or any f(constexpr nan).
> > >
> > > With the above documentation change, it is clear that with
> > > https://wg21.link/ P1841 std::numbers::quiet_NaN would be
> > > ill-formed under -ffinite-math- only. Without the documentation change,
> > > it can be argued either way.
> > >
> > > There's another interesting observation resulting from the above: double
> > > and double under -ffinite-math-only are different types. Any function
> > > call from one world to the other is dangerous. Inline functions
> > > translated in different TUs compiled with different math flags violate
> > > the ODR. But that's all the more reason to have a very precise
> > > documentation/understanding of what -ffinite-math-only does. Because this
> > > gotcha is already the status quo.>
> > >> Yeah, for that and other reasons, I think it would be good to avoid
> > >> giving the impression that -ffinite-math-only can be relied on to make
> > >> the assumption above. Wouldn't it be more accurate to say that the
> > >> compiler is allowed to make the assumption, at any point that it seems
> > >> convenient?
> > >
> > > I think undefined behavior does what you're asking for while unspecified
> > > behavior does what you want to avoid. I.e. its an undocumented behavior,
> > > but it can be relied on with a given implementation (compiler).
> >
> > Although it wasn't obvious from my quoting, I was worried more about
> > the wording of the first "Assume X" sentence than the unspecified vs.
> > undefined thing. I think saying "Assume X" loses an important but
> > correct nuance of the original wording: that completely ignoring the
> > option would be a valid (if not very useful) implementation. "Assume X"
> > instead makes it sound like the compiler is required to do something
> > (at least to me).
>
> F
Re: [PATCH, vect] Check alignment for no peeling gaps handling
On Fri, Apr 10, 2020 at 11:28 AM Kewen.Lin wrote: > > Hi, > > This is one fix following Richi's comments here: > https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542232.html > > I noticed the current half vector support for no peeling gaps > handled some cases which never check the half size vector > support. By further investigation, those cases are safe > to play without peeling gaps due to ideal alignment. It > means they don't require half vector handlings, we should > avoid to use half vector for them. > > The fix is to add alignment check as a part of conditions for > half vector support avoiding redundant half vector codes. > > Bootstrapped/regtested on powerpc64le-linux-gnu P8, while > aarch64-linux-gnu testing is ongoing. > > Is it ok for trunk if all testings are fine? OK for stage1 (it's just a missed optimization). Thanks, Richard. > BR, > Kewen > > > gcc/ChangeLog > > 2020-MM-DD Kewen Lin > > * gcc/tree-vect-stmts.c (vectorizable_load): Check alignment to avoid > redundant half vector handlings for no peeling gaps.
Re: [PATCH] rtl cse: Fix PR94740, ICE on testsuite/gcc.dg/sso/t5.c with -mcpu=future -mpcrel -O1
Peter Bergner writes:
> rtl-optimization: ICE on testsuite/gcc.dg/sso/t5.c with -mcpu=future -mpcrel
> -O1 [PR94740]
>
> We ICE on the test case below because decompose_normal_address() doesn't
> expect to see memory operands with constant addresses like below without
> a (const:DI ...) wrapped around the PLUS:
>
> (mem/c:SI (plus:DI (symbol_ref:DI ("*.LANCHOR0") [flags 0x182])
> (const_int 4 [0x4])) [1 array+4 S4 A32])
>
> What we expect to see is:
>
> (mem/c:SI (const:DI (plus:DI (symbol_ref:DI ("*.LANCHOR0") [flags
> 0x182])
>(const_int 4 [0x4]))) [1 arrayD.2903+4 S4
> A32])
>
> Sometimes, combine adds the (const: ...) for us via simplify_binary_operand
> call, but that only happens when combine actually does something with this
> insn. The bad address from the test case actually comes from CSE, so the
> fix here is to make CSE add the (const: ...) whenever it creates a MEM with
> a constant address.
I think we should do this in cse_process_notes_1, both to avoid creating
an invalid MEM in the first place, and so that we handle embedded MEMs
correctly too.
Also, the (const:P ...) ought to be there even outside of a MEM. E.g. we
ought to have:
(set (reg X) (const:P (plus:P (symbol_ref:P S) (const_int D
rather than:
(set (reg X) (plus:P (symbol_ref:P S) (const_int D)))
Adding a PLUS case would fix this example, but I guess a more general
fix would be to add a second switch statement (after the first) that
switches on GET_RTX_CLASS and uses logic like simplify_replace_fn_rtx:
case RTX_BIN_ARITH:
case RTX_COMM_ARITH:
op0 = simplify_replace_fn_rtx (XEXP (x, 0), old_rtx, fn, data);
op1 = simplify_replace_fn_rtx (XEXP (x, 1), old_rtx, fn, data);
if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1))
return x;
return simplify_gen_binary (code, mode, op0, op1);
Maybe we could even replace cse_process_notes_1 with simplify_replace_fn_rtx,
but that's obviously a much more invasive change :-)
Thanks,
Richard
Re: [PATCH] match.pd: Decrease number of nop conversions around bitwise ops [PR94718]
On Tue, Apr 28, 2020 at 08:54:42AM +0200, Richard Biener wrote: > > @@ -1311,7 +1311,7 @@ (define_operator_list COND_TERNARY > > We combine the above two cases by using a conditional convert. */ > > (for bitop (bit_and bit_ior bit_xor) > > (simplify > > - (bitop (convert @0) (convert? @1)) > > + (bitop (convert@2 @0) (convert?@3 @1)) > >(if (((TREE_CODE (@1) == INTEGER_CST > > && INTEGRAL_TYPE_P (TREE_TYPE (@0)) > > && int_fits_type_p (@1, TREE_TYPE (@0))) > > @@ -1330,8 +1330,24 @@ (define_operator_list COND_TERNARY > >|| GET_MODE_CLASS (TYPE_MODE (type)) != MODE_INT > >/* Or if the precision of TO is not the same as the precision > > of its mode. */ > > - || !type_has_mode_precision_p (type))) > > - (convert (bitop @0 (convert @1)) > > + || !type_has_mode_precision_p (type) > > + /* In GIMPLE, getting rid of 2 conversions for one new results > > + in smaller IL. */ > > + || (GIMPLE > > + && TREE_CODE (@1) != INTEGER_CST > > + && tree_nop_conversion_p (type, TREE_TYPE (@0)) > > + && single_use (@2) > > + && single_use (@3 > > I think you don't need explicit single_use checks here but could use :s > in the pattern since the result expression will be "complex" and thus > the :s isn't elided. I would have used :s if I were to do a separate simplify for it, but I didn't want to affect the other cases. So, do we want to use :s even for those? E.g. not sure for the INTEGER_CST case what would (convert?@3:s @1) do when the bitop has INTEGER_CST as second operand. Or should I e.g. move this GIMPLE rule as a separate simplify (under the same for) and then use :s in there? Jakub
[PATCH] tree: Fix up TREE_SIDE_EFFECTS on internal calls [PR94809]
Hi!
On the following testcase, match.pd during GENERIC folding
changes the -1U / x < y into __imag__ .MUL_OVERFLOW (x, y),
but unfortunately unlike for normal calls nothing sets TREE_SIDE_EFFECTS on
the call. There is the process_call_operands function that non-internal
call creation calls and it is usable for internal calls too,
e.g. TREE_SIDE_EFFECTS is derived from checking whether the
call has side-effects (non-ECF_{CONST,PURE}; we have those for internal
calls) and from whether any of the arguments has TREE_SIDE_EFFECTS.
Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?
2020-04-28 Jakub Jelinek
PR tree-optimization/94809
* tree.c (build_call_expr_internal_loc_array): Call
process_call_operands.
* gcc.c-torture/execute/pr94809.c: New test.
--- gcc/tree.c.jj 2020-04-27 14:31:09.260018763 +0200
+++ gcc/tree.c 2020-04-28 00:22:07.537891047 +0200
@@ -11523,6 +11523,7 @@ build_call_expr_internal_loc_array (loca
CALL_EXPR_ARG (t, i) = args[i];
SET_EXPR_LOCATION (t, loc);
CALL_EXPR_IFN (t) = ifn;
+ process_call_operands (t);
return t;
}
--- gcc/testsuite/gcc.c-torture/execute/pr94809.c.jj2020-04-28
00:26:23.647107159 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr94809.c 2020-04-28
00:25:42.289718195 +0200
@@ -0,0 +1,12 @@
+/* PR tree-optimization/94809 */
+
+int
+main ()
+{
+ int a = 0;
+ unsigned long long one = 1;
+ ((-1ULL / one) < a++, one);
+ if (a != 1)
+__builtin_abort ();
+ return 0;
+}
Jakub
Re: [RFC] Clarify -ffinite-math-only documentation
On Dienstag, 28. April 2020 09:21:38 CEST Richard Biener wrote:
> On Mon, Apr 27, 2020 at 11:26 PM Matthias Kretz wrote:
> > On Montag, 27. April 2020 21:39:17 CEST Richard Sandiford wrote:
> > > "Dr. Matthias Kretz" writes:
> > > > On Montag, 27. April 2020 18:59:08 CEST Richard Sandiford wrote:
> > > >> Richard Biener via Gcc-patches writes:
> > > >> > On Mon, Apr 27, 2020 at 6:09 PM Matthias Kretz
wrote:
> > > >> >> Hi,
> > > >> >>
> > > >> >> This documentation change clarifies the effect of
> > > >> >> -ffinite-math-only.
> > > >> >> With the current documentation, it is unclear what the presence of
> > > >> >> NaN
> > > >> >> and Inf representations means if (arithmetic) operations on such
> > > >> >> values
> > > >> >> are unspecified and even classification functions like isnan are
> > > >> >> unreliable. If the hardware thinks a certain bit pattern is a NaN,
> > > >> >> but
> > > >> >> the software assumes a NaN value cannot ever exist, it is
> > > >> >> questionable
> > > >> >> whether, from a language viewpoint, a representation for NaNs
> > > >> >> really
> > > >> >> exists. Because, a NaN is defined by its behavior. This change
> > > >> >> also
> > > >> >> clarifies that isnan(nan) returning false is fine.
> > > >> >>
> > > >> >> This relates to PR84949.
> > > >> >>
> > > >> >> * doc/invoke.texi: Clarify the effects of
> > > >> >> -ffinite-math-only.
> > > >> >>
> > > >> >> ---
> > > >> >>
> > > >> >> gcc/doc/invoke.texi | 6 --
> > > >> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >> >>
> > > >> >> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > > >> >> index a37a2ee9c19..9e76ab057a9 100644
> > > >> >> --- a/gcc/doc/invoke.texi
> > > >> >> +++ b/gcc/doc/invoke.texi
> > > >> >> @@ -11619,8 +11619,10 @@ The default is
> > > >> >> @option{-fno-reciprocal-math}.
> > > >> >>
> > > >> >> @item -ffinite-math-only
> > > >> >> @opindex ffinite-math-only
> > > >> >>
> > > >> >> -Allow optimizations for floating-point arithmetic that assume
> > > >> >> -that arguments and results are not NaNs or +-Infs.
> > > >> >> +Assume that floating-point types in the language do not have
> > > >> >> representations for
> > > >> >> +NaNs and +-Inf. Whether floating-point hardware supports and acts
> > > >> >> on
> > > >> >> NaNs and ++-Inf is not affected. The behavior of a program that
> > > >> >> uses a
> > > >> >> NaN or +-Inf value
> > > >> >> +as function argument, macro argument, or operand is undefined.
> > > >> >
> > > >> > Minor nit here - I'd avoid the 'undefined' word which has bad
> > > >> > connotation
> > > >> > and use 'unspecified'. Maybe we can even use ISO C language
> > > >> > specification
> > > >> > terms but I'm not sure which one is most appropriate here.
> > > >
> > > > I'm an ISO C++ person, and unspecified sounds too reliable to me:
> > > > https://wg21.link/intro.defs#defns.unspecified.
> > > >
> > > >> > Curiously __builtin_nan ("nan") still gets you a NaN representation
> > > >> > but isnan(__builtin_nan("nan")) is resolved to false.
> > > >
> > > > Right, that's because only the hardware thinks __builtin_nan ("nan")
> > > > is a
> > > > NaN representation. With -ffinite-math-only, the double data type in
> > > > C/C++ can either hold a finite real value, or an invalid value (i.e. a
> > > > value that the optimizer unconditionally excludes as a possible value
> > > > for
> > > > any object of floating-point type). FWIW, with -ffinite-math-only,
> > > > ubsan
> > > > should flag isnan(__builtin_nan("nan")) or any f(constexpr nan).
> > > >
> > > > With the above documentation change, it is clear that with
> > > > https://wg21.link/ P1841 std::numbers::quiet_NaN would be
> > > > ill-formed under -ffinite-math- only. Without the documentation
> > > > change,
> > > > it can be argued either way.
> > > >
> > > > There's another interesting observation resulting from the above:
> > > > double
> > > > and double under -ffinite-math-only are different types. Any function
> > > > call from one world to the other is dangerous. Inline functions
> > > > translated in different TUs compiled with different math flags violate
> > > > the ODR. But that's all the more reason to have a very precise
> > > > documentation/understanding of what -ffinite-math-only does. Because
> > > > this
> > > > gotcha is already the status quo.>
> > > >
> > > >> Yeah, for that and other reasons, I think it would be good to avoid
> > > >> giving the impression that -ffinite-math-only can be relied on to
> > > >> make
> > > >> the assumption above. Wouldn't it be more accurate to say that the
> > > >> compiler is allowed to make the assumption, at any point that it
> > > >> seems
> > > >> convenient?
> > > >
> > > > I think undefined behavior does what you're asking for while
> > > > unspecified
> > > > behavior does what you want to avoid. I.e. its an undocumented
> > > > behavior,
> > > > but it can be relied on with a given implementation (compiler).
> >
[PATCH] var-tracking.c: Fix possible use of uninitialized variable pre
While bootstrapping GCC on S/390 the following warning/error is raised:
gcc/var-tracking.c:10239:34: error: 'pre' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
10239 | VTI (bb)->out.stack_adjust += pre;
| ^
The lines of interest are:
HOST_WIDE_INT pre, post = 0;
// ...
if (!frame_pointer_needed)
{
insn_stack_adjust_offset_pre_post (insn, &pre, &post);
// ...
}
// ...
adjust_insn (bb, insn);
if (!frame_pointer_needed && pre)
VTI (bb)->out.stack_adjust += pre;
Both if statements depend on global variable frame_pointer_needed. In function
insn_stack_adjust_offset_pre_post local variable pre is initialized. The
problematic part is the function call between both if statements. Since
adjust_insn also calls functions which are defined in a different compilation
unit, we are not able to prove that global variable frame_pointer_needed is not
altered by adjust_insn and its siblings. Thus we must assume that
frame_pointer_needed may be true before the call and false afterwards which
renders the warning true (admitted the location hint is not totally perfect).
By initialising pre we silence the warning.
Bootstrapped and regtested on S/390. Ok for master?
gcc/ChangeLog:
2020-04-28 Stefan Schulze Frielinghaus
* var-tracking.c (vt_initialize): Initialize local variable pre.
---
gcc/var-tracking.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index 0d39326aa63..f693a4f83fe 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -10171,7 +10171,7 @@ vt_initialize (void)
FOR_EACH_BB_FN (bb, cfun)
{
rtx_insn *insn;
- HOST_WIDE_INT pre, post = 0;
+ HOST_WIDE_INT pre = 0, post = 0;
basic_block first_bb, last_bb;
if (MAY_HAVE_DEBUG_BIND_INSNS)
--
2.25.3
RE: [PATCH v5] aarch64: Add TX3 machine model
Hi Anton,
> -Original Message-
> From: Anton Youdkevitch
> Sent: 27 April 2020 18:21
> To: Kyrylo Tkachov
> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw
> ; James Greenhalgh
> ; Richard Sandiford
> ; jjo...@marvell.com
> Subject: Re: [PATCH v5] aarch64: Add TX3 machine model
>
> On Mon, Apr 27, 2020 at 04:34:49PM +, Kyrylo Tkachov wrote:
> > Hi Anton,
> >
> > > -Original Message-
> > > From: Anton Youdkevitch
> > > Sent: 27 April 2020 11:24
> > > To: gcc-patches@gcc.gnu.org
> > > Cc: Richard Earnshaw ; Kyrylo Tkachov
> > > ; James Greenhalgh
> > > ; Richard Sandiford
> > > ; jjo...@marvell.com
> > > Subject: [PATCH v5] aarch64: Add TX3 machine model
> > >
> > > Here is the patch introducing thunderx3t110 machine model
> > > for the scheduler. A name for the new chip was added to the
> > > list of the names to be recognized as a valid parameter for
> > > mcpu and mtune flags. Added the TX3 tuning table and cost
> > > model tables.
> > >
> > > Added the new chip name to the documentation. Fixed copyright
> > > names and dates.
> > >
> > > Lowering the chip capabilities to v8.3 to be on the safe side.
> > >
> > > Bootstrapped on AArch64.
> > >
> > > 2020-04-27 Anton Youdkevitch
> > >
> > > * config/aarch64/aarch64-cores.def: Add the chip name.
> > > * config/aarch64/aarch64-tune.md: Regenerated.
> > > * config/aarch64/aarch64.c: Add tuning table for the chip.
> > > * gcc/config/aarch64/aarch64-cost-tables.h: Add cost tables.
> > > * config/aarch64/thunderx3t110.md: New file: add the new
> > > machine model for the scheduler
> > > * config/aarch64/aarch64.md: Include the new model.
> > > * doc/invoke.texi: Add the new name to the list
> > >
> > >
> > > ---
> > > gcc/config/aarch64/aarch64-cores.def | 3 +
> > > gcc/config/aarch64/aarch64-cost-tables.h | 103 +++
> > > gcc/config/aarch64/aarch64-tune.md | 2 +-
> > > gcc/config/aarch64/aarch64.c | 83 ++
> > > gcc/config/aarch64/aarch64.md| 1 +
> > > gcc/config/aarch64/thunderx3t110.md | 686 +++
> > > gcc/doc/invoke.texi | 2 +-
> > > 7 files changed, 878 insertions(+), 2 deletions(-)
> >
> > diff --git a/gcc/config/aarch64/aarch64-cores.def
> b/gcc/config/aarch64/aarch64-cores.def
> > index ea9b98b..4d8605a 100644
> > --- a/gcc/config/aarch64/aarch64-cores.def
> > +++ b/gcc/config/aarch64/aarch64-cores.def
> > @@ -95,6 +95,9 @@ AARCH64_CORE("vulcan", vulcan, thunderx2t99,
> 8_1A, AARCH64_FL_FOR_ARCH8_1 | AA
> > /* Cavium ('C') cores. */
> > AARCH64_CORE("thunderx2t99", thunderx2t99, thunderx2t99, 8_1A,
> AARCH64_FL_FOR_ARCH8_1 | AARCH64_FL_CRYPTO, thunderx2t99, 0x43,
> 0x0af, -1)
> >
> > +/* Marvell cores (TX3). */
> > +AARCH64_CORE("thunderx3t110", thunderx3t110, thunderx3t110, 8_3A,
> AARCH64_FL_FOR_ARCH8_3 | AARCH64_FL_CRYPTO | AARCH64_FL_RCPC |
> AARCH64_FL_SM4 | AARCH64_FL_SHA3 | AARCH64_FL_F16FML |
> AARCH64_FL_RCPC8_4, thunderx3t110, 0x43, 0x0b8, 0x0a)
> > +
> >
> > Please move this to a new section with a comment /* ARMv8.3-A
> Architecture processors*/
> > So that we're consistent with the format of the file.
> Fixed.
Thanks, I've pushed the patch with a fixed ChangeLog (entries should list what
has changed in a file).
Can you please create a patch for the AArch64 changes in the
https://gcc.gnu.org/gcc-10/changes.html page for the release.
The instructions are at https://gcc.gnu.org/about.html
2020-04-27 Anton Youdkevitch
* config/aarch64/aarch64-cores.def (thunderx3t110): Add the chip name.
* config/aarch64/aarch64-tune.md: Regenerate.
* config/aarch64/aarch64.c (thunderx3t110_addrcost_table): Define.
(thunderx3t110_regmove_cost): Likewise.
(thunderx3t110_vector_cost): Likewise.
(thunderx3t110_prefetch_tune): Likewise.
(thunderx3t110_tunings): Likewise.
* gcc/config/aarch64/aarch64-cost-tables.h (thunderx3t110_extra_costs):
Define.
* config/aarch64/thunderx3t110.md: New file.
* config/aarch64/aarch64.md: Include thunderx3t110.md.
* doc/invoke.texi (AArch64 options): Add thunderx3t110.
Thanks,
Kyrill
>
> >
> > Ok with that change.
> > Kyrill
> >
> > /* ARMv8.2-A Architecture Processors. */
Re: [PATCH v5] aarch64: Add TX3 machine model
On 28.4.2020 12:02 , Kyrylo Tkachov wrote:
Hi Anton,
-Original Message-
From: Anton Youdkevitch
Sent: 27 April 2020 18:21
To: Kyrylo Tkachov
Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw
; James Greenhalgh
; Richard Sandiford
; jjo...@marvell.com
Subject: Re: [PATCH v5] aarch64: Add TX3 machine model
On Mon, Apr 27, 2020 at 04:34:49PM +, Kyrylo Tkachov wrote:
Hi Anton,
-Original Message-
From: Anton Youdkevitch
Sent: 27 April 2020 11:24
To: gcc-patches@gcc.gnu.org
Cc: Richard Earnshaw ; Kyrylo Tkachov
; James Greenhalgh
; Richard Sandiford
; jjo...@marvell.com
Subject: [PATCH v5] aarch64: Add TX3 machine model
Here is the patch introducing thunderx3t110 machine model
for the scheduler. A name for the new chip was added to the
list of the names to be recognized as a valid parameter for
mcpu and mtune flags. Added the TX3 tuning table and cost
model tables.
Added the new chip name to the documentation. Fixed copyright
names and dates.
Lowering the chip capabilities to v8.3 to be on the safe side.
Bootstrapped on AArch64.
2020-04-27 Anton Youdkevitch
* config/aarch64/aarch64-cores.def: Add the chip name.
* config/aarch64/aarch64-tune.md: Regenerated.
* config/aarch64/aarch64.c: Add tuning table for the chip.
* gcc/config/aarch64/aarch64-cost-tables.h: Add cost tables.
* config/aarch64/thunderx3t110.md: New file: add the new
machine model for the scheduler
* config/aarch64/aarch64.md: Include the new model.
* doc/invoke.texi: Add the new name to the list
---
gcc/config/aarch64/aarch64-cores.def | 3 +
gcc/config/aarch64/aarch64-cost-tables.h | 103 +++
gcc/config/aarch64/aarch64-tune.md | 2 +-
gcc/config/aarch64/aarch64.c | 83 ++
gcc/config/aarch64/aarch64.md| 1 +
gcc/config/aarch64/thunderx3t110.md | 686 +++
gcc/doc/invoke.texi | 2 +-
7 files changed, 878 insertions(+), 2 deletions(-)
diff --git a/gcc/config/aarch64/aarch64-cores.def
b/gcc/config/aarch64/aarch64-cores.def
index ea9b98b..4d8605a 100644
--- a/gcc/config/aarch64/aarch64-cores.def
+++ b/gcc/config/aarch64/aarch64-cores.def
@@ -95,6 +95,9 @@ AARCH64_CORE("vulcan", vulcan, thunderx2t99,
8_1A, AARCH64_FL_FOR_ARCH8_1 | AA
/* Cavium ('C') cores. */
AARCH64_CORE("thunderx2t99", thunderx2t99, thunderx2t99, 8_1A,
AARCH64_FL_FOR_ARCH8_1 | AARCH64_FL_CRYPTO, thunderx2t99, 0x43,
0x0af, -1)
+/* Marvell cores (TX3). */
+AARCH64_CORE("thunderx3t110", thunderx3t110, thunderx3t110, 8_3A,
AARCH64_FL_FOR_ARCH8_3 | AARCH64_FL_CRYPTO | AARCH64_FL_RCPC |
AARCH64_FL_SM4 | AARCH64_FL_SHA3 | AARCH64_FL_F16FML |
AARCH64_FL_RCPC8_4, thunderx3t110, 0x43, 0x0b8, 0x0a)
+
Please move this to a new section with a comment /* ARMv8.3-A
Architecture processors*/
So that we're consistent with the format of the file.
Fixed.
Thanks, I've pushed the patch with a fixed ChangeLog (entries should list what
has changed in a file).
Thanks a lot!
Can you please create a patch for the AArch64 changes in the
https://gcc.gnu.org/gcc-10/changes.html page for the release.
The instructions are at https://gcc.gnu.org/about.html
Will do.
2020-04-27 Anton Youdkevitch
* config/aarch64/aarch64-cores.def (thunderx3t110): Add the chip name.
* config/aarch64/aarch64-tune.md: Regenerate.
* config/aarch64/aarch64.c (thunderx3t110_addrcost_table): Define.
(thunderx3t110_regmove_cost): Likewise.
(thunderx3t110_vector_cost): Likewise.
(thunderx3t110_prefetch_tune): Likewise.
(thunderx3t110_tunings): Likewise.
* gcc/config/aarch64/aarch64-cost-tables.h (thunderx3t110_extra_costs):
Define.
* config/aarch64/thunderx3t110.md: New file.
* config/aarch64/aarch64.md: Include thunderx3t110.md.
* doc/invoke.texi (AArch64 options): Add thunderx3t110.
Thanks,
Kyrill
Ok with that change.
Kyrill
/* ARMv8.2-A Architecture Processors. */
gcc.dg testsuite glitches
Hi,
first, I do not have commit rights, so please somebody check and commit,
I guess this goes under the obvious and trivial rules.
There are several malformed dejagnu directives in the gcc.dg testsuite.
Below I fixed some of them following these criteria:
- fix mis-typed directives
- require that the outermost braces are space padded
- fix imbalanced braces
Several of these changes are no-ops, as nonsensical directives act as "dg-do
compile",
but they should be fixed nevertheless, IMO.
Cheers,
Manfred
diff --git a/gcc/testsuite/gcc.dg/20050121-1.c b/gcc/testsuite/gcc.dg/20050121-1.c
index 3fe299a6dc..e7bfdafeb5 100644
--- a/gcc/testsuite/gcc.dg/20050121-1.c
+++ b/gcc/testsuite/gcc.dg/20050121-1.c
@@ -1,6 +1,6 @@
/* I accidentally broke this while developing a patch for PR 13000,
and didn't notice since the testsuite didn't catch it -- ian */
-/* { dg-do-compile } */
+/* { dg-do compile } */
void foo()
{
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr93382.c b/gcc/testsuite/gcc.dg/analyzer/pr93382.c
index dae32f5a2b..bd11e10611 100644
--- a/gcc/testsuite/gcc.dg/analyzer/pr93382.c
+++ b/gcc/testsuite/gcc.dg/analyzer/pr93382.c
@@ -14,7 +14,7 @@ ql (void)
int n1[1];
fread (n1, sizeof (n1[0]), 1, fp); /* { dg-message "'n1' gets an unchecked value here" } */
- idx = n1[0]; /* { dg-message "'idx' has an unchecked value here (from 'n1')" */
+ idx = n1[0]; /* { dg-message "'idx' has an unchecked value here (from 'n1')" } */
}
int arr[10];
diff --git a/gcc/testsuite/gcc.dg/autopar/pr68460.c b/gcc/testsuite/gcc.dg/autopar/pr68460.c
index 0c00065afb..74f852d68a 100644
--- a/gcc/testsuite/gcc.dg/autopar/pr68460.c
+++ b/gcc/testsuite/gcc.dg/autopar/pr68460.c
@@ -1,4 +1,4 @@
-/* { dg-do "compile" } */
+/* { dg-do compile } */
/* { dg-options "-O -ftree-parallelize-loops=2 -ftree-vectorize -fno-tree-ch -fno-tree-dominator-opts" } */
void abort (void);
diff --git a/gcc/testsuite/gcc.dg/c90-fordecl-1.c b/gcc/testsuite/gcc.dg/c90-fordecl-1.c
index 46ba16abbc..6a8e061fb4 100644
--- a/gcc/testsuite/gcc.dg/c90-fordecl-1.c
+++ b/gcc/testsuite/gcc.dg/c90-fordecl-1.c
@@ -9,6 +9,6 @@ foo (void)
int j = 0;
for (int i = 1; i <= 10; i++) /* { dg-bogus "warning" "warning in place of error" } */
/* { dg-error "'for' loop initial declarations are only allowed in C99 or C11 mode" "declaration in for loop" { target *-*-* } .-1 } */
-/* { dg-message "note: use option '-std=c99', '-std=gnu99', '-std=c11' or '-std=gnu11' to compile your code" "note" { target *-*-* } .-2 }} */
+/* { dg-message "note: use option '-std=c99', '-std=gnu99', '-std=c11' or '-std=gnu11' to compile your code" "note" { target *-*-* } .-2 } */
j += i;
}
diff --git a/gcc/testsuite/gcc.dg/cpp/trad/funlike-5.c b/gcc/testsuite/gcc.dg/cpp/trad/funlike-5.c
index f60a6ea786..b5481bd3a0 100644
--- a/gcc/testsuite/gcc.dg/cpp/trad/funlike-5.c
+++ b/gcc/testsuite/gcc.dg/cpp/trad/funlike-5.c
@@ -1,7 +1,7 @@
/* Test function like macro. */
/* Contributed by Devang Patel */
-/* {do-do preprocess } */
+/* { dg-do preprocess } */
/* { dg-options "-traditional-cpp -E -dD" } */
int __srget (char *);
#define __sgetc(p) (--(p)->_r < 0 ? __srget(p) : (int)(*(p)->_p++))
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-dfp.c b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-dfp.c
index 951380f125..3e27461222 100644
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-dfp.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-dfp.c
@@ -1,6 +1,6 @@
/* Verify the DWARF encoding of C99 decimal floating point types. */
-/* { dg-do compile */
+/* { dg-do compile } */
/* { dg-require-effective-target dfp } */
/* { dg-options "-O0 -gdwarf -dA" } */
/* { dg-final { scan-assembler "0x10.*DW_AT_encoding" } } */
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-float.c b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-float.c
index a028d1484a..f4883842b8 100644
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-float.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/dwarf-float.c
@@ -1,6 +1,6 @@
/* Verify the DWARF encoding of C99 floating point types. */
-/* { dg-do compile */
+/* { dg-do compile } */
/* { dg-options "-O0 -gdwarf -dA" } */
/* { dg-final { scan-assembler "0x4.*DW_AT_encoding" } } */
/* { dg-final { scan-assembler "0x4.*DW_AT_byte_size" } } */
diff --git a/gcc/testsuite/gcc.dg/lto/pr52634_0.c b/gcc/testsuite/gcc.dg/lto/pr52634_0.c
index 5e14ad9a73..7731abf4bc 100644
--- a/gcc/testsuite/gcc.dg/lto/pr52634_0.c
+++ b/gcc/testsuite/gcc.dg/lto/pr52634_0.c
@@ -1,7 +1,7 @@
/* { dg-require-weak "" } */
/* { dg-require-alias "" } */
/* { dg-lto-do link } */
-/* { dg-lto-options {{-flto -r -flto-partition=1to1}} */
+/* { dg-lto-options {-flto -r -flto-partition=1to1} } */
/* { dg-extra-ld-options "-flinker-output=nolto-rel" } */
extern int cfliteValueCallBacks;
void baz (int *);
diff --git a/gcc/testsuite/gcc.dg/pr32069.c b/gcc/testsuite/gcc.dg/pr32069.c
index 2ec37c675e..87a3f187e1 100644
--- a/gcc/testsuite/gcc.dg/pr32069.c
+++ b/gcc/testsuite/gcc.dg/
Re: [PATCH] var-tracking.c: Fix possible use of uninitialized variable pre
Stefan Schulze Frielinghaus via Gcc-patches writes:
> While bootstrapping GCC on S/390 the following warning/error is raised:
>
> gcc/var-tracking.c:10239:34: error: 'pre' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> 10239 | VTI (bb)->out.stack_adjust += pre;
> | ^
>
> The lines of interest are:
>
> HOST_WIDE_INT pre, post = 0;
> // ...
> if (!frame_pointer_needed)
> {
> insn_stack_adjust_offset_pre_post (insn, &pre, &post);
> // ...
> }
>
> // ...
> adjust_insn (bb, insn);
>
> if (!frame_pointer_needed && pre)
> VTI (bb)->out.stack_adjust += pre;
>
> Both if statements depend on global variable frame_pointer_needed. In
> function
> insn_stack_adjust_offset_pre_post local variable pre is initialized. The
> problematic part is the function call between both if statements. Since
> adjust_insn also calls functions which are defined in a different compilation
> unit, we are not able to prove that global variable frame_pointer_needed is
> not
> altered by adjust_insn and its siblings. Thus we must assume that
> frame_pointer_needed may be true before the call and false afterwards which
> renders the warning true (admitted the location hint is not totally perfect).
> By initialising pre we silence the warning.
>
> Bootstrapped and regtested on S/390. Ok for master?
>
> gcc/ChangeLog:
>
> 2020-04-28 Stefan Schulze Frielinghaus
>
> * var-tracking.c (vt_initialize): Initialize local variable pre.
OK, thanks.
Richard
Re: [PATCH] match.pd: Decrease number of nop conversions around bitwise ops [PR94718]
On Tue, 28 Apr 2020, Jakub Jelinek wrote: > On Tue, Apr 28, 2020 at 08:54:42AM +0200, Richard Biener wrote: > > > @@ -1311,7 +1311,7 @@ (define_operator_list COND_TERNARY > > > We combine the above two cases by using a conditional convert. */ > > > (for bitop (bit_and bit_ior bit_xor) > > > (simplify > > > - (bitop (convert @0) (convert? @1)) > > > + (bitop (convert@2 @0) (convert?@3 @1)) > > >(if (((TREE_CODE (@1) == INTEGER_CST > > >&& INTEGRAL_TYPE_P (TREE_TYPE (@0)) > > >&& int_fits_type_p (@1, TREE_TYPE (@0))) > > > @@ -1330,8 +1330,24 @@ (define_operator_list COND_TERNARY > > > || GET_MODE_CLASS (TYPE_MODE (type)) != MODE_INT > > > /* Or if the precision of TO is not the same as the precision > > > of its mode. */ > > > -|| !type_has_mode_precision_p (type))) > > > - (convert (bitop @0 (convert @1)) > > > +|| !type_has_mode_precision_p (type) > > > +/* In GIMPLE, getting rid of 2 conversions for one new results > > > + in smaller IL. */ > > > +|| (GIMPLE > > > +&& TREE_CODE (@1) != INTEGER_CST > > > +&& tree_nop_conversion_p (type, TREE_TYPE (@0)) > > > +&& single_use (@2) > > > +&& single_use (@3 > > > > I think you don't need explicit single_use checks here but could use :s > > in the pattern since the result expression will be "complex" and thus > > the :s isn't elided. > > I would have used :s if I were to do a separate simplify for it, but I > didn't want to affect the other cases. So, do we want to use > :s even for those? E.g. not sure for the INTEGER_CST case what would > (convert?@3:s @1) do when the bitop has INTEGER_CST as second operand. I think we generally want to use :s, but OTOH even for your new case, since you restrict to nop-conversions, :s isn't strictly needed since VN should know how to materialize nop-converted variants. I think it's different for the 2nd case you add since there we deal with bitops in different sign. So maybe just remove the single_use () calls? > Or should I e.g. move this GIMPLE rule as a separate simplify (under the > same for) and then use :s in there? That wouldn't work, if a structural pattern matched others are not considered so it wouldn't ever be used. genmatch even warns for this I think. Richard.
Re: [PATCH] tree: Fix up TREE_SIDE_EFFECTS on internal calls [PR94809]
On Tue, 28 Apr 2020, Jakub Jelinek wrote:
> Hi!
>
> On the following testcase, match.pd during GENERIC folding
> changes the -1U / x < y into __imag__ .MUL_OVERFLOW (x, y),
> but unfortunately unlike for normal calls nothing sets TREE_SIDE_EFFECTS on
> the call. There is the process_call_operands function that non-internal
> call creation calls and it is usable for internal calls too,
> e.g. TREE_SIDE_EFFECTS is derived from checking whether the
> call has side-effects (non-ECF_{CONST,PURE}; we have those for internal
> calls) and from whether any of the arguments has TREE_SIDE_EFFECTS.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
OK.
Richard.
> 2020-04-28 Jakub Jelinek
>
> PR tree-optimization/94809
> * tree.c (build_call_expr_internal_loc_array): Call
> process_call_operands.
>
> * gcc.c-torture/execute/pr94809.c: New test.
>
> --- gcc/tree.c.jj 2020-04-27 14:31:09.260018763 +0200
> +++ gcc/tree.c2020-04-28 00:22:07.537891047 +0200
> @@ -11523,6 +11523,7 @@ build_call_expr_internal_loc_array (loca
> CALL_EXPR_ARG (t, i) = args[i];
>SET_EXPR_LOCATION (t, loc);
>CALL_EXPR_IFN (t) = ifn;
> + process_call_operands (t);
>return t;
> }
>
> --- gcc/testsuite/gcc.c-torture/execute/pr94809.c.jj 2020-04-28
> 00:26:23.647107159 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr94809.c 2020-04-28
> 00:25:42.289718195 +0200
> @@ -0,0 +1,12 @@
> +/* PR tree-optimization/94809 */
> +
> +int
> +main ()
> +{
> + int a = 0;
> + unsigned long long one = 1;
> + ((-1ULL / one) < a++, one);
> + if (a != 1)
> +__builtin_abort ();
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Biener
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Re: [PATCH] c-attribs.c: Fix use of uninitialized variable nunits
On Tue, Apr 28, 2020 at 10:03 AM Stefan Schulze Frielinghaus via Gcc-patches wrote: > > In function handle_vector_size_attribute local variable nunits is > supposed to be initialized by function type_valid_for_vector_size. > However, in case ARGS is null the function may return with a non-null > value and leave nunits uninitialized. This results in warning/error: > > gcc/poly-int.h: In function 'tree_node* > handle_vector_size_attribute(tree_node**, tree, tree, int, bool*)': > gcc/poly-int.h:330:3: error: 'nunits' may be used uninitialized in this > function [-Werror=maybe-uninitialized] > 330 | ((void) (&(RES).coeffs[0] == (C *) 0), \ > | ^ > gcc/c-family/c-attribs.c:3695:26: note: 'nunits' was declared here > 3695 | unsigned HOST_WIDE_INT nunits; > | > > This is fixed by also checking whether ARGS is null or not. > > Bootstrapped and regtested on S/390. Ok for master? I think it's better to assert that it is not null for example by adding a nonnull attribute? Can you check if that works? If it doesn't the patch is OK. Thanks, Richard. > gcc/c-family/ChangeLog: > > 2020-04-28 Stefan Schulze Frielinghaus > > * c-attribs.c (handle_vector_size_attribute): Fix use of > unintialized variable nunits. > --- > gcc/c-family/c-attribs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > index ac936d5..a8992e76755 100644 > --- a/gcc/c-family/c-attribs.c > +++ b/gcc/c-family/c-attribs.c > @@ -3694,7 +3694,7 @@ handle_vector_size_attribute (tree *node, tree name, > tree args, > the number of vector units. */ >unsigned HOST_WIDE_INT nunits; >type = type_valid_for_vector_size (type, name, args, &nunits); > - if (!type) > + if (!type || !args) > return NULL_TREE; > >tree new_type = build_vector_type (type, nunits); > -- > 2.25.3 >
Re: [RFC] Clarify -ffinite-math-only documentation
On Tue, Apr 28, 2020 at 10:15 AM Matthias Kretz wrote:
>
> On Dienstag, 28. April 2020 09:21:38 CEST Richard Biener wrote:
> > On Mon, Apr 27, 2020 at 11:26 PM Matthias Kretz wrote:
> > > On Montag, 27. April 2020 21:39:17 CEST Richard Sandiford wrote:
> > > > "Dr. Matthias Kretz" writes:
> > > > > On Montag, 27. April 2020 18:59:08 CEST Richard Sandiford wrote:
> > > > >> Richard Biener via Gcc-patches writes:
> > > > >> > On Mon, Apr 27, 2020 at 6:09 PM Matthias Kretz
> wrote:
> > > > >> >> Hi,
> > > > >> >>
> > > > >> >> This documentation change clarifies the effect of
> > > > >> >> -ffinite-math-only.
> > > > >> >> With the current documentation, it is unclear what the presence of
> > > > >> >> NaN
> > > > >> >> and Inf representations means if (arithmetic) operations on such
> > > > >> >> values
> > > > >> >> are unspecified and even classification functions like isnan are
> > > > >> >> unreliable. If the hardware thinks a certain bit pattern is a NaN,
> > > > >> >> but
> > > > >> >> the software assumes a NaN value cannot ever exist, it is
> > > > >> >> questionable
> > > > >> >> whether, from a language viewpoint, a representation for NaNs
> > > > >> >> really
> > > > >> >> exists. Because, a NaN is defined by its behavior. This change
> > > > >> >> also
> > > > >> >> clarifies that isnan(nan) returning false is fine.
> > > > >> >>
> > > > >> >> This relates to PR84949.
> > > > >> >>
> > > > >> >> * doc/invoke.texi: Clarify the effects of
> > > > >> >> -ffinite-math-only.
> > > > >> >>
> > > > >> >> ---
> > > > >> >>
> > > > >> >> gcc/doc/invoke.texi | 6 --
> > > > >> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >> >>
> > > > >> >> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > > > >> >> index a37a2ee9c19..9e76ab057a9 100644
> > > > >> >> --- a/gcc/doc/invoke.texi
> > > > >> >> +++ b/gcc/doc/invoke.texi
> > > > >> >> @@ -11619,8 +11619,10 @@ The default is
> > > > >> >> @option{-fno-reciprocal-math}.
> > > > >> >>
> > > > >> >> @item -ffinite-math-only
> > > > >> >> @opindex ffinite-math-only
> > > > >> >>
> > > > >> >> -Allow optimizations for floating-point arithmetic that assume
> > > > >> >> -that arguments and results are not NaNs or +-Infs.
> > > > >> >> +Assume that floating-point types in the language do not have
> > > > >> >> representations for
> > > > >> >> +NaNs and +-Inf. Whether floating-point hardware supports and acts
> > > > >> >> on
> > > > >> >> NaNs and ++-Inf is not affected. The behavior of a program that
> > > > >> >> uses a
> > > > >> >> NaN or +-Inf value
> > > > >> >> +as function argument, macro argument, or operand is undefined.
> > > > >> >
> > > > >> > Minor nit here - I'd avoid the 'undefined' word which has bad
> > > > >> > connotation
> > > > >> > and use 'unspecified'. Maybe we can even use ISO C language
> > > > >> > specification
> > > > >> > terms but I'm not sure which one is most appropriate here.
> > > > >
> > > > > I'm an ISO C++ person, and unspecified sounds too reliable to me:
> > > > > https://wg21.link/intro.defs#defns.unspecified.
> > > > >
> > > > >> > Curiously __builtin_nan ("nan") still gets you a NaN representation
> > > > >> > but isnan(__builtin_nan("nan")) is resolved to false.
> > > > >
> > > > > Right, that's because only the hardware thinks __builtin_nan ("nan")
> > > > > is a
> > > > > NaN representation. With -ffinite-math-only, the double data type in
> > > > > C/C++ can either hold a finite real value, or an invalid value (i.e. a
> > > > > value that the optimizer unconditionally excludes as a possible value
> > > > > for
> > > > > any object of floating-point type). FWIW, with -ffinite-math-only,
> > > > > ubsan
> > > > > should flag isnan(__builtin_nan("nan")) or any f(constexpr nan).
> > > > >
> > > > > With the above documentation change, it is clear that with
> > > > > https://wg21.link/ P1841 std::numbers::quiet_NaN would be
> > > > > ill-formed under -ffinite-math- only. Without the documentation
> > > > > change,
> > > > > it can be argued either way.
> > > > >
> > > > > There's another interesting observation resulting from the above:
> > > > > double
> > > > > and double under -ffinite-math-only are different types. Any function
> > > > > call from one world to the other is dangerous. Inline functions
> > > > > translated in different TUs compiled with different math flags violate
> > > > > the ODR. But that's all the more reason to have a very precise
> > > > > documentation/understanding of what -ffinite-math-only does. Because
> > > > > this
> > > > > gotcha is already the status quo.>
> > > > >
> > > > >> Yeah, for that and other reasons, I think it would be good to avoid
> > > > >> giving the impression that -ffinite-math-only can be relied on to
> > > > >> make
> > > > >> the assumption above. Wouldn't it be more accurate to say that the
> > > > >> compiler is allowed to make the assumption, at any point that it
> > > > >> seems
> > > > >> convenient?
> > > > >
> >
Re: [PATCH] var-tracking.c: Fix possible use of uninitialized variable pre
On Tue, Apr 28, 2020 at 11:28 AM Stefan Schulze Frielinghaus via
Gcc-patches wrote:
>
> While bootstrapping GCC on S/390 the following warning/error is raised:
>
> gcc/var-tracking.c:10239:34: error: 'pre' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> 10239 | VTI (bb)->out.stack_adjust += pre;
> | ^
>
> The lines of interest are:
>
> HOST_WIDE_INT pre, post = 0;
> // ...
> if (!frame_pointer_needed)
> {
> insn_stack_adjust_offset_pre_post (insn, &pre, &post);
> // ...
> }
>
> // ...
> adjust_insn (bb, insn);
>
> if (!frame_pointer_needed && pre)
> VTI (bb)->out.stack_adjust += pre;
>
> Both if statements depend on global variable frame_pointer_needed. In
> function
> insn_stack_adjust_offset_pre_post local variable pre is initialized. The
> problematic part is the function call between both if statements. Since
> adjust_insn also calls functions which are defined in a different compilation
> unit, we are not able to prove that global variable frame_pointer_needed is
> not
> altered by adjust_insn and its siblings. Thus we must assume that
> frame_pointer_needed may be true before the call and false afterwards which
> renders the warning true (admitted the location hint is not totally perfect).
> By initialising pre we silence the warning.
>
> Bootstrapped and regtested on S/390. Ok for master?
I think even better would be to move the pre declaration inside
FOR_BB_INSNS_SAFE (bb, insn, next)
{
if (INSN_P (insn))
{
initialized with zero and then elide the !frame_pointer_needed part
of
if (!frame_pointer_needed && pre)
can you check that works? OK if it does.
Btw, does s390 have different inlining parameters somehow?
Richard.
> gcc/ChangeLog:
>
> 2020-04-28 Stefan Schulze Frielinghaus
>
> * var-tracking.c (vt_initialize): Initialize local variable pre.
> ---
> gcc/var-tracking.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
> index 0d39326aa63..f693a4f83fe 100644
> --- a/gcc/var-tracking.c
> +++ b/gcc/var-tracking.c
> @@ -10171,7 +10171,7 @@ vt_initialize (void)
>FOR_EACH_BB_FN (bb, cfun)
> {
>rtx_insn *insn;
> - HOST_WIDE_INT pre, post = 0;
> + HOST_WIDE_INT pre = 0, post = 0;
>basic_block first_bb, last_bb;
>
>if (MAY_HAVE_DEBUG_BIND_INSNS)
> --
> 2.25.3
>
Re: introduce target tmpnam and require it in tests relying on it
On 24 April 2020 11:59:50 CEST, Alexandre Oliva wrote: >On Apr 23, 2020, Martin Sebor wrote: > >> Sure. I'd go with _fileio but that's just a suggestion. > >Okiedokie, here's the patch using fileio instead of tmpnam for the >effective target name. I'm going to check it in shortly. > > >introduce target fileio and require it in tests that use tmpnam ISTM the corresponding documentation hunk for sourcebuild.texi is missing. TIA, > >Some target C libraries that aren't recognized as freestanding don't >have filesystem support, so calling tmpnam, fopen/open and >remove/unlink fails to link. > >This patch introduces a fileio effective target to the testsuite, and >requires it in the tests that call tmpnam. > > >for gcc/testsuite/ChangeLog > > * lib/target-supports.exp (check_effective_target_fileio): New. > * gcc.c-torture/execute/fprintf-2.c: Require it. > * gcc.c-torture/execute/printf-2.c: Likewise. > * gcc.c-torture/execute/user-printf.c: Likewise. >--- > gcc/testsuite/gcc.c-torture/execute/fprintf-2.c |1 + > gcc/testsuite/gcc.c-torture/execute/printf-2.c|1 + > gcc/testsuite/gcc.c-torture/execute/user-printf.c |1 + > gcc/testsuite/lib/target-supports.exp | 13 + > 4 files changed, 16 insertions(+)
Re: [RFC] Clarify -ffinite-math-only documentation
Matthias Kretz writes:
> On Dienstag, 28. April 2020 09:21:38 CEST Richard Biener wrote:
>> On Mon, Apr 27, 2020 at 11:26 PM Matthias Kretz wrote:
>> > * Why not disable NaN and Inf independently? Inf is just a reciprocal 0.
>> > Inf is as far away from numeric_limits::max as 0 is from
>> > numeric_limits::min (infinitely many steps on a floating-point "type"
>> > with infinite exponent).
>>
>> Just a comment on the last point - internally it's already split into
>> HONOR_NANS and HONOR_INFINITIES that there's just
>> a single user-facing option -ffinite-math-only is historical (IMHO more
>> options are more opportunities for users to get confused ;)).
>>
>> Maybe also interesting to the discussion the internal comments say
>>
>> /* True if the given mode has a NaN representation and the treatment of
>>NaN operands is important. Certain optimizations, such as folding
>>x * 0 into 0, are not correct for NaN operands, and are normally
>>disabled for modes with NaNs. The user can ask for them to be
>>done anyway using the -funsafe-math-optimizations switch. */
>> extern bool HONOR_NANS (machine_mode);
>> [...]
>> note how the comment says "treatment of NaN operands is important"
>
> ... if HONOR_NANS(mode) == true. So with -ffinite-math-only that implies
> "treatment of NaN operands is not important", right?
>
>> and the example of simplifying x * 0 to 0 is about preserving NaNs
>> during expression simplification when the FPU would.
>
> Yes, the `nan * 0 -> 0` simplification with -ffinite-math-only is why I'm
> saying the status quo already has no NaN representation *in C/C++* anymore.
> Because a float with NaN bits doesn't behave like C17 5.2.4.2.2/4 requires.
> And thus it's an invalid/unspecified/undefined value, not a NaN. If there are
> no NaNs (as defined by C), then there is no NaN representation and
> __FLT_HAS_QUIET_NAN__ must be 0.
>
>> I think these
>> kind of optimizations are what originally was intended to be allowed
>> with -ffinite-math-only - that we started to simplify isnan(x) to false
>> was extending the scope and that change wasn't uncontested since
>> it makes -ffinite-math-only less useful to some people.
>
> I understand and agree with this point. However I think the problem with the
> current -ffinite-math-only is that it doesn't do enough. I.e. it's a
> dangerous
> optimization for people who rely on nan+inf in some way. But in many cases
> people don't realize they do, and it may be a latent issue that turns into a
> bug on unrelated code changes (or compiler changes). By going all the way,
> aggressively optimizing assuming there can't ever be nan/inf in the program,
> making the use of inf/nan ill-formed NDR, and integrating with ubsan, I think
> the option becomes less dangerous and more useful. That would be a bold move,
> certainly.
I think I understand your position, but what about options like
-fassociative-math:
--
Allow re-association of operands in series of floating-point operations.
This violates the ISO C and C++ language standard by possibly changing
computation result. NOTE: re-ordering may change the sign of zero as
well as ignore NaNs and inhibit or create underflow or overflow (and
thus cannot be used on code that relies on rounding behavior like
@code{(x + 2**52) - 2**52}. May also reorder floating-point comparisons
and thus may not be used when ordered comparisons are required.
This option requires that both @option{-fno-signed-zeros} and
@option{-fno-trapping-math} be in effect. Moreover, it doesn't make
much sense with @option{-frounding-math}. For Fortran the option
is automatically enabled when both @option{-fno-signed-zeros} and
@option{-fno-trapping-math} are in effect.
--
The point about NaN handling doesn't seem obviously different from the
X * 0 -> 0 case. But on its own, -fassociative-math doesn't give us
the freedom to make __builtin_isnan always return false. I.e. with
-fassociative-math on its own, NaNs definitely still exist, but don't
always behave as the standard says they should.
I think the spirit of the two options was supposed to be the same.
We're deliberately setting aside normal language rules rather than
working within the framework of the standard. So in that sense, what
the standard says about __FLT_HAS_QUIET_NAN__ isn't necessarily binding.
It can just be another rule that we're setting aside. The same goes
for the numeric_limits question: I think the fact that it can currently
be argued both ways is an accurate reflection of how the option was
supposed to “work”.
In other words, we're leaving the standard behind, so trying to define
how the option works in terms of the standard isn't necessarily meaningful.
I realise that isn't very elegant from a language definition POV though...
This sound similar to the situation Richard Smith was trying to avoid
Re: [PATCH] Add value range info for affine combination to improve store motion (PR83403)
On 2020/4/28 15:01, Richard Biener wrote:
> On Tue, 28 Apr 2020, Xionghu Luo wrote:
>
>> From: Xionghu Luo
>>
>> Get and propagate value range info to convert expressions with convert
>> operation on PLUS_EXPR/MINUS_EXPR/MULT_EXPR when not overflow. i.e.:
>>
>> (long unsigned int)((unsigned int)n * 10 + 1)
>> =>
>> (long unsigned int)((unsigned int) n * (long unsigned int)10 + (long
>> unsigned int)1)
>>
>> With this patch for affine combination, load/store motion could detect
>> more address refs independency and promote some memory expressions to
>> registers within loop.
>>
>> PS: Replace the previous "(T1)(X + CST) as (T1)X - (T1)(-CST))"
>> to "(T1)(X + CST) as (T1)X + (T1)(CST))" for wrapping overflow.
>>
>> Bootstrap and regression tested pass on Power8-LE. Any comments?
>> Thanks.
>
> So the core of the patch is adding handling of MULT_EXPR and the rest
> is a no-op? It's not clear from the patch what passing down a
> value-range does and your description doesn't say anything about this
> either.
This patch handles MULT_EXPR first, and also handles (long unsigned int)(n_93 *
10 + 1) as
"n_93*10" is not a ssa_name by using the value_range passed down, minv&maxv
[1,81] is the
range info of "n_93 * 10 +1", so wi::leu_p (maxv, wi::to_wide (TYPE_MAX_VALUE
(itype)) in
expr_to_aff_combination could ensure that no wrapping overflow of this
converting. Not sure
if it's better described now... And some debug message as follows:
131t.lim2:
# n_93 = PHI <0(2), n_36(7)>
_118 = n_93 * 10;
_120 = (long unsigned int) _118;
_121 = _120 * 8;
_122 = &C[0][0] + _121;
*_122 = 0.0;
_128 = _118 + 1;
_129 = (long unsigned int) _128;
_130 = _129 * 8;
_131 = &C[0][0] + _130;
*_131 = 0.0;
_137 = _118 + 2;
_138 = (long unsigned int) _137;
_139 = _138 * 8;
_140 = &C[0][0] + _139;
*_140 = 0.0;
Breakpoint 28, expr_to_aff_combination (comb=0x7fffc068, code=NOP_EXPR,
type=, op0=, op1=,
vr=0x7fffc490) at ../../gcc-mast
er/gcc/tree-affine.c:350
92: itype =
93: otype =
94: icode = PLUS_EXPR
95: code = NOP_EXPR
(gdb) ptree op0
unit-size
align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x753c0690 precision:32 min <
integer_cst 0x752f1230 0> max
context
pointer_to_this >
arg:0
visited var
def_stmt n_93 = PHI <0(2), n_36(7)>
version:93
ptr-info 0x7530b680>
arg:1 constant 10>>
(gdb) ptree op1
constant 1>
(gdb) p minv
$580 = {
= {
val = {1, 0, 140737310886240},
len = 1,
precision = 32
},
members of generic_wide_int:
static is_sign_extended = true
}
(gdb) p maxv
$581 = {
= {
val = {81, 65, 40},
len = 1,
precision = 32
},
members of generic_wide_int:
static is_sign_extended = true
}
Thanks,
Xionghu
>
> Richard.
>
>> gcc/ChangeLog
>>
>> 2020-04-28 Xiong Hu Luo
>>
>> PR tree-optimization/83403
>> * tree-affine.c (aff_combination_convert): New parameter
>> value_range.
>> (expr_to_aff_combination): Use range info to check CONVERT
>> expr on PLUS_EXPR/MINUS_EXPR/MULT_EXPR.
>> (tree_to_aff_combination): Likewise.
>> (aff_combination_expand): Get and propagate range info.
>> * tree-affine.h: Include value-range.h.
>> (aff_combination_convert): New parameter value_range.
>> (tree_to_aff_combination): Likewise.
>> (aff_combination_expand): Likewise.
>> ---
>> gcc/tree-affine.c | 66 +++
>> gcc/tree-affine.h | 8 +++---
>> 2 files changed, 43 insertions(+), 31 deletions(-)
>>
>> diff --git a/gcc/tree-affine.c b/gcc/tree-affine.c
>> index 0eb8db1b086..63f8acd4c73 100644
>> --- a/gcc/tree-affine.c
>> +++ b/gcc/tree-affine.c
>> @@ -220,7 +220,7 @@ aff_combination_add (aff_tree *comb1, aff_tree *comb2)
>> /* Converts affine combination COMB to TYPE. */
>>
>> void
>> -aff_combination_convert (aff_tree *comb, tree type)
>> +aff_combination_convert (aff_tree *comb, tree type, value_range *vr)
>> {
>> unsigned i, j;
>> tree comb_type = comb->type;
>> @@ -228,7 +228,7 @@ aff_combination_convert (aff_tree *comb, tree type)
>> if (TYPE_PRECISION (type) > TYPE_PRECISION (comb_type))
>> {
>> tree val = fold_convert (type, aff_combination_to_tree (comb));
>> - tree_to_aff_combination (val, type, comb);
>> + tree_to_aff_combination (val, type, comb, vr);
>> return;
>> }
>>
>> @@ -263,8 +263,8 @@ aff_combination_convert (aff_tree *comb, tree type)
>> true when that was successful and returns the combination in COMB. */
>>
>> static bool
>> -expr_to_aff_combination (aff_tree *comb, tree_code code, tree type,
>> - tree op0, tree op1 = NULL_TREE)
>> +expr_to_aff_combination (aff_tree *comb, tree_code code, tree type, tree
>> op0,
>> + tree op1 = NULL_TREE, value_range *vr = NULL)
>> {
>> aff_tree tmp;
>> poly_int64 bitpos, bitsize, bytepos;
>> @@ -27
RE: [Arm] Account for C++17 artificial field determining Homogeneous Aggregates
Hi Matthew,
> -Original Message-
> From: Matthew Malcomson
> Sent: 27 April 2020 16:32
> To: gcc-patches@gcc.gnu.org
> Cc: nd ; ni...@redhat.com; Richard Earnshaw
> ; Ramana Radhakrishnan
> ; Kyrylo Tkachov
> ; ja...@redhat.com
> Subject: [Arm] Account for C++17 artificial field determining Homogeneous
> Aggregates
>
> NOTE:
> There is another patch in the making to handle the APCS abi (selected with
> `-mabi=apcs-gnu`). That patch has a very different change in behaviour and
> is in a different part of the code so I'm keeping it in a different patch.
>
> In C++14, an empty class deriving from an empty base is not an
> aggregate, while in C++17 it is. In order to implement this, GCC adds
> an artificial field to such classes.
>
> This artificial field has no mapping to Fundamental Data Types in the
> Arm PCS ABI and hence should not count towards determining whether an
> object can be passed using the vector registers as per section
> "7.1.2 Procedure Calling" in the arm PCS
> https://developer.arm.com/docs/ihi0042/latest?_ga=2.60211309.150685319
> 6.1533541889-405231439.1528186050
>
> This patch avoids counting this artificial field in
> aapcs_vfp_sub_candidate, and hence calculates whether such objects
> should be passed in vector registers in the same manner as C++14 (where
> the artificial field does not exist).
>
> Before this change, the test below would pass the arguments to `f` in
> general registers. After this change, the test passes the arguments to
> `f` using the vector registers.
>
> The new behaviour matches the behaviour of `armclang`, and also matches
> the GCC behaviour when run with `-std=gnu++14`.
>
> > gcc -std=gnu++17 -march=armv8-a+simd -mfloat-abi=hard test.cpp
>
> ``` test.cpp
> struct base {};
>
> struct pair : base
> {
> float first;
> float second;
> pair (float f, float s) : first(f), second(s) {}
> };
>
> void f (pair);
> int main()
> {
> f({3.14, 666});
> return 1;
> }
> ```
>
> We add a `-Wpsabi` warning to catch cases where this fix has changed the
> ABI for
> some functions. Unfortunately this warning is not emitted twice for multiple
> calls to the same function, but I feel this is not much of a problem and can
> be
> fixed later if needs be.
>
> (i.e. if `main` called `f` twice in a row we only emit a diagnostic for the
> first).
>
> Testing:
> Bootstrapped and regression tested on arm-linux.
> This change fixes the struct-layout-1 tests Jakub added
> https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544204.html
> Regression tested on arm-none-eabi.
>
This looks like it follows the logic of the other port patches.
Ok.
Thanks,
Kyrill
> gcc/ChangeLog:
>
> 2020-04-27 Matthew Malcomson
> Jakub Jelinek
>
> PR target/94383
> * config/arm/arm.c (aapcs_vfp_sub_candidate): Account for C++17
> empty
> base class artificial fields.
> (aapcs_vfp_is_call_or_return_candidate): Warn when PCS ABI
> decision is different after this fix.
>
>
>
> ### Attachment also inlined for ease of reply
> ###
>
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index
> 0151bda90d961ae1a001c61cd5e94d6ec67e3aea..0db444c3fdb2ba6fb7ebad4
> 10310fb5b1bc0e304 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -6139,9 +6139,19 @@ aapcs_vfp_cum_init (CUMULATIVE_ARGS *pcum
> ATTRIBUTE_UNUSED,
> If *MODEP is VOIDmode, then set it to the first valid floating point
> type. If a non-floating point type is found, or if a floating point
> type that doesn't match a non-VOIDmode *MODEP is found, then return -
> 1,
> - otherwise return the count in the sub-tree. */
> + otherwise return the count in the sub-tree.
> +
> + The AVOID_CXX17_EMPTY_BASE argument is to allow the caller to check
> whether
> + this function has changed its behavior after the fix for PR94384 -- this
> fix
> + is to avoid artificial fields in empty base classes.
> + When called with this argument as a NULL pointer this function does not
> + avoid the artificial fields -- this is useful to check whether the
> function
> + returns something different after the fix.
> + When called pointing at a value, this function avoids such artificial
> fields
> + and sets the value to TRUE when one of these fields has been set. */
> static int
> -aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
> +aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep,
> + bool *avoid_cxx17_empty_base)
> {
>machine_mode mode;
>HOST_WIDE_INT size;
> @@ -6213,7 +6223,8 @@ aapcs_vfp_sub_candidate (const_tree type,
> machine_mode *modep)
> || TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST)
> return -1;
>
> - count = aapcs_vfp_sub_candidate (TREE_TYPE (type), modep);
> + count = aapcs_vfp_sub_candidate (TREE_TYPE (type), modep,
> + avoid_cxx17_empty_base);
>
Re: [PATCH] Add value range info for affine combination to improve store motion (PR83403)
On Tue, 28 Apr 2020, luoxhu wrote:
>
>
> On 2020/4/28 15:01, Richard Biener wrote:
> > On Tue, 28 Apr 2020, Xionghu Luo wrote:
> >
> >> From: Xionghu Luo
> >>
> >> Get and propagate value range info to convert expressions with convert
> >> operation on PLUS_EXPR/MINUS_EXPR/MULT_EXPR when not overflow. i.e.:
> >>
> >> (long unsigned int)((unsigned int)n * 10 + 1)
> >> =>
> >> (long unsigned int)((unsigned int) n * (long unsigned int)10 + (long
> >> unsigned int)1)
> >>
> >> With this patch for affine combination, load/store motion could detect
> >> more address refs independency and promote some memory expressions to
> >> registers within loop.
> >>
> >> PS: Replace the previous "(T1)(X + CST) as (T1)X - (T1)(-CST))"
> >> to "(T1)(X + CST) as (T1)X + (T1)(CST))" for wrapping overflow.
> >>
> >> Bootstrap and regression tested pass on Power8-LE. Any comments?
> >> Thanks.
> >
> > So the core of the patch is adding handling of MULT_EXPR and the rest
> > is a no-op? It's not clear from the patch what passing down a
> > value-range does and your description doesn't say anything about this
> > either.
>
> This patch handles MULT_EXPR first, and also handles (long unsigned int)(n_93
> * 10 + 1) as
> "n_93*10" is not a ssa_name by using the value_range passed down, minv&maxv
> [1,81] is the
> range info of "n_93 * 10 +1", so wi::leu_p (maxv, wi::to_wide (TYPE_MAX_VALUE
> (itype)) in
> expr_to_aff_combination could ensure that no wrapping overflow of this
> converting. Not sure
> if it's better described now... And some debug message as follows:
>
> 131t.lim2:
> # n_93 = PHI <0(2), n_36(7)>
> _118 = n_93 * 10;
> _120 = (long unsigned int) _118;
> _121 = _120 * 8;
> _122 = &C[0][0] + _121;
> *_122 = 0.0;
> _128 = _118 + 1;
> _129 = (long unsigned int) _128;
> _130 = _129 * 8;
> _131 = &C[0][0] + _130;
> *_131 = 0.0;
> _137 = _118 + 2;
> _138 = (long unsigned int) _137;
> _139 = _138 * 8;
> _140 = &C[0][0] + _139;
> *_140 = 0.0;
>
>
> Breakpoint 28, expr_to_aff_combination (comb=0x7fffc068, code=NOP_EXPR,
> type= e0 long unsigned int>, op0=, op1=,
> vr=0x7fffc490) at ../../gcc-mast
> er/gcc/tree-affine.c:350
> 92: itype =
> 93: otype =
> 94: icode = PLUS_EXPR
> 95: code = NOP_EXPR
> (gdb) ptree op0
> type unsigned SI
> size
> unit-size
> align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x753c0690 precision:32 min <
> integer_cst 0x752f1230 0> max
> context 75552850 pr83403.c>
> pointer_to_this >
>
> arg:0 int>
> visited var
> def_stmt n_93 = PHI <0(2), n_36(7)>
> version:93
> ptr-info 0x7530b680>
> arg:1 unsigned int> constant 10>>
> (gdb) ptree op1
>
> constant 1>
> (gdb) p minv
> $580 = {
>= {
> val = {1, 0, 140737310886240},
> len = 1,
> precision = 32
> },
> members of generic_wide_int:
> static is_sign_extended = true
> }
> (gdb) p maxv
> $581 = {
>= {
> val = {81, 65, 40},
> len = 1,
> precision = 32
> },
> members of generic_wide_int:
> static is_sign_extended = true
> }
OK, I guess instead of get_range_info expr_to_aff_combination could
simply use determine_value_range (op0, &minv, &maxv) == VR_RANGE
(the && TREE_CODE (op0) == SSA_NAME check can then be removed)?
Thanks,
Richard.
>
> Thanks,
> Xionghu
>
> >
> > Richard.
> >
> >> gcc/ChangeLog
> >>
> >>2020-04-28 Xiong Hu Luo
> >>
> >>PR tree-optimization/83403
> >>* tree-affine.c (aff_combination_convert): New parameter
> >>value_range.
> >>(expr_to_aff_combination): Use range info to check CONVERT
> >>expr on PLUS_EXPR/MINUS_EXPR/MULT_EXPR.
> >>(tree_to_aff_combination): Likewise.
> >>(aff_combination_expand): Get and propagate range info.
> >>* tree-affine.h: Include value-range.h.
> >>(aff_combination_convert): New parameter value_range.
> >>(tree_to_aff_combination): Likewise.
> >>(aff_combination_expand): Likewise.
> >> ---
> >> gcc/tree-affine.c | 66 +++
> >> gcc/tree-affine.h | 8 +++---
> >> 2 files changed, 43 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/gcc/tree-affine.c b/gcc/tree-affine.c
> >> index 0eb8db1b086..63f8acd4c73 100644
> >> --- a/gcc/tree-affine.c
> >> +++ b/gcc/tree-affine.c
> >> @@ -220,7 +220,7 @@ aff_combination_add (aff_tree *comb1, aff_tree *comb2)
> >> /* Converts affine combination COMB to TYPE. */
> >>
> >> void
> >> -aff_combination_convert (aff_tree *comb, tree type)
> >> +aff_combination_convert (aff_tree *comb, tree type, value_range *vr)
> >> {
> >> unsigned i, j;
> >> tree comb_type = comb->type;
> >> @@ -228,7 +228,7 @@ aff_combination_convert (aff_tree *comb, tree type)
> >> if (TYPE_PRECISION (type) > TYPE_PRECISION (comb_type))
> >> {
> >> tree val = fold_convert (type, aff_combination_to_tree (comb));
> >> - tree_to_aff_combination (val, type, comb);
> >> +
[PATCH] middle-end, rs6000: Handle empty [[no_unique_address]] fields like empty bases on powerpc64le-linux [PR94707]
Hi!
Ok, I've tried:
struct X { };
struct Y { int : 0; };
struct Z { int : 0; Y y; };
struct U : public X { X q; };
struct A { float a, b, c, d; };
struct B : public X { float a, b, c, d; };
struct C : public Y { float a, b, c, d; };
struct D : public Z { float a, b, c, d; };
struct E : public U { float a, b, c, d; };
struct F { [[no_unique_address]] X x; float a, b, c, d; };
struct G { [[no_unique_address]] Y y; float a, b, c, d; };
struct H { [[no_unique_address]] Z z; float a, b, c, d; };
struct I { [[no_unique_address]] U u; float a, b, c, d; };
struct J { float a, b; [[no_unique_address]] X x; float c, d; };
struct K { float a, b; [[no_unique_address]] Y y; float c, d; };
struct L { float a, b; [[no_unique_address]] Z z; float c, d; };
struct M { float a, b; [[no_unique_address]] U u; float c, d; };
#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s);
return 0; }
T (A, a)
T (B, b)
T (C, c)
T (D, d)
T (E, e)
T (F, f)
T (G, g)
T (H, h)
T (I, i)
T (J, j)
T (K, k)
T (L, l)
T (M, m)
testcase on powerpc64-linux. Results:
G++ 9 -std=c++14A, B, C passed in fprs, the rest in gprs
G++ 9 -std=c++17A passed in fprs, the rest in gprs
current trunk -std=c++14 & 17 A, B, C passed in fprs, the rest in gprs
patched trunk -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the rest in
gprs
clang++ [*] -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the rest in
gprs
[*] clang version 11.0.0 (g...@github.com:llvm/llvm-project.git
5c352e69e76a26e4eda075e20aa6a9bb7686042c)
Is that what we want? I think it matches the stated intent of P0840R2 or
what Jason/Jonathan said, and doing something different like e.g. not
treating C, G and K as homogenous because of the int : 0 in empty bases
or in zero sized [[no_unique_address] fields would be quite hard to
implement (because for C++14 the FIELD_DECL just isn't there).
I've included the above testcase as g++.target/powerpc/ testcases.
2020-04-28 Jakub Jelinek
PR target/94707
* calls.h (no_unique_address_empty_field_p): Declare.
* calls.c (no_unique_address_empty_field_p): New function.
* rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, ignore also no_unique_address_empty_field_p
fields and propagate that fact to caller.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 12:38:35.292851412 +0200
@@ -136,5 +136,6 @@ extern void maybe_warn_nonstring_arg (tr
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
extern bool cxx17_empty_base_field_p (const_tree);
+extern bool no_unique_address_empty_field_p (const_tree);
#endif // GCC_CALLS_H
--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
+++ gcc/calls.c 2020-04-28 12:39:11.936308866 +0200
@@ -6279,5 +6279,24 @@ cxx17_empty_base_field_p (const_tree fie
&& !integer_zerop (TYPE_SIZE (TREE_TYPE (field;
}
+/* Return true if FIELD is a non-static data member with empty
+ type and [[no_unique_address]] attribute that should be
+ ignored for ABI calling convention decisions, in order to make
+ struct S {};
+ struct T : S { float x; };
+ and
+ struct T2 : { [[no_unique_address]] S s; float x; };
+ ABI compatible. */
+
+bool
+no_unique_address_empty_field_p (const_tree field)
+{
+ return (TREE_CODE (field) == FIELD_DECL
+ && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field))
+ && DECL_SIZE (field)
+ && integer_zerop (DECL_SIZE (field))
+ && lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (field)));
+}
+
/* Tell the garbage collector about GTY markers in this source file. */
#include "gt-calls.h"
--- gcc/config/rs6000/rs6000-call.c.jj 2020-04-23 14:42:26.323839084 +0200
+++ gcc/config/rs6000/rs6000-call.c 2020-04-28 12:43:28.277513460 +0200
@@ -5529,7 +5529,7 @@ const struct altivec_builtin_types altiv
static int
rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
- bool *cxx17_empty_base_seen)
+ int *empty_base_seen)
{
machine_mode mode;
HOST_WIDE_INT size;
@@ -5600,7 +5600,7 @@ rs6000_aggregate_candidate (const_tree t
return -1;
count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
- cxx17_empty_base_seen);
+ emp
Re: [PATCH] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout [PR94707]
On 4/27/20 5:23 PM, Jakub Jelinek wrote:
On Mon, Apr 27, 2020 at 05:11:38PM -0400, Jason Merrill wrote:
struct empty { };
struct X { [[no_unique_address]] empty e; };
and have them be layout compatible, otherwise the attribute is useless
to the standard library.
Why are zero-size fields changing layout/parameter passing in these ABIs,
anyway?
There are many affected ABIs and they have different rules.
Some are e.g. looking for homogenous aggregates, say
struct S { float a, b, c, d, e; };
contains just float fields and is passed in floating point registers in some
ABIs, and that is how C++14 represented even
struct empty {};
struct S : public empty { float a, b, c, d, e; };
and with the changes from last week also the C++17. Thus, I guess
we want to handle
struct T { [[no_unique_address]] empty x; float a, b, c, d, e; };
that way too. The question is shall that be only when it is the first
field, or at any position as long as it is [[no_unique_address]] FIELD_DECL
with DECL_SIZE of bitsize_zero?
Any position.
Or do we have to further check that it
really doesn't contain any fields other than empty classes?
E.g. some of the ABIs pass differently:
struct A {};
struct B { A a; float b, c, d; };
struct C { int : 0; };
struct D { C a; float b, c, d; };
because the zero sized bitfield is spelled in the ABI as causing different
behavior.
Other ABIs look for first field and do something if it is of some kind (or
if it is the first and only say float or vector field).
E.g. the powerpc-darwin or powerpc*-aix* ABIs do different alignment
depending on if it is:
struct E {};
struct F { double d; };
or
struct G { E e; double d; };
One can just grep the backends for TYPE_FIELDS walks...
Is there a bitfield in FIELD_DECL we could use to mean 'this field
should not affect ABI' (other than as the front end has already used it
for layout)? From tree-core.h it looks like addressable_flag,
static_flag, public_flag, asm_written_flag, nothrow_flag,
saturating_flag, default_def_flag are all unused on FIELD_DECL.
Jason
Re: [PATCH] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout [PR94707]
On Tue, Apr 28, 2020 at 07:44:26AM -0400, Jason Merrill via Gcc-patches wrote:
> > Or do we have to further check that it
> > really doesn't contain any fields other than empty classes?
> > E.g. some of the ABIs pass differently:
> > struct A {};
> > struct B { A a; float b, c, d; };
> > struct C { int : 0; };
> > struct D { C a; float b, c, d; };
> > because the zero sized bitfield is spelled in the ABI as causing different
> > behavior.
> > Other ABIs look for first field and do something if it is of some kind (or
> > if it is the first and only say float or vector field).
> > E.g. the powerpc-darwin or powerpc*-aix* ABIs do different alignment
> > depending on if it is:
> > struct E {};
> > struct F { double d; };
> > or
> > struct G { E e; double d; };
> >
> > One can just grep the backends for TYPE_FIELDS walks...
>
> Is there a bitfield in FIELD_DECL we could use to mean 'this field should
> not affect ABI' (other than as the front end has already used it for
> layout)? From tree-core.h it looks like addressable_flag, static_flag,
> public_flag, asm_written_flag, nothrow_flag, saturating_flag,
> default_def_flag are all unused on FIELD_DECL.
Because of the -Wpsabi diagnostics that (some) targets want, having
just one bit "ignore this FIELD_DECL in ABI decisions" is not good enough.
If we want to use some bit, perhaps
(FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
could be used to mark something, either the C++17 empty bases only,
or both those and [[no_unique_address]] and then the backends could use
lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (field))
to find out if it is the C++14 vs. C++17 ABI incompatibility, or the
all C++ versions [[no_unique_address]] thing.
See the powerpc64le-linux patch I've just posted.
Jakub
[PATCH, PR94769] fortran/io.c: Fix use of uninitialized variable num
While bootstrapping GCC on S/390 the following warning occurs:
gcc/fortran/io.c: In function 'bool gfc_resolve_dt(gfc_code*, gfc_dt*, locus*)':
gcc/fortran/io.c:3857:7: error: 'num' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
3857 | if (num == 0)
| ^~
gcc/fortran/io.c:3843:11: note: 'num' was declared here
3843 | int num;
Since gfc_resolve_dt is a non-static function we cannot assume anything about
argument DT. Argument DT gets passed to function check_io_constraints which
passes values depending on DT, namely dt->asynchronous->value.character.string
to function compare_to_allowed_values as well as argument warn which is true as
soon as DT->dterr is true. Thus both arguments depend on DT.
If function compare_to_allowed_values is called with
dt->asynchronous->value.character.string not being an allowed value, and
ALLOWED_F2003 as well as ALLOWED_GNU being NULL (which is the case at the
particular call side), and WARN equals true, then the function returns with a
non-zero value and leaves num uninitialized which renders the warning true.
Initializing num to any value but zero mimics the behavior of an uninitialized
variable except UB because zero is the only value it is tested for at the
moment. Since num is used as an index into array asynchronous initializing it
to 2 seems plausible.
Bootstrapped and regtested on S/390. Ok for master?
gcc/fortran/ChangeLog:
2020-04-28 Stefan Schulze Frielinghaus
PR fortran/94769
* io.c (check_io_constraints): Initialize local variable num.
---
gcc/fortran/io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c
index e06e01d..4526f729d1d 100644
--- a/gcc/fortran/io.c
+++ b/gcc/fortran/io.c
@@ -3840,7 +3840,7 @@ if (condition) \
if (dt->asynchronous)
{
- int num;
+ int num = 2;
static const char * asynchronous[] = { "YES", "NO", NULL };
/* Note: gfc_reduce_init_expr reports an error if not init-expr. */
--
2.25.3
[PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]
Hi!
So, based on the yesterday's discussions, similarly to powerpc64le-linux
I've done some testing for s390x-linux too.
First of all, I found a bug in my patch from yesterday, it was printing
the wrong type like 'double' etc. rather than the class that contained such
the element. Fix below.
For s390x-linux, I was using
struct X { };
struct Y { int : 0; };
struct Z { int : 0; Y y; };
struct U : public X { X q; };
struct A { double a; };
struct B : public X { double a; };
struct C : public Y { double a; };
struct D : public Z { double a; };
struct E : public U { double a; };
struct F { [[no_unique_address]] X x; double a; };
struct G { [[no_unique_address]] Y y; double a; };
struct H { [[no_unique_address]] Z z; double a; };
struct I { [[no_unique_address]] U u; double a; };
struct J { double a; [[no_unique_address]] X x; };
struct K { double a; [[no_unique_address]] Y y; };
struct L { double a; [[no_unique_address]] Z z; };
struct M { double a; [[no_unique_address]] U u; };
#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s);
return 0; }
T (A, a)
T (B, b)
T (C, c)
T (D, d)
T (E, e)
T (F, f)
T (G, g)
T (H, h)
T (I, i)
T (J, j)
T (K, k)
T (L, l)
T (M, m)
as testcase and looking for "\tld\t%f0,".
While g++ 9 with -std=c++17 used to pass in fpr just
A, g++ 9 -std=c++14, as well as current trunk -std=c++14 & 17
and clang++ from today -std=c++14 & 17 all pass A, B, C
in fpr and nothing else. The intent stated by Jason seems to be
that A, B, C, F, G, J, K should all be passed in fpr. So, shall
we change both GCC and clang? Or is the published ABI clear on this?
Or does it need clarification?
2020-04-28 Jakub Jelinek
PR target/94704
* config/s390/s390.c (s390_function_arg_vector,
s390_function_arg_float): In -Wpsabi diagnostics use the type
passed to the function rather than the type of the single element.
--- gcc/config/s390/s390.c.jj 2020-04-28 10:26:08.499984223 +0200
+++ gcc/config/s390/s390.c 2020-04-28 14:01:31.813712290 +0200
@@ -11912,6 +11912,7 @@ s390_function_arg_vector (machine_mode m
/* The ABI says that record types with a single member are treated
just like that member would be. */
bool cxx17_empty_base_seen = false;
+ const_tree orig_type = type;
while (TREE_CODE (type) == RECORD_TYPE)
{
tree field, single = NULL_TREE;
@@ -11958,7 +11959,7 @@ s390_function_arg_vector (machine_mode m
last_reported_type_uid = uid;
inform (input_location, "parameter passing for argument of type "
"%qT when C++17 is enabled changed to match "
- "C++14 in GCC 10.1", type);
+ "C++14 in GCC 10.1", orig_type);
}
}
return true;
@@ -11984,6 +11985,7 @@ s390_function_arg_float (machine_mode mo
/* The ABI says that record types with a single member are treated
just like that member would be. */
bool cxx17_empty_base_seen = false;
+ const_tree orig_type = type;
while (TREE_CODE (type) == RECORD_TYPE)
{
tree field, single = NULL_TREE;
@@ -12022,7 +12024,7 @@ s390_function_arg_float (machine_mode mo
last_reported_type_uid = uid;
inform (input_location, "parameter passing for argument of type "
"%qT when C++17 is enabled changed to match "
- "C++14 in GCC 10.1", type);
+ "C++14 in GCC 10.1", orig_type);
}
}
Jakub
[PATCH][GCC-8][Aarch64]: Fix for PR target/9481
Hi,
Backport of PR target/94518: Fix memmodel index in
aarch64_store_exclusive_pair
This fixes bootstrap with --enable-checking=yes,rtl for aarch64.
OK for gcc-8?
Cheers,
Andre
gcc/ChangeLog:
2020-04-28 Andre Vieira
PR target/94814
Backport from gcc-9.
2020-04-07 Kyrylo Tkachov
PR target/94518
2019-09-23 Richard Sandiford
* config/aarch64/atomics.md (aarch64_store_exclusive_pair): Fix
memmodel index.
diff --git a/gcc/config/aarch64/atomics.md b/gcc/config/aarch64/atomics.md
index
1005462ae23aa13dbc3013a255aa189096e33366..0e0b03731922d8e50e8468de94e0ff345d10c32f
100644
--- a/gcc/config/aarch64/atomics.md
+++ b/gcc/config/aarch64/atomics.md
@@ -752,7 +752,7 @@
UNSPECV_SX))]
""
{
-enum memmodel model = memmodel_from_int (INTVAL (operands[3]));
+enum memmodel model = memmodel_from_int (INTVAL (operands[4]));
if (is_mm_relaxed (model) || is_mm_consume (model) || is_mm_acquire
(model))
return "stxp\t%w0, %x2, %x3, %1";
else
RE: [PATCH][GCC-8][Aarch64]: Fix for PR target/9481
> -Original Message- > From: Andre Vieira (lists) > Sent: 28 April 2020 13:23 > To: gcc-patches@gcc.gnu.org > Cc: Kyrylo Tkachov > Subject: [PATCH][GCC-8][Aarch64]: Fix for PR target/9481 > > Hi, > > Backport of PR target/94518: Fix memmodel index in > aarch64_store_exclusive_pair > > This fixes bootstrap with --enable-checking=yes,rtl for aarch64. > > OK for gcc-8? Ok. Thanks, Kyrill > > Cheers, > Andre > > gcc/ChangeLog: > 2020-04-28 Andre Vieira > > PR target/94814 > Backport from gcc-9. > 2020-04-07 Kyrylo Tkachov > > PR target/94518 > 2019-09-23 Richard Sandiford > > * config/aarch64/atomics.md (aarch64_store_exclusive_pair): Fix > memmodel index.
Re: [PATCH] rtl cse: Fix PR94740, ICE on testsuite/gcc.dg/sso/t5.c with -mcpu=future -mpcrel -O1
On 4/28/20 2:38 AM, Richard Sandiford wrote: > I think we should do this in cse_process_notes_1, both to avoid creating > an invalid MEM in the first place, and so that we handle embedded MEMs > correctly too. > > Also, the (const:P ...) ought to be there even outside of a MEM. E.g. we > ought to have: > > (set (reg X) (const:P (plus:P (symbol_ref:P S) (const_int D > > rather than: > > (set (reg X) (plus:P (symbol_ref:P S) (const_int D))) I wondered about things such as this, but I couldn't remember ever seeing a (const:P ...) wrapped around a constant expression before. Maybe I just wasn't looking for it??? > Adding a PLUS case would fix this example, but I guess a more general > fix would be to add a second switch statement (after the first) that > switches on GET_RTX_CLASS and uses logic like simplify_replace_fn_rtx: > > case RTX_BIN_ARITH: > case RTX_COMM_ARITH: > op0 = simplify_replace_fn_rtx (XEXP (x, 0), old_rtx, fn, data); > op1 = simplify_replace_fn_rtx (XEXP (x, 1), old_rtx, fn, data); > if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1)) > return x; > return simplify_gen_binary (code, mode, op0, op1); Ok, I'll try this and see whether we survive bootstrap and regtesting. Thanks! Peter
Re: [PATCH, PR94769] fortran/io.c: Fix use of uninitialized variable num
On Tue, Apr 28, 2020 at 01:53:07PM +0200, Stefan Schulze Frielinghaus via
Gcc-patches wrote:
> gcc/fortran/ChangeLog:
>
> 2020-04-28 Stefan Schulze Frielinghaus
>
> PR fortran/94769
> * io.c (check_io_constraints): Initialize local variable num.
> ---
> gcc/fortran/io.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c
> index e06e01d..4526f729d1d 100644
> --- a/gcc/fortran/io.c
> +++ b/gcc/fortran/io.c
> @@ -3840,7 +3840,7 @@ if (condition) \
>
>if (dt->asynchronous)
> {
> - int num;
> + int num = 2;
>static const char * asynchronous[] = { "YES", "NO", NULL };
Just nitpicking, wouldn't -1 be more usual value?
And, I think there should be an assertion that it didn't remain -1 after the
call, above
/* For "YES", mark related symbols as asynchronous. */
do
gcc_checking (num != -1);
or so.
Note, the reason why this triggers only on s390x is the vastly different
inlining parameters the target uses, that causes a lot of headaches
everywhere.
Jakub
[PATCH] fix regression with MEM commoning
While working on PR57359 I noticed that a change during GCC 9
development broke a pointer equality check in ref_always_accessed.
The following corrects this - the way LIM works it is enough
to check for store vs. load.
The issue leads to more conditional SMs than necessary.
Bootstrap / regtest running on x86_64-unknown-linux-gnu.
Richard.
This fixes a regression when canonicalizing refs for LIM PR84362.
This possibly unshares and rewrites the refs in the internal data
and thus pointer equality no longer works in ref_always_accessed
computation.
2020-04-28 Richard Biener
* tree-ssa-loop-im.c (ref_always_accessed::operator ()):
Just check whether the stmt stores.
---
gcc/tree-ssa-loop-im.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/gcc/tree-ssa-loop-im.c b/gcc/tree-ssa-loop-im.c
index 2bb47e9d6d2..471b620f2c4 100644
--- a/gcc/tree-ssa-loop-im.c
+++ b/gcc/tree-ssa-loop-im.c
@@ -2407,20 +2407,21 @@ ref_always_accessed::operator () (mem_ref_loc *loc)
{
class loop *must_exec;
- if (!get_lim_data (loc->stmt))
+ struct lim_aux_data *lim_data = get_lim_data (loc->stmt);
+ if (!lim_data)
return false;
/* If we require an always executed store make sure the statement
- stores to the reference. */
+ is a store. */
if (stored_p)
{
tree lhs = gimple_get_lhs (loc->stmt);
if (!lhs
- || lhs != *loc->ref)
+ || !(DECL_P (lhs) || REFERENCE_CLASS_P (lhs)))
return false;
}
- must_exec = get_lim_data (loc->stmt)->always_executed_in;
+ must_exec = lim_data->always_executed_in;
if (!must_exec)
return false;
--
2.13.7
Re: [PATCH] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout [PR94707]
On 4/28/20 7:54 AM, Jakub Jelinek wrote:
On Tue, Apr 28, 2020 at 07:44:26AM -0400, Jason Merrill via Gcc-patches wrote:
Or do we have to further check that it
really doesn't contain any fields other than empty classes?
E.g. some of the ABIs pass differently:
struct A {};
struct B { A a; float b, c, d; };
struct C { int : 0; };
struct D { C a; float b, c, d; };
because the zero sized bitfield is spelled in the ABI as causing different
behavior.
Other ABIs look for first field and do something if it is of some kind (or
if it is the first and only say float or vector field).
E.g. the powerpc-darwin or powerpc*-aix* ABIs do different alignment
depending on if it is:
struct E {};
struct F { double d; };
or
struct G { E e; double d; };
One can just grep the backends for TYPE_FIELDS walks...
Is there a bitfield in FIELD_DECL we could use to mean 'this field should
not affect ABI' (other than as the front end has already used it for
layout)? From tree-core.h it looks like addressable_flag, static_flag,
public_flag, asm_written_flag, nothrow_flag, saturating_flag,
default_def_flag are all unused on FIELD_DECL.
Because of the -Wpsabi diagnostics that (some) targets want, having
just one bit "ignore this FIELD_DECL in ABI decisions" is not good enough.
If we want to use some bit, perhaps
(FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
could be used to mark something, either the C++17 empty bases only,
or both those and [[no_unique_address]] and then the backends could use
lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (field))
to find out if it is the C++14 vs. C++17 ABI incompatibility, or the
all C++ versions [[no_unique_address]] thing.
That sounds good.
Jason
Re: [PATCH, PR94769] fortran/io.c: Fix use of uninitialized variable num
On Tue, Apr 28, 2020 at 2:44 PM Stefan Schulze Frielinghaus via
Gcc-patches wrote:
>
> While bootstrapping GCC on S/390 the following warning occurs:
>
> gcc/fortran/io.c: In function 'bool gfc_resolve_dt(gfc_code*, gfc_dt*,
> locus*)':
> gcc/fortran/io.c:3857:7: error: 'num' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> 3857 | if (num == 0)
> | ^~
> gcc/fortran/io.c:3843:11: note: 'num' was declared here
> 3843 | int num;
>
> Since gfc_resolve_dt is a non-static function we cannot assume anything about
> argument DT. Argument DT gets passed to function check_io_constraints which
> passes values depending on DT, namely dt->asynchronous->value.character.string
> to function compare_to_allowed_values as well as argument warn which is true
> as
> soon as DT->dterr is true. Thus both arguments depend on DT.
>
> If function compare_to_allowed_values is called with
> dt->asynchronous->value.character.string not being an allowed value, and
> ALLOWED_F2003 as well as ALLOWED_GNU being NULL (which is the case at the
> particular call side), and WARN equals true, then the function returns with a
> non-zero value and leaves num uninitialized which renders the warning true.
>
> Initializing num to any value but zero mimics the behavior of an uninitialized
> variable except UB because zero is the only value it is tested for at the
> moment. Since num is used as an index into array asynchronous initializing it
> to 2 seems plausible.
>
> Bootstrapped and regtested on S/390. Ok for master?
You need to CC fortran patches to fort...@gcc.gnu.org (done hereby)
Richard.
> gcc/fortran/ChangeLog:
>
> 2020-04-28 Stefan Schulze Frielinghaus
>
> PR fortran/94769
> * io.c (check_io_constraints): Initialize local variable num.
> ---
> gcc/fortran/io.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c
> index e06e01d..4526f729d1d 100644
> --- a/gcc/fortran/io.c
> +++ b/gcc/fortran/io.c
> @@ -3840,7 +3840,7 @@ if (condition) \
>
>if (dt->asynchronous)
> {
> - int num;
> + int num = 2;
>static const char * asynchronous[] = { "YES", "NO", NULL };
>
>/* Note: gfc_reduce_init_expr reports an error if not init-expr. */
> --
> 2.25.3
>
Re: [RFC] Clarify -ffinite-math-only documentation
On Tue, Apr 28, 2020 at 12:04 PM Richard Sandiford
wrote:
>
> Matthias Kretz writes:
> > On Dienstag, 28. April 2020 09:21:38 CEST Richard Biener wrote:
> >> On Mon, Apr 27, 2020 at 11:26 PM Matthias Kretz wrote:
> >> > * Why not disable NaN and Inf independently? Inf is just a reciprocal 0.
> >> > Inf is as far away from numeric_limits::max as 0 is from
> >> > numeric_limits::min (infinitely many steps on a floating-point "type"
> >> > with infinite exponent).
> >>
> >> Just a comment on the last point - internally it's already split into
> >> HONOR_NANS and HONOR_INFINITIES that there's just
> >> a single user-facing option -ffinite-math-only is historical (IMHO more
> >> options are more opportunities for users to get confused ;)).
> >>
> >> Maybe also interesting to the discussion the internal comments say
> >>
> >> /* True if the given mode has a NaN representation and the treatment of
> >>NaN operands is important. Certain optimizations, such as folding
> >>x * 0 into 0, are not correct for NaN operands, and are normally
> >>disabled for modes with NaNs. The user can ask for them to be
> >>done anyway using the -funsafe-math-optimizations switch. */
> >> extern bool HONOR_NANS (machine_mode);
> >> [...]
> >> note how the comment says "treatment of NaN operands is important"
> >
> > ... if HONOR_NANS(mode) == true. So with -ffinite-math-only that implies
> > "treatment of NaN operands is not important", right?
> >
> >> and the example of simplifying x * 0 to 0 is about preserving NaNs
> >> during expression simplification when the FPU would.
> >
> > Yes, the `nan * 0 -> 0` simplification with -ffinite-math-only is why I'm
> > saying the status quo already has no NaN representation *in C/C++* anymore.
> > Because a float with NaN bits doesn't behave like C17 5.2.4.2.2/4 requires.
> > And thus it's an invalid/unspecified/undefined value, not a NaN. If there
> > are
> > no NaNs (as defined by C), then there is no NaN representation and
> > __FLT_HAS_QUIET_NAN__ must be 0.
> >
> >> I think these
> >> kind of optimizations are what originally was intended to be allowed
> >> with -ffinite-math-only - that we started to simplify isnan(x) to false
> >> was extending the scope and that change wasn't uncontested since
> >> it makes -ffinite-math-only less useful to some people.
> >
> > I understand and agree with this point. However I think the problem with the
> > current -ffinite-math-only is that it doesn't do enough. I.e. it's a
> > dangerous
> > optimization for people who rely on nan+inf in some way. But in many cases
> > people don't realize they do, and it may be a latent issue that turns into a
> > bug on unrelated code changes (or compiler changes). By going all the way,
> > aggressively optimizing assuming there can't ever be nan/inf in the program,
> > making the use of inf/nan ill-formed NDR, and integrating with ubsan, I
> > think
> > the option becomes less dangerous and more useful. That would be a bold
> > move,
> > certainly.
>
> I think I understand your position, but what about options like
> -fassociative-math:
>
> --
> Allow re-association of operands in series of floating-point operations.
> This violates the ISO C and C++ language standard by possibly changing
> computation result. NOTE: re-ordering may change the sign of zero as
> well as ignore NaNs and inhibit or create underflow or overflow (and
> thus cannot be used on code that relies on rounding behavior like
> @code{(x + 2**52) - 2**52}. May also reorder floating-point comparisons
> and thus may not be used when ordered comparisons are required.
> This option requires that both @option{-fno-signed-zeros} and
> @option{-fno-trapping-math} be in effect. Moreover, it doesn't make
> much sense with @option{-frounding-math}. For Fortran the option
> is automatically enabled when both @option{-fno-signed-zeros} and
> @option{-fno-trapping-math} are in effect.
> --
Just to add some additional information - this option (like a few others)
was added in the attempt to decompose the kitchen-sink
-funsafe-math-optimizations
into more specific bits based on the kind of transforms they allow.
That is in contrast to the set of options trying to enable/disable certain
parts of the IEEE specs. But of course both sets interact and certain
combinations of options do not make sense (some are rejected and
some options enable/disable other options automatically).
Overall it's quite a mess ;)
Usually I advocate an all-or-nothing approach - if -ffast-math works for
you use it, otherwise don't try to be "clever" with any of the more
specific flags.
> The point about NaN handling doesn't seem obviously different from the
> X * 0 -> 0 case. But on its own, -fassociative-math doesn't give us
> the freedom to make __builtin_isnan always return false. I.e. with
> -fassociative-math on its
[committed] analyzer: remove -Wanalyzer-use-of-uninitialized-value for GCC 10
>From what I can tell -Wanalyzer-use-of-uninitialized-value has not
yet found a true diagnostic in real-world code, and seems to be
particularly susceptible to false positives. These relate to bugs in
the region_model code.
For GCC 10 it seems best to remove this warning, which this patch does.
Internally it also removes POISON_KIND_UNINIT.
I'm working on a rewrite of the region_model code for GCC 11 that I
hope will fix these issues, and allow this warning to be reintroduced.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to master as r10-8012-g78b9783774bfd3540f38f5b1e3c7fc9f719653d7.
gcc/analyzer/ChangeLog:
PR analyzer/94447
PR analyzer/94639
PR analyzer/94732
PR analyzer/94754
* analyzer.opt (Wanalyzer-use-of-uninitialized-value): Delete.
* program-state.cc (selftest::test_program_state_dumping): Update
expected dump result for removal of "uninit".
* region-model.cc (poison_kind_to_str): Delete POISON_KIND_UNINIT
case.
(root_region::ensure_stack_region): Initialize stack with null
svalue_id rather than with a typeless POISON_KIND_UNINIT value.
(root_region::ensure_heap_region): Likewise for the heap.
(region_model::dump_summary_of_rep_path_vars): Remove
summarization of uninit values.
(region_model::validate): Remove check that the stack has a
POISON_KIND_UNINIT value.
(poisoned_value_diagnostic::emit): Remove POISON_KIND_UNINIT
case.
(poisoned_value_diagnostic::describe_final_event): Likewise.
(selftest::test_dump): Update expected dump result for removal of
"uninit".
(selftest::test_svalue_equality): Remove "uninit" and "freed".
* region-model.h (enum poison_kind): Remove POISON_KIND_UNINIT.
gcc/ChangeLog:
PR analyzer/94447
PR analyzer/94639
PR analyzer/94732
PR analyzer/94754
* doc/invoke.texi (Static Analyzer Options): Remove
-Wanalyzer-use-of-uninitialized-value.
(-Wno-analyzer-use-of-uninitialized-value): Remove item.
gcc/testsuite/ChangeLog:
PR analyzer/94447
PR analyzer/94639
PR analyzer/94732
PR analyzer/94754
* gcc.dg/analyzer/data-model-1.c: Mark "use of uninitialized
value" warnings as xfail for now.
* gcc.dg/analyzer/data-model-5b.c: Remove uninitialized warning.
* gcc.dg/analyzer/pr94099.c: Mark "uninitialized" warning as xfail
for now.
* gcc.dg/analyzer/pr94447.c: New test.
* gcc.dg/analyzer/pr94639.c: New test.
* gcc.dg/analyzer/pr94732.c: New test.
* gcc.dg/analyzer/pr94754.c: New test.
* gcc.dg/analyzer/zlib-6.c: Mark "uninitialized" warning as xfail
for now.
---
gcc/analyzer/analyzer.opt | 4 -
gcc/analyzer/program-state.cc | 14 ++--
gcc/analyzer/region-model.cc | 75 ++-
gcc/analyzer/region-model.h | 3 -
gcc/doc/invoke.texi | 10 ---
gcc/testsuite/gcc.dg/analyzer/data-model-1.c | 4 +-
gcc/testsuite/gcc.dg/analyzer/data-model-5b.c | 3 +-
gcc/testsuite/gcc.dg/analyzer/pr94099.c | 2 +-
gcc/testsuite/gcc.dg/analyzer/pr94447.c | 10 +++
gcc/testsuite/gcc.dg/analyzer/pr94639.c | 14
gcc/testsuite/gcc.dg/analyzer/pr94732.c | 13
gcc/testsuite/gcc.dg/analyzer/pr94754.c | 20 +
gcc/testsuite/gcc.dg/analyzer/zlib-6.c| 2 +-
13 files changed, 75 insertions(+), 99 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr94447.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr94639.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr94732.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr94754.c
diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 22cf4b0ad3b..3133cdd4106 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -98,10 +98,6 @@ Wanalyzer-use-of-pointer-in-stale-stack-frame
Common Var(warn_analyzer_use_of_pointer_in_stale_stack_frame) Init(1) Warning
Warn about code paths in which a pointer to a stale stack frame is used.
-Wanalyzer-use-of-uninitialized-value
-Common Var(warn_analyzer_use_of_uninitialized_value) Init(1) Warning
-Warn about code paths in which an uninitialized value is used.
-
Wanalyzer-too-complex
Common Var(warn_analyzer_too_complex) Init(0) Warning
Warn if the code is too complicated for the analyzer to fully explore.
diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index 43396c65827..1a5843be16d 100644
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -1449,23 +1449,21 @@ test_program_state_dumping ()
ASSERT_DUMP_EQ
(s, ext_state, false,
"rmodel: r0: {kind: `root', parent: null, sval: null}\n"
- "|-heap: r1: {kind: `heap', parent: r0, sval: sv0}\n"
- "| |: sval: sv0: {p
[PATCH] c++: Parameter pack in requires parameter list [PR94808]
When printing the substituted parameter list of a requires-expression as
part of the "in requirements with ..." context line during concepts
diagnostics, we weren't considering that substitution into a parameter
pack can yield zero or multiple parameters.
Since this patch affects only concepts diagnostics, so far I tested with
RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer ICE
with the unreduced testcase in the PR. Is this OK to commit after a
full bootstrap and regtest?
gcc/cp/ChangeLog:
PR c++/94808
* error.c (print_requires_expression_info): Substitute into and
collect all parameters in a vector first before printing them.
Handle zero or multiple parameters of an expanded parameter
pack.
gcc/testsuite/ChangeLog:
PR c++/94808
* g++.dg/concepts/diagnostic12.C: New test.
---
gcc/cp/error.c | 21 ++--
gcc/testsuite/g++.dg/concepts/diagnostic12.C | 14 +
2 files changed, 29 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 98c163db572..a314412988f 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -3752,8 +3752,7 @@ print_requires_expression_info (diagnostic_context
*context, tree constr, tree a
pp_verbatim (context->printer, "in requirements ");
tree parms = TREE_OPERAND (expr, 0);
- if (parms)
-pp_verbatim (context->printer, "with ");
+ auto_vec substed_parms;
while (parms)
{
tree next = TREE_CHAIN (parms);
@@ -3761,15 +3760,25 @@ print_requires_expression_info (diagnostic_context
*context, tree constr, tree a
TREE_CHAIN (parms) = NULL_TREE;
cp_unevaluated u;
tree p = tsubst (parms, args, tf_none, NULL_TREE);
- pp_verbatim (context->printer, "%q#D", p);
+ while (p)
+ {
+ substed_parms.safe_push (p);
+ p = TREE_CHAIN (p);
+ }
TREE_CHAIN (parms) = next;
- if (next)
-pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
-
parms = next;
}
+ for (unsigned i = 0; i < substed_parms.length (); i++)
+{
+ if (i == 0)
+ pp_verbatim (context->printer, "with ");
+ else
+ pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
+ pp_verbatim (context->printer, "%q#D", substed_parms[i]);
+}
+
pp_verbatim (context->printer, "\n");
}
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic12.C
b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
new file mode 100644
index 000..7dd286291f3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
@@ -0,0 +1,14 @@
+// PR c++/94808
+// { dg-do compile { target concepts } }
+
+template
+ concept c1 = requires (T t, Args... args) { *t; };
+// { dg-message "in requirements with .int t." "" { target *-*-* } .-1 }
+
+static_assert(c1); // { dg-error "failed" }
+
+template
+ concept c2 = requires (T t, Args... args) { *t; };
+// { dg-message "in requirements with .int t., .char args#0., .bool args#1."
"" { target *-*-* } .-1 }
+
+static_assert(c2); // { dg-error "failed" }
--
2.26.2.266.ge870325ee8
[PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
Hi!
On Tue, Apr 28, 2020 at 08:53:31AM -0400, Jason Merrill wrote:
> That sounds good.
So like this? Or better name for the new macro?
The calls.h macro is there only after all the backends are converted
to use ABI_IGNORED_FIELD_P.
Not sure if I shouldn't
if (lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (field))
ABI_IGNORED_FIELD_P (field) = 1;
in end_of_class, as there seems to be some ObjC++ partially overlapping
case too and probably we don't want to change ABI for that.
This patch is a merge of the powerpc64le-linux patch for
[[no_unique_address]] and the powerpc*-{darwin,aix}* patch with the
langhook etc. removed for ABI_IGNORED_FIELD_P.
Untested so far.
2020-04-28 Jakub Jelinek
PR target/94707
* tree-core.h (tree_decl_common): Note decl_flag_0 used for
ABI_IGNORED_FIELD_P.
* tree.h (ABI_IGNORED_FIELD_P): Define.
* calls.h (cxx17_empty_base_field_p): Change into a temporary
macro, check ABI_IGNORED_FIELD_P flag with no "no_unique_address"
attribute.
* calls.c (cxx17_empty_base_field_p): Remove.
* tree-streamer-out.c (pack_ts_decl_common_value_fields): Handle
ABI_IGNORED_FIELD_P.
* tree-streamer-in.c (unpack_ts_decl_common_value_fields): Likewise.
* lto-streamer-out.c (hash_tree): Likewise.
* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, use ABI_IGNORED_FIELD_P instead of
cxx17_empty_base_field_p, if "no_unique_address" attribute is
present, propagate that to the caller too.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* config/rs6000/rs6000.c (rs6000_special_round_type_align,
darwin_rs6000_special_round_type_align): Skip ABI_IGNORED_FIELD_P
fields.
* class.c (build_base_field): Set ABI_IGNORED_FIELD_P on C++17 empty
base artificial FIELD_DECLs.
(end_of_class): Set ABI_IGNORED_FIELD_P on empty class
field_poverlapping_p FIELD_DECLs.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
* g++.target/powerpc/pr94707-5.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/tree-core.h.jj 2020-04-08 18:15:36.936946772 +0200
+++ gcc/tree-core.h 2020-04-28 15:14:06.598814022 +0200
@@ -1709,7 +1709,8 @@ struct GTY(()) tree_decl_common {
unsigned lang_flag_8 : 1;
/* In VAR_DECL and PARM_DECL, this is DECL_REGISTER
- IN TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P. */
+ In TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
+ In FIELD_DECL, this is ABI_IGNORED_FIELD_P. */
unsigned decl_flag_0 : 1;
/* In FIELD_DECL, this is DECL_BIT_FIELD
In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
--- gcc/tree.h.jj 2020-04-08 18:15:36.939946727 +0200
+++ gcc/tree.h 2020-04-28 15:13:07.579695258 +0200
@@ -2750,6 +2750,13 @@ extern void decl_value_expr_insert (tree
/* In a FIELD_DECL, indicates this field should be bit-packed. */
#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->base.u.bits.packed_flag)
+/* In a FIELD_DECL, indicates this field should be ignored for ABI decisions
+ like passing/returning containing struct by value.
+ Set for C++17 empty base artificial FIELD_DECLs as well as
+ empty [[no_unique_address]] non-static data members. */
+#define ABI_IGNORED_FIELD_P(NODE) \
+ (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
+
/* Nonzero in a FIELD_DECL means it is a bit field, and must be accessed
specially. */
#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_1)
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 15:26:29.221724466 +0200
@@ -135,6 +135,9 @@ extern tree get_attr_nonstring_decl (tre
extern void maybe_warn_nonstring_arg (tree, tree);
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
-extern bool cxx17_empty_base_field_p (const_tree);
+/* FIXME: Remove after all backends are converted. */
+#define cxx17_empty_base_field_p(t) \
+ (ABI_IGNORED_FIELD_P (t) \
+ && !lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)))
#endif // GCC_CALLS_H
--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
+++ gcc/calls.c 2020-04-28 15:26:42.276529517 +0200
@@ -6261,23 +6261,5 @@ must_pass_va_arg_in_stack (tree type)
return targetm.calls.must_pass_in_stack (arg);
}
-/* Return true if FIELD is the C++17 empty base field that should
- be igno
Re: [PATCH, PR94769] fortran/io.c: Fix use of uninitialized variable num
On Tue, Apr 28, 2020 at 3:23 PM Jakub Jelinek via Gcc-patches
wrote:
>
> On Tue, Apr 28, 2020 at 01:53:07PM +0200, Stefan Schulze Frielinghaus via
> Gcc-patches wrote:
> > gcc/fortran/ChangeLog:
> >
> > 2020-04-28 Stefan Schulze Frielinghaus
> >
> > PR fortran/94769
> > * io.c (check_io_constraints): Initialize local variable num.
> > ---
> > gcc/fortran/io.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c
> > index e06e01d..4526f729d1d 100644
> > --- a/gcc/fortran/io.c
> > +++ b/gcc/fortran/io.c
> > @@ -3840,7 +3840,7 @@ if (condition) \
> >
> >if (dt->asynchronous)
> > {
> > - int num;
> > + int num = 2;
> >static const char * asynchronous[] = { "YES", "NO", NULL };
>
> Just nitpicking, wouldn't -1 be more usual value?
> And, I think there should be an assertion that it didn't remain -1 after the
> call, above
> /* For "YES", mark related symbols as asynchronous. */
> do
> gcc_checking (num != -1);
> or so.
>
> Note, the reason why this triggers only on s390x is the vastly different
> inlining parameters the target uses, that causes a lot of headaches
> everywhere.
... which also do not get adjusted when the global settings do.
s390 param_max_inline_insns_auto is now (for >= z13) more than
five times the default one! param_inline_min_speedup is also
wy off (2 vs. 30)
But it's always the target maintainers call ... (note that -finline-functions
is now enabled at -O2 so this will _vastly_ increase binary size there).
Honza, I guess you didn't consider targets playing with those defaults?
A better approach than setting a different absolute value per target
is to scale the default with a factor.
Richard.
> Jakub
>
Re: [PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
On 4/28/20 10:04 AM, Jakub Jelinek wrote:
Hi!
On Tue, Apr 28, 2020 at 08:53:31AM -0400, Jason Merrill wrote:
That sounds good.
So like this? Or better name for the new macro?
The calls.h macro is there only after all the backends are converted
to use ABI_IGNORED_FIELD_P.
Not sure if I shouldn't
if (lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (field))
ABI_IGNORED_FIELD_P (field) = 1;
in end_of_class, as there seems to be some ObjC++ partially overlapping
case too and probably we don't want to change ABI for that.
I would think it would make sense to set it here:
else if (might_overlap && is_empty_class (type))
layout_empty_base_or_field (rli, field, empty_base_offsets);
Jason
Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]
On 28.04.20 14:13, Jakub Jelinek wrote:
> Hi!
>
> So, based on the yesterday's discussions, similarly to powerpc64le-linux
> I've done some testing for s390x-linux too.
>
> First of all, I found a bug in my patch from yesterday, it was printing
> the wrong type like 'double' etc. rather than the class that contained such
> the element. Fix below.
>
> For s390x-linux, I was using
> struct X { };
> struct Y { int : 0; };
> struct Z { int : 0; Y y; };
> struct U : public X { X q; };
> struct A { double a; };
> struct B : public X { double a; };
> struct C : public Y { double a; };
> struct D : public Z { double a; };
> struct E : public U { double a; };
> struct F { [[no_unique_address]] X x; double a; };
> struct G { [[no_unique_address]] Y y; double a; };
> struct H { [[no_unique_address]] Z z; double a; };
> struct I { [[no_unique_address]] U u; double a; };
> struct J { double a; [[no_unique_address]] X x; };
> struct K { double a; [[no_unique_address]] Y y; };
> struct L { double a; [[no_unique_address]] Z z; };
> struct M { double a; [[no_unique_address]] U u; };
> #define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s
> (s); return 0; }
> T (A, a)
> T (B, b)
> T (C, c)
> T (D, d)
> T (E, e)
> T (F, f)
> T (G, g)
> T (H, h)
> T (I, i)
> T (J, j)
> T (K, k)
> T (L, l)
> T (M, m)
> as testcase and looking for "\tld\t%f0,".
> While g++ 9 with -std=c++17 used to pass in fpr just
> A, g++ 9 -std=c++14, as well as current trunk -std=c++14 & 17
> and clang++ from today -std=c++14 & 17 all pass A, B, C
> in fpr and nothing else. The intent stated by Jason seems to be
> that A, B, C, F, G, J, K should all be passed in fpr. So, shall
> we change both GCC and clang? Or is the published ABI clear on this?
> Or does it need clarification?
Our ABI doesn't specify anything regarding C++ so there is nothing in there
which really conflicts
with that. I assume these things will be part of a cross-platform C++ ABI
instead? I don't see a way
to add this to our platform ABI without introducing C++ in general there.
Since "no_unique_address" is a new feature we want to pick whatever is most
efficient and matches
what other archs do. Passing F, G, J, K in FPRs looks reasonable to me.
Given that this is something which hasn't been covered by the ABI so far I'm
not sure we really need
a -Wpsabi warning for that.
I also checked with Ulrich. He agrees with that approach and will have a look
at the LLVM side.
Btw. is the no_unique_address flag visible also in Dwarf? Probably it is enough
for GDB to just
observe the effect of it to decide how to pass arguments?!
Thanks for doing these investigations!
Andreas
>
> 2020-04-28 Jakub Jelinek
>
> PR target/94704
> * config/s390/s390.c (s390_function_arg_vector,
> s390_function_arg_float): In -Wpsabi diagnostics use the type
> passed to the function rather than the type of the single element.
Ok. Thanks!
Andreas
>
> --- gcc/config/s390/s390.c.jj 2020-04-28 10:26:08.499984223 +0200
> +++ gcc/config/s390/s390.c2020-04-28 14:01:31.813712290 +0200
> @@ -11912,6 +11912,7 @@ s390_function_arg_vector (machine_mode m
>/* The ABI says that record types with a single member are treated
> just like that member would be. */
>bool cxx17_empty_base_seen = false;
> + const_tree orig_type = type;
>while (TREE_CODE (type) == RECORD_TYPE)
> {
>tree field, single = NULL_TREE;
> @@ -11958,7 +11959,7 @@ s390_function_arg_vector (machine_mode m
> last_reported_type_uid = uid;
> inform (input_location, "parameter passing for argument of type "
> "%qT when C++17 is enabled changed to match "
> - "C++14 in GCC 10.1", type);
> + "C++14 in GCC 10.1", orig_type);
> }
> }
>return true;
> @@ -11984,6 +11985,7 @@ s390_function_arg_float (machine_mode mo
>/* The ABI says that record types with a single member are treated
> just like that member would be. */
>bool cxx17_empty_base_seen = false;
> + const_tree orig_type = type;
>while (TREE_CODE (type) == RECORD_TYPE)
> {
>tree field, single = NULL_TREE;
> @@ -12022,7 +12024,7 @@ s390_function_arg_float (machine_mode mo
> last_reported_type_uid = uid;
> inform (input_location, "parameter passing for argument of type "
> "%qT when C++17 is enabled changed to match "
> - "C++14 in GCC 10.1", type);
> + "C++14 in GCC 10.1", orig_type);
> }
> }
>
>
> Jakub
>
Re: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
On 4/28/20 9:48 AM, Patrick Palka wrote:
When printing the substituted parameter list of a requires-expression as
part of the "in requirements with ..." context line during concepts
diagnostics, we weren't considering that substitution into a parameter
pack can yield zero or multiple parameters.
Since this patch affects only concepts diagnostics, so far I tested with
RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer ICE
with the unreduced testcase in the PR. Is this OK to commit after a
full bootstrap and regtest?
OK.
Though I wonder about printing the dependent form and template arguments
instead. Do you have an opinion?
gcc/cp/ChangeLog:
PR c++/94808
* error.c (print_requires_expression_info): Substitute into and
collect all parameters in a vector first before printing them.
Handle zero or multiple parameters of an expanded parameter
pack.
gcc/testsuite/ChangeLog:
PR c++/94808
* g++.dg/concepts/diagnostic12.C: New test.
---
gcc/cp/error.c | 21 ++--
gcc/testsuite/g++.dg/concepts/diagnostic12.C | 14 +
2 files changed, 29 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 98c163db572..a314412988f 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -3752,8 +3752,7 @@ print_requires_expression_info (diagnostic_context
*context, tree constr, tree a
pp_verbatim (context->printer, "in requirements ");
tree parms = TREE_OPERAND (expr, 0);
- if (parms)
-pp_verbatim (context->printer, "with ");
+ auto_vec substed_parms;
while (parms)
{
tree next = TREE_CHAIN (parms);
@@ -3761,15 +3760,25 @@ print_requires_expression_info (diagnostic_context
*context, tree constr, tree a
TREE_CHAIN (parms) = NULL_TREE;
cp_unevaluated u;
tree p = tsubst (parms, args, tf_none, NULL_TREE);
- pp_verbatim (context->printer, "%q#D", p);
+ while (p)
+ {
+ substed_parms.safe_push (p);
+ p = TREE_CHAIN (p);
+ }
TREE_CHAIN (parms) = next;
- if (next)
-pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
-
parms = next;
}
+ for (unsigned i = 0; i < substed_parms.length (); i++)
+{
+ if (i == 0)
+ pp_verbatim (context->printer, "with ");
+ else
+ pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
+ pp_verbatim (context->printer, "%q#D", substed_parms[i]);
+}
+
pp_verbatim (context->printer, "\n");
}
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic12.C b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
new file mode 100644
index 000..7dd286291f3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
@@ -0,0 +1,14 @@
+// PR c++/94808
+// { dg-do compile { target concepts } }
+
+template
+ concept c1 = requires (T t, Args... args) { *t; };
+// { dg-message "in requirements with .int t." "" { target *-*-* } .-1 }
+
+static_assert(c1); // { dg-error "failed" }
+
+template
+ concept c2 = requires (T t, Args... args) { *t; };
+// { dg-message "in requirements with .int t., .char args#0., .bool args#1."
"" { target *-*-* } .-1 }
+
+static_assert(c2); // { dg-error "failed" }
Re: gcc.dg testsuite glitches
On 4/28/20 5:12 AM, Manfred Schwarb wrote: Hi, first, I do not have commit rights, so please somebody check and commit, I guess this goes under the obvious and trivial rules. There are several malformed dejagnu directives in the gcc.dg testsuite. Below I fixed some of them following these criteria: - fix mis-typed directives - require that the outermost braces are space padded - fix imbalanced braces Several of these changes are no-ops, as nonsensical directives act as "dg-do compile", but they should be fixed nevertheless, IMO. thanks, these look good, but gonna wait until stage 1. nathan -- Nathan Sidwell
Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]
On Tue, Apr 28, 2020 at 04:23:24PM +0200, Andreas Krebbel wrote: > Our ABI doesn't specify anything regarding C++ so there is nothing in there > which really conflicts > with that. I assume these things will be part of a cross-platform C++ ABI > instead? I don't see a way > to add this to our platform ABI without introducing C++ in general there. > > Since "no_unique_address" is a new feature we want to pick whatever is most > efficient and matches > what other archs do. Passing F, G, J, K in FPRs looks reasonable to me. Ok, will tweak the patch then once the powerpc+generic one is finalized. > Given that this is something which hasn't been covered by the ABI so far I'm > not sure we really need > a -Wpsabi warning for that. But [[no_unique_address]] has been introduced already in GCC 9, so e.g. in the powerpc64le patch I want to warn users about the ABI incompatibility when padding those. Your call though. > Btw. is the no_unique_address flag visible also in Dwarf? Probably it is > enough for GDB to just > observe the effect of it to decide how to pass arguments?! Dunno. Jakub
[wwwdocs] Update cxx1y links to cxx-status
While looking up C++14 information, I noticed that some links in current navigation pages refer to cxx1y.html instead of cxx-status.html. This patch changes the NEWS item to refer to cxx-status.html#cxx14 and the Projects index to refer to C++ language features instead of C++14 language features. This makes it easier for users to find the cxx-status.html page. Committed. * news.html: GCC 5 C++14 complete announcement. * projects/index.html: C++14 projects changed to C++ projects. diff --git a/htdocs/news.html b/htdocs/news.html index dd4e1c09..0dd6b265 100644 --- a/htdocs/news.html +++ b/htdocs/news.html @@ -171,7 +171,7 @@ GCC 5 C++14 language feature-complete [2014-12-23] -Support for all C++14 language +Support for all C++14 language features has been added to the development sources for GCC, and will be available when GCC 5 is released next year. Contributed by Jason Merrill, Braden Obrzut, Adam Butcher, Edward Smith-Rowland, diff --git a/htdocs/projects/index.html b/htdocs/projects/index.html index 282d79eb..0647ed6b 100644 --- a/htdocs/projects/index.html +++ b/htdocs/projects/index.html @@ -29,7 +29,7 @@ help develop GCC: inadequacies. Projects for the C preprocessor. Projects for improving the C front end. -Implementing new C++14 features. +Implementing new C++ features. The GNU UPC Project. Improve the installation procedure Simpler porting
Re: [PATCH] tsan: Add optional support for distinguishing volatiles
On Thu, Apr 23, 2020 at 5:43 PM Marco Elver wrote: > > Add support to optionally emit different instrumentation for accesses to > volatile variables. While the default TSAN runtime likely will never > require this feature, other runtimes for different environments that > have subtly different memory models or assumptions may require > distinguishing volatiles. > > One such environment are OS kernels, where volatile is still used in > various places for various reasons, and often declare volatile to be > "safe enough" even in multi-threaded contexts. One such example is the > Linux kernel, which implements various synchronization primitives using > volatile (READ_ONCE(), WRITE_ONCE()). Here the Kernel Concurrency > Sanitizer (KCSAN) [1], is a runtime that uses TSAN instrumentation but > otherwise implements a very different approach to race detection from > TSAN. > > While in the Linux kernel it is generally discouraged to use volatiles > explicitly, the topic will likely come up again, and we will eventually > need to distinguish volatile accesses [2]. The other use-case is > ignoring data races on specially marked variables in the kernel, for > example bit-flags (here we may hide 'volatile' behind a different name > such as 'no_data_race'). > > [1] https://github.com/google/ktsan/wiki/KCSAN > [2] > https://lkml.kernel.org/r/CANpmjNOfXNE-Zh3MNP=-gmnhvKbsfUfTtWkyg_=vqtxs4nn...@mail.gmail.com Hi Jakub, FWIW this is: Acked-by: Dmitry Vyukov We just landed a similar change to llvm: https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 Do you have any objections? Yes, I know volatile is not related to threading :) But 5 years we have a similar patch for gcc for another race detector prototype: https://gist.github.com/xairy/862ba3260348efe23a37decb93aa79e9 So this is not the first time this comes up. Thanks > 2020-04-23 Marco Elver > > gcc/ > * params.opt: Define --param=tsan-distinguish-volatile=[0,1]. > * sanitizer.def (BUILT_IN_TSAN_VOLATILE_READ1): Define new > builtin for volatile instrumentation of reads/writes. > (BUILT_IN_TSAN_VOLATILE_READ2): Likewise. > (BUILT_IN_TSAN_VOLATILE_READ4): Likewise. > (BUILT_IN_TSAN_VOLATILE_READ8): Likewise. > (BUILT_IN_TSAN_VOLATILE_READ16): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE1): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE2): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE4): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE8): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE16): Likewise. > * tsan.c (get_memory_access_decl): Argument if access is > volatile. If param tsan-distinguish-volatile is non-zero, and > access if volatile, return volatile instrumentation decl. > (instrument_expr): Check if access is volatile. > > gcc/testsuite/ > * c-c++-common/tsan/volatile.c: New test. > --- > gcc/ChangeLog | 19 +++ > gcc/params.opt | 4 ++ > gcc/sanitizer.def | 21 > gcc/testsuite/ChangeLog| 4 ++ > gcc/testsuite/c-c++-common/tsan/volatile.c | 62 ++ > gcc/tsan.c | 53 -- > 6 files changed, 146 insertions(+), 17 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/tsan/volatile.c > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog > index 5f299e463db..aa2bb98ae05 100644 > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -1,3 +1,22 @@ > +2020-04-23 Marco Elver > + > + * params.opt: Define --param=tsan-distinguish-volatile=[0,1]. > + * sanitizer.def (BUILT_IN_TSAN_VOLATILE_READ1): Define new > + builtin for volatile instrumentation of reads/writes. > + (BUILT_IN_TSAN_VOLATILE_READ2): Likewise. > + (BUILT_IN_TSAN_VOLATILE_READ4): Likewise. > + (BUILT_IN_TSAN_VOLATILE_READ8): Likewise. > + (BUILT_IN_TSAN_VOLATILE_READ16): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE1): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE2): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE4): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE8): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE16): Likewise. > + * tsan.c (get_memory_access_decl): Argument if access is > + volatile. If param tsan-distinguish-volatile is non-zero, and > + access if volatile, return volatile instrumentation decl. > + (instrument_expr): Check if access is volatile. > + > 2020-04-23 Srinath Parvathaneni > > * config/arm/arm_mve.h (__arm_vbicq_n_u16): Modify function > parameter's > diff --git a/gcc/params.opt b/gcc/params.opt > index 4aec480798b..9b564bb046c 100644 > --- a/gcc/params.opt > +++ b/gcc/params.opt > @@ -908,6 +908,10 @@ Stop reverse growth if the reverse probability of best > edge is less than this th > Common Joined UInteger Var(param_tree_reassoc_width) Param Optimization
Re: [PATCH] tsan: Add optional support for distinguishing volatiles
On Tue, Apr 28, 2020 at 04:48:31PM +0200, Dmitry Vyukov wrote: > FWIW this is: > > Acked-by: Dmitry Vyukov > > We just landed a similar change to llvm: > https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 > > Do you have any objections? I don't have objections or anything right now, we are just trying to finalize GCC 10 and once it branches, patches like this can be reviewed/committed for GCC11. Jakub
Re: [PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
Hi!
On Tue, Apr 28, 2020 at 10:18:44AM -0400, Jason Merrill wrote:
> I would think it would make sense to set it here:
>
> > else if (might_overlap && is_empty_class (type))
> > layout_empty_base_or_field (rli, field, empty_base_offsets);
That works too, plus on the IRC suggested change
ABI_IGNORED_FIELD_P -> DECL_FIELD_ABI_IGNORED.
Ok for trunk if it passes bootstrap/regtest on powerpc64{,le}-linux?
2020-04-28 Jakub Jelinek
PR target/94707
* tree-core.h (tree_decl_common): Note decl_flag_0 used for
DECL_FIELD_ABI_IGNORED.
* tree.h (DECL_FIELD_ABI_IGNORED): Define.
* calls.h (cxx17_empty_base_field_p): Change into a temporary
macro, check DECL_FIELD_ABI_IGNORED flag with no "no_unique_address"
attribute.
* calls.c (cxx17_empty_base_field_p): Remove.
* tree-streamer-out.c (pack_ts_decl_common_value_fields): Handle
DECL_FIELD_ABI_IGNORED.
* tree-streamer-in.c (unpack_ts_decl_common_value_fields): Likewise.
* lto-streamer-out.c (hash_tree): Likewise.
* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, use DECL_FIELD_ABI_IGNORED instead of
cxx17_empty_base_field_p, if "no_unique_address" attribute is
present, propagate that to the caller too.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* config/rs6000/rs6000.c (rs6000_special_round_type_align,
darwin_rs6000_special_round_type_align): Skip DECL_FIELD_ABI_IGNORED
fields.
* class.c (build_base_field): Set DECL_FIELD_ABI_IGNORED on C++17 empty
base artificial FIELD_DECLs.
(layout_class_type): Set DECL_FIELD_ABI_IGNORED on empty class
field_poverlapping_p FIELD_DECLs.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
* g++.target/powerpc/pr94707-5.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/tree-core.h.jj 2020-04-08 18:15:36.936946772 +0200
+++ gcc/tree-core.h 2020-04-28 15:14:06.598814022 +0200
@@ -1709,7 +1709,8 @@ struct GTY(()) tree_decl_common {
unsigned lang_flag_8 : 1;
/* In VAR_DECL and PARM_DECL, this is DECL_REGISTER
- IN TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P. */
+ In TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
+ In FIELD_DECL, this is DECL_FIELD_ABI_IGNORED. */
unsigned decl_flag_0 : 1;
/* In FIELD_DECL, this is DECL_BIT_FIELD
In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
--- gcc/tree.h.jj 2020-04-08 18:15:36.939946727 +0200
+++ gcc/tree.h 2020-04-28 15:13:07.579695258 +0200
@@ -2750,6 +2750,13 @@ extern void decl_value_expr_insert (tree
/* In a FIELD_DECL, indicates this field should be bit-packed. */
#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->base.u.bits.packed_flag)
+/* In a FIELD_DECL, indicates this field should be ignored for ABI decisions
+ like passing/returning containing struct by value.
+ Set for C++17 empty base artificial FIELD_DECLs as well as
+ empty [[no_unique_address]] non-static data members. */
+#define DECL_FIELD_ABI_IGNORED(NODE) \
+ (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
+
/* Nonzero in a FIELD_DECL means it is a bit field, and must be accessed
specially. */
#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_1)
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 15:26:29.221724466 +0200
@@ -135,6 +135,9 @@ extern tree get_attr_nonstring_decl (tre
extern void maybe_warn_nonstring_arg (tree, tree);
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
-extern bool cxx17_empty_base_field_p (const_tree);
+/* FIXME: Remove after all backends are converted. */
+#define cxx17_empty_base_field_p(t) \
+ (DECL_FIELD_ABI_IGNORED (t) \
+ && !lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)))
#endif // GCC_CALLS_H
--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
+++ gcc/calls.c 2020-04-28 15:26:42.276529517 +0200
@@ -6261,23 +6261,5 @@ must_pass_va_arg_in_stack (tree type)
return targetm.calls.must_pass_in_stack (arg);
}
-/* Return true if FIELD is the C++17 empty base field that should
- be ignored for ABI calling convention decisions in order to
- maintain ABI compatibility between C++14 and earlier, which doesn't
- add this FIELD to classes with empty bases, and C++17 and later
- which does. */
-
-bool
-cxx17_
Re: [PATCH] tsan: Add optional support for distinguishing volatiles
On Tue, Apr 28, 2020 at 4:55 PM Jakub Jelinek wrote: > > On Tue, Apr 28, 2020 at 04:48:31PM +0200, Dmitry Vyukov wrote: > > FWIW this is: > > > > Acked-by: Dmitry Vyukov > > > > We just landed a similar change to llvm: > > https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 > > > > Do you have any objections? > > I don't have objections or anything right now, we are just trying to > finalize GCC 10 and once it branches, patches like this can be > reviewed/committed for GCC11. Thanks for clarification! Then we will just wait.
Re: [PATCH, PR94774] tree-optimization: Fix use of uninitialized variable
On 4/27/20 10:58 AM, Stefan Schulze Frielinghaus wrote:
Array retval is not necessarily initialized by function is_call_safe and
may be used afterwards. Thus, initialize it explicitly.
Ok for master?
The change looks (even obviously) good to me but strictly speaking
it needs the approval of a middle end maintainer.
Thanks
Martin
gcc/ChangeLog:
2020-04-27 Stefan Schulze Frielinghaus
PR tree-optimization/94774
* gimple-ssa-sprintf.c (try_substitute_return_value): Initialize
variable retval.
---
gcc/gimple-ssa-sprintf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 1879686ce0a..011c3e21e63 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -4120,7 +4120,7 @@ try_substitute_return_value (gimple_stmt_iterator *gsi,
bool removed = false;
/* The minimum and maximum return value. */
- unsigned HOST_WIDE_INT retval[2];
+ unsigned HOST_WIDE_INT retval[2] = {0};
bool safe = is_call_safe (info, res, true, retval);
if (safe
Re: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
On 4/28/20 9:48 AM, Patrick Palka via Gcc-patches wrote: When printing the substituted parameter list of a requires-expression as part of the "in requirements with ..." context line during concepts diagnostics, we weren't considering that substitution into a parameter pack can yield zero or multiple parameters. Since this patch affects only concepts diagnostics, so far I tested with RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer ICE with the unreduced testcase in the PR. Is this OK to commit after a full bootstrap and regtest? gcc/cp/ChangeLog: PR c++/94808 * error.c (print_requires_expression_info): Substitute into and collect all parameters in a vector first before printing them. Handle zero or multiple parameters of an expanded parameter pack. ok, thanks -- Nathan Sidwell
Re: [PATCH, PR94769] fortran/io.c: Fix use of uninitialized variable num
Hi Stefan,
I prefer Jakub's suggestion – his change LGTM.
Cheers,
Tobias
On 4/28/20 2:30 PM, Jakub Jelinek via Gcc-patches wrote:
On Tue, Apr 28, 2020 at 01:53:07PM +0200, Stefan Schulze Frielinghaus via
Gcc-patches wrote:
gcc/fortran/ChangeLog:
2020-04-28 Stefan Schulze Frielinghaus
PR fortran/94769
* io.c (check_io_constraints): Initialize local variable num.
---
gcc/fortran/io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c
index e06e01d..4526f729d1d 100644
--- a/gcc/fortran/io.c
+++ b/gcc/fortran/io.c
@@ -3840,7 +3840,7 @@ if (condition) \
if (dt->asynchronous)
{
- int num;
+ int num = 2;
static const char * asynchronous[] = { "YES", "NO", NULL };
Just nitpicking, wouldn't -1 be more usual value?
And, I think there should be an assertion that it didn't remain -1 after the
call, above
/* For "YES", mark related symbols as asynchronous. */
do
gcc_checking (num != -1);
or so.
Note, the reason why this triggers only on s390x is the vastly different
inlining parameters the target uses, that causes a lot of headaches
everywhere.
Jakub
-
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] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]
On 4/28/20 10:38 AM, Jakub Jelinek wrote: On Tue, Apr 28, 2020 at 04:23:24PM +0200, Andreas Krebbel wrote: Our ABI doesn't specify anything regarding C++ so there is nothing in there which really conflicts with that. I assume these things will be part of a cross-platform C++ ABI instead? I don't see a way to add this to our platform ABI without introducing C++ in general there. Since "no_unique_address" is a new feature we want to pick whatever is most efficient and matches what other archs do. Passing F, G, J, K in FPRs looks reasonable to me. Ok, will tweak the patch then once the powerpc+generic one is finalized. Given that this is something which hasn't been covered by the ABI so far I'm not sure we really need a -Wpsabi warning for that. But [[no_unique_address]] has been introduced already in GCC 9, so e.g. in the powerpc64le patch I want to warn users about the ABI incompatibility when padding those. Your call though. Though C++20 support is still highly experimental, we're not trying to maintain compatibility. Btw. is the no_unique_address flag visible also in Dwarf? Probably it is enough for GDB to just observe the effect of it to decide how to pass arguments?! I agree that's probably enough. Jason
Re: [PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
Jakub, thanks for continuing to track down and fix all these cases.
I think this looks good. My only comment would be to please add some
comments in the test cases about the purpose, or at least to explain
the regexes in the scan-assembler-* directives, to save us all some
mental cycles in the future.
Need Segher/David to approve the rs6000 bits, of course.
Thanks!
Bill
On 4/28/20 9:04 AM, Jakub Jelinek via Gcc-patches wrote:
Hi!
On Tue, Apr 28, 2020 at 08:53:31AM -0400, Jason Merrill wrote:
That sounds good.
So like this? Or better name for the new macro?
The calls.h macro is there only after all the backends are converted
to use ABI_IGNORED_FIELD_P.
Not sure if I shouldn't
if (lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (field))
ABI_IGNORED_FIELD_P (field) = 1;
in end_of_class, as there seems to be some ObjC++ partially overlapping
case too and probably we don't want to change ABI for that.
This patch is a merge of the powerpc64le-linux patch for
[[no_unique_address]] and the powerpc*-{darwin,aix}* patch with the
langhook etc. removed for ABI_IGNORED_FIELD_P.
Untested so far.
2020-04-28 Jakub Jelinek
PR target/94707
* tree-core.h (tree_decl_common): Note decl_flag_0 used for
ABI_IGNORED_FIELD_P.
* tree.h (ABI_IGNORED_FIELD_P): Define.
* calls.h (cxx17_empty_base_field_p): Change into a temporary
macro, check ABI_IGNORED_FIELD_P flag with no "no_unique_address"
attribute.
* calls.c (cxx17_empty_base_field_p): Remove.
* tree-streamer-out.c (pack_ts_decl_common_value_fields): Handle
ABI_IGNORED_FIELD_P.
* tree-streamer-in.c (unpack_ts_decl_common_value_fields): Likewise.
* lto-streamer-out.c (hash_tree): Likewise.
* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, use ABI_IGNORED_FIELD_P instead of
cxx17_empty_base_field_p, if "no_unique_address" attribute is
present, propagate that to the caller too.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* config/rs6000/rs6000.c (rs6000_special_round_type_align,
darwin_rs6000_special_round_type_align): Skip ABI_IGNORED_FIELD_P
fields.
* class.c (build_base_field): Set ABI_IGNORED_FIELD_P on C++17 empty
base artificial FIELD_DECLs.
(end_of_class): Set ABI_IGNORED_FIELD_P on empty class
field_poverlapping_p FIELD_DECLs.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
* g++.target/powerpc/pr94707-5.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/tree-core.h.jj 2020-04-08 18:15:36.936946772 +0200
+++ gcc/tree-core.h 2020-04-28 15:14:06.598814022 +0200
@@ -1709,7 +1709,8 @@ struct GTY(()) tree_decl_common {
unsigned lang_flag_8 : 1;
/* In VAR_DECL and PARM_DECL, this is DECL_REGISTER
- IN TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P. */
+ In TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
+ In FIELD_DECL, this is ABI_IGNORED_FIELD_P. */
unsigned decl_flag_0 : 1;
/* In FIELD_DECL, this is DECL_BIT_FIELD
In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
--- gcc/tree.h.jj 2020-04-08 18:15:36.939946727 +0200
+++ gcc/tree.h 2020-04-28 15:13:07.579695258 +0200
@@ -2750,6 +2750,13 @@ extern void decl_value_expr_insert (tree
/* In a FIELD_DECL, indicates this field should be bit-packed. */
#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->base.u.bits.packed_flag)
+/* In a FIELD_DECL, indicates this field should be ignored for ABI decisions
+ like passing/returning containing struct by value.
+ Set for C++17 empty base artificial FIELD_DECLs as well as
+ empty [[no_unique_address]] non-static data members. */
+#define ABI_IGNORED_FIELD_P(NODE) \
+ (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
+
/* Nonzero in a FIELD_DECL means it is a bit field, and must be accessed
specially. */
#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK
(NODE)->decl_common.decl_flag_1)
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 15:26:29.221724466 +0200
@@ -135,6 +135,9 @@ extern tree get_attr_nonstring_decl (tre
extern void maybe_warn_nonstring_arg (tree, tree);
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
-extern bool cxx17_empty_base_field_p (const_tree);
+/* FIXME: Remove after all backends are converted. */
+#define cxx17_empty_base_field_p(t) \
+ (ABI_IGNORED_FIE
Re: [PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
On 4/28/20 10:57 AM, Jakub Jelinek wrote:
Hi!
On Tue, Apr 28, 2020 at 10:18:44AM -0400, Jason Merrill wrote:
I would think it would make sense to set it here:
else if (might_overlap && is_empty_class (type))
layout_empty_base_or_field (rli, field, empty_base_offsets);
That works too, plus on the IRC suggested change
ABI_IGNORED_FIELD_P -> DECL_FIELD_ABI_IGNORED.
Ok for trunk if it passes bootstrap/regtest on powerpc64{,le}-linux?
OK.
2020-04-28 Jakub Jelinek
PR target/94707
* tree-core.h (tree_decl_common): Note decl_flag_0 used for
DECL_FIELD_ABI_IGNORED.
* tree.h (DECL_FIELD_ABI_IGNORED): Define.
* calls.h (cxx17_empty_base_field_p): Change into a temporary
macro, check DECL_FIELD_ABI_IGNORED flag with no "no_unique_address"
attribute.
* calls.c (cxx17_empty_base_field_p): Remove.
* tree-streamer-out.c (pack_ts_decl_common_value_fields): Handle
DECL_FIELD_ABI_IGNORED.
* tree-streamer-in.c (unpack_ts_decl_common_value_fields): Likewise.
* lto-streamer-out.c (hash_tree): Likewise.
* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, use DECL_FIELD_ABI_IGNORED instead of
cxx17_empty_base_field_p, if "no_unique_address" attribute is
present, propagate that to the caller too.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* config/rs6000/rs6000.c (rs6000_special_round_type_align,
darwin_rs6000_special_round_type_align): Skip DECL_FIELD_ABI_IGNORED
fields.
* class.c (build_base_field): Set DECL_FIELD_ABI_IGNORED on C++17 empty
base artificial FIELD_DECLs.
(layout_class_type): Set DECL_FIELD_ABI_IGNORED on empty class
field_poverlapping_p FIELD_DECLs.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
* g++.target/powerpc/pr94707-5.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/tree-core.h.jj 2020-04-08 18:15:36.936946772 +0200
+++ gcc/tree-core.h 2020-04-28 15:14:06.598814022 +0200
@@ -1709,7 +1709,8 @@ struct GTY(()) tree_decl_common {
unsigned lang_flag_8 : 1;
/* In VAR_DECL and PARM_DECL, this is DECL_REGISTER
- IN TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P. */
+ In TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
+ In FIELD_DECL, this is DECL_FIELD_ABI_IGNORED. */
unsigned decl_flag_0 : 1;
/* In FIELD_DECL, this is DECL_BIT_FIELD
In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
--- gcc/tree.h.jj 2020-04-08 18:15:36.939946727 +0200
+++ gcc/tree.h 2020-04-28 15:13:07.579695258 +0200
@@ -2750,6 +2750,13 @@ extern void decl_value_expr_insert (tree
/* In a FIELD_DECL, indicates this field should be bit-packed. */
#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->base.u.bits.packed_flag)
+/* In a FIELD_DECL, indicates this field should be ignored for ABI decisions
+ like passing/returning containing struct by value.
+ Set for C++17 empty base artificial FIELD_DECLs as well as
+ empty [[no_unique_address]] non-static data members. */
+#define DECL_FIELD_ABI_IGNORED(NODE) \
+ (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
+
/* Nonzero in a FIELD_DECL means it is a bit field, and must be accessed
specially. */
#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK
(NODE)->decl_common.decl_flag_1)
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 15:26:29.221724466 +0200
@@ -135,6 +135,9 @@ extern tree get_attr_nonstring_decl (tre
extern void maybe_warn_nonstring_arg (tree, tree);
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
-extern bool cxx17_empty_base_field_p (const_tree);
+/* FIXME: Remove after all backends are converted. */
+#define cxx17_empty_base_field_p(t) \
+ (DECL_FIELD_ABI_IGNORED (t) \
+ && !lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)))
#endif // GCC_CALLS_H
--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
+++ gcc/calls.c 2020-04-28 15:26:42.276529517 +0200
@@ -6261,23 +6261,5 @@ must_pass_va_arg_in_stack (tree type)
return targetm.calls.must_pass_in_stack (arg);
}
-/* Return true if FIELD is the C++17 empty base field that should
- be ignored for ABI calling convention decisions in order to
- maintain ABI compatibility between C++14 and earlier, which doesn't
- add this FIELD to classes with
Re: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
On Tue, 28 Apr 2020, Jason Merrill wrote:
> On 4/28/20 9:48 AM, Patrick Palka wrote:
> > When printing the substituted parameter list of a requires-expression as
> > part of the "in requirements with ..." context line during concepts
> > diagnostics, we weren't considering that substitution into a parameter
> > pack can yield zero or multiple parameters.
> >
> > Since this patch affects only concepts diagnostics, so far I tested with
> > RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer ICE
> > with the unreduced testcase in the PR. Is this OK to commit after a
> > full bootstrap and regtest?
>
> OK.
Thanks for the review.
>
> Though I wonder about printing the dependent form and template arguments
> instead. Do you have an opinion?
I was going to say that on the one hand, it's convenient to have the
substituted form printed since it would let us to see through
complicated dependent type aliases, but it seems we don't strip type
aliases when pretty printing a parameter and its type with '%q#D'
anyway. And I can't think of any other possible advantage of printing
the substituted form.
So IMHO printing the dependent form and template arguments instead would
be better here. I'll try to write a patch for this today.
>
> > gcc/cp/ChangeLog:
> >
> > PR c++/94808
> > * error.c (print_requires_expression_info): Substitute into and
> > collect all parameters in a vector first before printing them.
> > Handle zero or multiple parameters of an expanded parameter
> > pack.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/94808
> > * g++.dg/concepts/diagnostic12.C: New test.
> > ---
> > gcc/cp/error.c | 21 ++--
> > gcc/testsuite/g++.dg/concepts/diagnostic12.C | 14 +
> > 2 files changed, 29 insertions(+), 6 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
> >
> > diff --git a/gcc/cp/error.c b/gcc/cp/error.c
> > index 98c163db572..a314412988f 100644
> > --- a/gcc/cp/error.c
> > +++ b/gcc/cp/error.c
> > @@ -3752,8 +3752,7 @@ print_requires_expression_info (diagnostic_context
> > *context, tree constr, tree a
> > pp_verbatim (context->printer, "in requirements ");
> > tree parms = TREE_OPERAND (expr, 0);
> > - if (parms)
> > -pp_verbatim (context->printer, "with ");
> > + auto_vec substed_parms;
> > while (parms)
> > {
> > tree next = TREE_CHAIN (parms);
> > @@ -3761,15 +3760,25 @@ print_requires_expression_info (diagnostic_context
> > *context, tree constr, tree a
> > TREE_CHAIN (parms) = NULL_TREE;
> > cp_unevaluated u;
> > tree p = tsubst (parms, args, tf_none, NULL_TREE);
> > - pp_verbatim (context->printer, "%q#D", p);
> > + while (p)
> > + {
> > + substed_parms.safe_push (p);
> > + p = TREE_CHAIN (p);
> > + }
> > TREE_CHAIN (parms) = next;
> > - if (next)
> > -pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
> > -
> > parms = next;
> > }
> > + for (unsigned i = 0; i < substed_parms.length (); i++)
> > +{
> > + if (i == 0)
> > + pp_verbatim (context->printer, "with ");
> > + else
> > + pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
> > + pp_verbatim (context->printer, "%q#D", substed_parms[i]);
> > +}
> > +
> > pp_verbatim (context->printer, "\n");
> > }
> > diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic12.C
> > b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
> > new file mode 100644
> > index 000..7dd286291f3
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
> > @@ -0,0 +1,14 @@
> > +// PR c++/94808
> > +// { dg-do compile { target concepts } }
> > +
> > +template
> > + concept c1 = requires (T t, Args... args) { *t; };
> > +// { dg-message "in requirements with .int t." "" { target *-*-* } .-1 }
> > +
> > +static_assert(c1); // { dg-error "failed" }
> > +
> > +template
> > + concept c2 = requires (T t, Args... args) { *t; };
> > +// { dg-message "in requirements with .int t., .char args#0., .bool
> > args#1." "" { target *-*-* } .-1 }
> > +
> > +static_assert(c2); // { dg-error "failed" }
> >
>
>
[PATCH][arm] Remove +nofp from -mcpu=cortex-m55 options
Hi all, Turns out for consistency with LLVM the +nofp option shouldn't remove ALL of FP and MVE, just the FP part of MVE. This requires more surgery with feature bits so for GCC 10 I'd rather just not support +nofp for -mcpu=cortex-m55 and implement it properly for GCC 11. Committing to trunk. Thanks, Kyrill 2020-04-28 Kyrylo Tkachov * config/arm/arm-cpus.in (cortex-m55): Remove +nofp option. * doc/invoke.texi (Arm Options): Remove -mcpu=cortex-m55 from +nofp option. m55-nofp.patch Description: m55-nofp.patch
Re: [PATCH] middle-end, rs6000: Handle empty [[no_unique_address]] fields like empty bases on powerpc64le-linux [PR94707]
On 4/28/20 6:38 AM, Jakub Jelinek via Gcc-patches wrote:
Hi!
Ok, I've tried:
struct X { };
struct Y { int : 0; };
struct Z { int : 0; Y y; };
struct U : public X { X q; };
struct A { float a, b, c, d; };
struct B : public X { float a, b, c, d; };
struct C : public Y { float a, b, c, d; };
struct D : public Z { float a, b, c, d; };
struct E : public U { float a, b, c, d; };
struct F { [[no_unique_address]] X x; float a, b, c, d; };
struct G { [[no_unique_address]] Y y; float a, b, c, d; };
struct H { [[no_unique_address]] Z z; float a, b, c, d; };
struct I { [[no_unique_address]] U u; float a, b, c, d; };
struct J { float a, b; [[no_unique_address]] X x; float c, d; };
struct K { float a, b; [[no_unique_address]] Y y; float c, d; };
struct L { float a, b; [[no_unique_address]] Z z; float c, d; };
struct M { float a, b; [[no_unique_address]] U u; float c, d; };
#define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s (s);
return 0; }
T (A, a)
T (B, b)
T (C, c)
T (D, d)
T (E, e)
T (F, f)
T (G, g)
T (H, h)
T (I, i)
T (J, j)
T (K, k)
T (L, l)
T (M, m)
testcase on powerpc64-linux. Results:
G++ 9 -std=c++14A, B, C passed in fprs, the rest in gprs
G++ 9 -std=c++17A passed in fprs, the rest in gprs
current trunk -std=c++14 & 17 A, B, C passed in fprs, the rest in gprs
patched trunk -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the
rest in gprs
clang++ [*] -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the rest in gprs
[*] clang version 11.0.0 (g...@github.com:llvm/llvm-project.git
5c352e69e76a26e4eda075e20aa6a9bb7686042c)
Is that what we want? I think it matches the stated intent of P0840R2 or
what Jason/Jonathan said, and doing something different like e.g. not
treating C, G and K as homogenous because of the int : 0 in empty bases
or in zero sized [[no_unique_address] fields would be quite hard to
implement (because for C++14 the FIELD_DECL just isn't there).
Without commenting on the patch itself, I agree that this is what we want.
Thank you for the thorough testing!
Same comment as the other patch about test case comments.
Bill
I've included the above testcase as g++.target/powerpc/ testcases.
2020-04-28 Jakub Jelinek
PR target/94707
* calls.h (no_unique_address_empty_field_p): Declare.
* calls.c (no_unique_address_empty_field_p): New function.
* rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, ignore also no_unique_address_empty_field_p
fields and propagate that fact to caller.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 12:38:35.292851412 +0200
@@ -136,5 +136,6 @@ extern void maybe_warn_nonstring_arg (tr
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
extern bool cxx17_empty_base_field_p (const_tree);
+extern bool no_unique_address_empty_field_p (const_tree);
#endif // GCC_CALLS_H
--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
+++ gcc/calls.c 2020-04-28 12:39:11.936308866 +0200
@@ -6279,5 +6279,24 @@ cxx17_empty_base_field_p (const_tree fie
&& !integer_zerop (TYPE_SIZE (TREE_TYPE (field;
}
+/* Return true if FIELD is a non-static data member with empty
+ type and [[no_unique_address]] attribute that should be
+ ignored for ABI calling convention decisions, in order to make
+ struct S {};
+ struct T : S { float x; };
+ and
+ struct T2 : { [[no_unique_address]] S s; float x; };
+ ABI compatible. */
+
+bool
+no_unique_address_empty_field_p (const_tree field)
+{
+ return (TREE_CODE (field) == FIELD_DECL
+ && RECORD_OR_UNION_TYPE_P (TREE_TYPE (field))
+ && DECL_SIZE (field)
+ && integer_zerop (DECL_SIZE (field))
+ && lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (field)));
+}
+
/* Tell the garbage collector about GTY markers in this source file. */
#include "gt-calls.h"
--- gcc/config/rs6000/rs6000-call.c.jj 2020-04-23 14:42:26.323839084 +0200
+++ gcc/config/rs6000/rs6000-call.c 2020-04-28 12:43:28.277513460 +0200
@@ -5529,7 +5529,7 @@ const struct altivec_builtin_types altiv
static int
rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
- bool *cxx17_empty_base_seen)
+ int *empty_base_seen)
{
machine_mode mode;
HOST_WIDE_INT size;
@@ -5600,7
Re: [PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
On Tue, Apr 28, 2020 at 10:16:24AM -0500, Bill Schmidt via Gcc-patches wrote:
> I think this looks good. My only comment would be to please add some
> comments in the test cases about the purpose, or at least to explain
> the regexes in the scan-assembler-* directives, to save us all some
> mental cycles in the future.
Ok, below in the updated patch:
2020-04-28 Jakub Jelinek
PR target/94707
* tree-core.h (tree_decl_common): Note decl_flag_0 used for
DECL_FIELD_ABI_IGNORED.
* tree.h (DECL_FIELD_ABI_IGNORED): Define.
* calls.h (cxx17_empty_base_field_p): Change into a temporary
macro, check DECL_FIELD_ABI_IGNORED flag with no "no_unique_address"
attribute.
* calls.c (cxx17_empty_base_field_p): Remove.
* tree-streamer-out.c (pack_ts_decl_common_value_fields): Handle
DECL_FIELD_ABI_IGNORED.
* tree-streamer-in.c (unpack_ts_decl_common_value_fields): Likewise.
* lto-streamer-out.c (hash_tree): Likewise.
* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, use DECL_FIELD_ABI_IGNORED instead of
cxx17_empty_base_field_p, if "no_unique_address" attribute is
present, propagate that to the caller too.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* config/rs6000/rs6000.c (rs6000_special_round_type_align,
darwin_rs6000_special_round_type_align): Skip DECL_FIELD_ABI_IGNORED
fields.
* class.c (build_base_field): Set DECL_FIELD_ABI_IGNORED on C++17 empty
base artificial FIELD_DECLs.
(layout_class_type): Set DECL_FIELD_ABI_IGNORED on empty class
field_poverlapping_p FIELD_DECLs.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
* g++.target/powerpc/pr94707-5.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/tree-core.h.jj 2020-04-08 18:15:36.936946772 +0200
+++ gcc/tree-core.h 2020-04-28 15:14:06.598814022 +0200
@@ -1709,7 +1709,8 @@ struct GTY(()) tree_decl_common {
unsigned lang_flag_8 : 1;
/* In VAR_DECL and PARM_DECL, this is DECL_REGISTER
- IN TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P. */
+ In TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
+ In FIELD_DECL, this is DECL_FIELD_ABI_IGNORED. */
unsigned decl_flag_0 : 1;
/* In FIELD_DECL, this is DECL_BIT_FIELD
In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
--- gcc/tree.h.jj 2020-04-08 18:15:36.939946727 +0200
+++ gcc/tree.h 2020-04-28 15:13:07.579695258 +0200
@@ -2750,6 +2750,13 @@ extern void decl_value_expr_insert (tree
/* In a FIELD_DECL, indicates this field should be bit-packed. */
#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->base.u.bits.packed_flag)
+/* In a FIELD_DECL, indicates this field should be ignored for ABI decisions
+ like passing/returning containing struct by value.
+ Set for C++17 empty base artificial FIELD_DECLs as well as
+ empty [[no_unique_address]] non-static data members. */
+#define DECL_FIELD_ABI_IGNORED(NODE) \
+ (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
+
/* Nonzero in a FIELD_DECL means it is a bit field, and must be accessed
specially. */
#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_1)
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 15:26:29.221724466 +0200
@@ -135,6 +135,9 @@ extern tree get_attr_nonstring_decl (tre
extern void maybe_warn_nonstring_arg (tree, tree);
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
-extern bool cxx17_empty_base_field_p (const_tree);
+/* FIXME: Remove after all backends are converted. */
+#define cxx17_empty_base_field_p(t) \
+ (DECL_FIELD_ABI_IGNORED (t) \
+ && !lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)))
#endif // GCC_CALLS_H
--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
+++ gcc/calls.c 2020-04-28 15:26:42.276529517 +0200
@@ -6261,23 +6261,5 @@ must_pass_va_arg_in_stack (tree type)
return targetm.calls.must_pass_in_stack (arg);
}
-/* Return true if FIELD is the C++17 empty base field that should
- be ignored for ABI calling convention decisions in order to
- maintain ABI compatibility between C++14 and earlier, which doesn't
- add this FIELD to classes with empty bases, and C++17 and later
- which does. */
-
-bool
-cxx17_empty_base_field_p (const_tree field)
-{
- return (TREE_CODE (fiel
Re: [PATCH] s390: Fix up -Wpsabi diagnostcs + analysis [PR94704]
On Tue, Apr 28, 2020 at 04:23:24PM +0200, Andreas Krebbel via Gcc-patches wrote:
> Given that this is something which hasn't been covered by the ABI so far I'm
> not sure we really need
> a -Wpsabi warning for that.
Attached are two (updated) versions of the patch on top of the
powerpc+middle-end patch just posted.
The first one emits two separate -Wpsabi warnings like powerpc, one for
the -std=c++14 vs. -std=c++17 ABI difference and one for GCC 9 vs. 10
[[no_unique_address]] passing changes, the other one is silent about the
second case. Will bootstrap/regtest both and you can choose.
Jakub
2020-04-28 Jakub Jelinek
PR target/94704
* config/s390/s390.c (s390_function_arg_vector,
s390_function_arg_float): Use DECL_FIELD_ABI_IGNORED instead of
cxx17_empty_base_field_p. In -Wpsabi diagnostics use the type
passed to the function rather than the type of the single element.
Rename cxx17_empty_base_seen variable to empty_base_seen, change
type to int, and adjust diagnostics depending on if the field
has [[no_unique_attribute]] or not.
* g++.target/s390/s390.exp: New file.
* g++.target/s390/pr94704-1.C: New test.
* g++.target/s390/pr94704-2.C: New test.
* g++.target/s390/pr94704-3.C: New test.
* g++.target/s390/pr94704-4.C: New test.
--- gcc/config/s390/s390.c.jj 2020-04-28 16:15:39.542541382 +0200
+++ gcc/config/s390/s390.c 2020-04-28 17:20:06.087872951 +0200
@@ -11911,7 +11911,8 @@ s390_function_arg_vector (machine_mode m
/* The ABI says that record types with a single member are treated
just like that member would be. */
- bool cxx17_empty_base_seen = false;
+ int empty_base_seen = 0;
+ const_tree orig_type = type;
while (TREE_CODE (type) == RECORD_TYPE)
{
tree field, single = NULL_TREE;
@@ -11921,9 +11922,13 @@ s390_function_arg_vector (machine_mode m
if (TREE_CODE (field) != FIELD_DECL)
continue;
- if (cxx17_empty_base_field_p (field))
+ if (DECL_FIELD_ABI_IGNORED (field))
{
- cxx17_empty_base_seen = true;
+ if (lookup_attribute ("no_unique_address",
+ DECL_ATTRIBUTES (field)))
+ empty_base_seen |= 2;
+ else
+ empty_base_seen |= 1;
continue;
}
@@ -11949,16 +11954,23 @@ s390_function_arg_vector (machine_mode m
if (!VECTOR_TYPE_P (type))
return false;
- if (warn_psabi && cxx17_empty_base_seen)
+ if (warn_psabi && empty_base_seen)
{
static unsigned last_reported_type_uid;
- unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+ unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type));
if (uid != last_reported_type_uid)
{
last_reported_type_uid = uid;
- inform (input_location, "parameter passing for argument of type "
- "%qT when C++17 is enabled changed to match "
- "C++14 in GCC 10.1", type);
+ if (empty_base_seen & 1)
+ inform (input_location,
+ "parameter passing for argument of type %qT when C++17 "
+ "is enabled changed to match C++14 in GCC 10.1",
+ orig_type);
+ else
+ inform (input_location,
+ "parameter passing for argument of type %qT with "
+ "%<[[no_unique_address]]%> members changed in GCC 10.1",
+ orig_type);
}
}
return true;
@@ -11983,7 +11995,8 @@ s390_function_arg_float (machine_mode mo
/* The ABI says that record types with a single member are treated
just like that member would be. */
- bool cxx17_empty_base_seen = false;
+ int empty_base_seen = 0;
+ const_tree orig_type = type;
while (TREE_CODE (type) == RECORD_TYPE)
{
tree field, single = NULL_TREE;
@@ -11992,9 +12005,13 @@ s390_function_arg_float (machine_mode mo
{
if (TREE_CODE (field) != FIELD_DECL)
continue;
- if (cxx17_empty_base_field_p (field))
+ if (DECL_FIELD_ABI_IGNORED (field))
{
- cxx17_empty_base_seen = true;
+ if (lookup_attribute ("no_unique_address",
+ DECL_ATTRIBUTES (field)))
+ empty_base_seen |= 2;
+ else
+ empty_base_seen |= 1;
continue;
}
@@ -12013,16 +12030,23 @@ s390_function_arg_float (machine_mode mo
if (TREE_CODE (type) != REAL_TYPE)
return false;
- if (warn_psabi && cxx17_empty_base_seen)
+ if (warn_psabi && empty_base_seen)
{
static unsigned last_reported_type_uid;
- unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+ unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (orig_type));
if (uid != last_reported_type_uid)
{
last_reported_type
Re: [PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
On April 28, 2020 4:04:58 PM GMT+02:00, Jakub Jelinek via Gcc-patches
wrote:
>Hi!
>
>On Tue, Apr 28, 2020 at 08:53:31AM -0400, Jason Merrill wrote:
>> That sounds good.
>
>So like this? Or better name for the new macro?
I think you miss a hunk for lto/ to compare the flag for tree merging.
>The calls.h macro is there only after all the backends are converted
>to use ABI_IGNORED_FIELD_P.
>
>Not sure if I shouldn't
>if (lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (field))
>ABI_IGNORED_FIELD_P (field) = 1;
>in end_of_class, as there seems to be some ObjC++ partially overlapping
>case too and probably we don't want to change ABI for that.
>
>This patch is a merge of the powerpc64le-linux patch for
>[[no_unique_address]] and the powerpc*-{darwin,aix}* patch with the
>langhook etc. removed for ABI_IGNORED_FIELD_P.
>
>Untested so far.
>
>2020-04-28 Jakub Jelinek
>
> PR target/94707
> * tree-core.h (tree_decl_common): Note decl_flag_0 used for
> ABI_IGNORED_FIELD_P.
> * tree.h (ABI_IGNORED_FIELD_P): Define.
> * calls.h (cxx17_empty_base_field_p): Change into a temporary
> macro, check ABI_IGNORED_FIELD_P flag with no "no_unique_address"
> attribute.
> * calls.c (cxx17_empty_base_field_p): Remove.
> * tree-streamer-out.c (pack_ts_decl_common_value_fields): Handle
> ABI_IGNORED_FIELD_P.
> * tree-streamer-in.c (unpack_ts_decl_common_value_fields): Likewise.
> * lto-streamer-out.c (hash_tree): Likewise.
> * config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Rename
> cxx17_empty_base_seen to empty_base_seen, change type to int *,
> adjust recursive calls, use ABI_IGNORED_FIELD_P instead of
> cxx17_empty_base_field_p, if "no_unique_address" attribute is
> present, propagate that to the caller too.
> (rs6000_discover_homogeneous_aggregate): Adjust
> rs6000_aggregate_candidate caller, emit different diagnostics
> when c++17 empty base fields are present and when empty
> [[no_unique_address]] fields are present.
> * config/rs6000/rs6000.c (rs6000_special_round_type_align,
> darwin_rs6000_special_round_type_align): Skip ABI_IGNORED_FIELD_P
> fields.
>
> * class.c (build_base_field): Set ABI_IGNORED_FIELD_P on C++17 empty
> base artificial FIELD_DECLs.
> (end_of_class): Set ABI_IGNORED_FIELD_P on empty class
> field_poverlapping_p FIELD_DECLs.
>
> * g++.target/powerpc/pr94707-1.C: New test.
> * g++.target/powerpc/pr94707-2.C: New test.
> * g++.target/powerpc/pr94707-3.C: New test.
> * g++.target/powerpc/pr94707-4.C: New test.
> * g++.target/powerpc/pr94707-5.C: New test.
> * g++.target/powerpc/pr94707-4.C: New test.
>
>--- gcc/tree-core.h.jj 2020-04-08 18:15:36.936946772 +0200
>+++ gcc/tree-core.h2020-04-28 15:14:06.598814022 +0200
>@@ -1709,7 +1709,8 @@ struct GTY(()) tree_decl_common {
> unsigned lang_flag_8 : 1;
>
> /* In VAR_DECL and PARM_DECL, this is DECL_REGISTER
>- IN TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
>*/
>+ In TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
>+ In FIELD_DECL, this is ABI_IGNORED_FIELD_P. */
> unsigned decl_flag_0 : 1;
> /* In FIELD_DECL, this is DECL_BIT_FIELD
> In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
>--- gcc/tree.h.jj 2020-04-08 18:15:36.939946727 +0200
>+++ gcc/tree.h 2020-04-28 15:13:07.579695258 +0200
>@@ -2750,6 +2750,13 @@ extern void decl_value_expr_insert (tree
> /* In a FIELD_DECL, indicates this field should be bit-packed. */
>#define DECL_PACKED(NODE) (FIELD_DECL_CHECK
>(NODE)->base.u.bits.packed_flag)
>
>+/* In a FIELD_DECL, indicates this field should be ignored for ABI
>decisions
>+ like passing/returning containing struct by value.
>+ Set for C++17 empty base artificial FIELD_DECLs as well as
>+ empty [[no_unique_address]] non-static data members. */
>+#define ABI_IGNORED_FIELD_P(NODE) \
>+ (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
>+
>/* Nonzero in a FIELD_DECL means it is a bit field, and must be
>accessed
>specially. */
>#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK
>(NODE)->decl_common.decl_flag_1)
>--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
>+++ gcc/calls.h2020-04-28 15:26:29.221724466 +0200
>@@ -135,6 +135,9 @@ extern tree get_attr_nonstring_decl (tre
> extern void maybe_warn_nonstring_arg (tree, tree);
> extern bool get_size_range (tree, tree[2], bool = false);
> extern rtx rtx_for_static_chain (const_tree, bool);
>-extern bool cxx17_empty_base_field_p (const_tree);
>+/* FIXME: Remove after all backends are converted. */
>+#define cxx17_empty_base_field_p(t) \
>+ (ABI_IGNORED_FIELD_P (t)\
>+ && !lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)))
>
> #endif // GCC_CALLS_H
>--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
>+++ gcc/calls.c
[PATCH] aarch64: don't emit bti j after NOTE_INSN_DELETED_LABEL [PR94748]
It was previously discussed that indirect branches cannot go to
NOTE_INSN_DELETED_LABEL so inserting a landing pad is unnecessary.
See https://gcc.gnu.org/pipermail/gcc-patches/2019-May/522625.html
Before the patch a bti j was inserted after the label in
__attribute__((target("branch-protection=bti")))
int foo (void)
{
label:
return 0;
}
This is not necessary and weakens the security protection.
gcc/ChangeLog:
2020-04-28 Szabolcs Nagy
PR target/94748
* config/aarch64/aarch64-bti-insert.c (rest_of_insert_bti): Remove
the check for NOTE_INSN_DELETED_LABEL.
gcc/testsuite/ChangeLog:
2020-04-28 Szabolcs Nagy
PR target/94748
* gcc.target/aarch64/pr94748.c: New test.
---
gcc/config/aarch64/aarch64-bti-insert.c| 8 ++--
gcc/testsuite/gcc.target/aarch64/pr94748.c | 10 ++
2 files changed, 12 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/pr94748.c
diff --git a/gcc/config/aarch64/aarch64-bti-insert.c
b/gcc/config/aarch64/aarch64-bti-insert.c
index aa091c308f6..57663ee23b4 100644
--- a/gcc/config/aarch64/aarch64-bti-insert.c
+++ b/gcc/config/aarch64/aarch64-bti-insert.c
@@ -139,14 +139,10 @@ rest_of_insert_bti (void)
insn = NEXT_INSN (insn))
{
/* If a label is marked to be preserved or can be a non-local goto
-target, it must be protected with a BTI J. The same applies to
-NOTE_INSN_DELETED_LABEL since they are basically labels that might
-be referenced via variables or constant pool. */
- if ((LABEL_P (insn)
+target, it must be protected with a BTI J. */
+ if (LABEL_P (insn)
&& (LABEL_PRESERVE_P (insn)
|| bb->flags & BB_NON_LOCAL_GOTO_TARGET))
- || (NOTE_P (insn)
- && NOTE_KIND (insn) == NOTE_INSN_DELETED_LABEL))
{
bti_insn = gen_bti_j ();
emit_insn_after (bti_insn, insn);
diff --git a/gcc/testsuite/gcc.target/aarch64/pr94748.c
b/gcc/testsuite/gcc.target/aarch64/pr94748.c
new file mode 100644
index 000..2a2850d2fac
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr94748.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+
+__attribute__ ((target("branch-protection=bti")))
+int foo ()
+{
+label:
+ return 0;
+}
+
+/* { dg-final { scan-assembler-not {hint (36|38) // bti (j|jc)} } } */
--
2.17.1
[PATCH] ia64: Adjust the C++14 vs. C++17 ABI thing for [[no_unique_address]] too [PR94706]
Hi!
Untested. If the rs6000+generic change makes it in, is this ok for trunk
too?
2020-04-28 Jakub Jelinek
PR target/94706
* config/ia64/ia64.c (hfa_element_mode): Use DECL_FIELD_ABI_IGNORED
instead of cxx17_empty_base_field_p.
--- gcc/config/ia64/ia64.c.jj 2020-04-22 16:47:46.979994610 +0200
+++ gcc/config/ia64/ia64.c 2020-04-28 17:45:11.456798296 +0200
@@ -4665,7 +4665,7 @@ hfa_element_mode (const_tree type, bool
case QUAL_UNION_TYPE:
for (t = TYPE_FIELDS (type); t; t = DECL_CHAIN (t))
{
- if (TREE_CODE (t) != FIELD_DECL || cxx17_empty_base_field_p (t))
+ if (TREE_CODE (t) != FIELD_DECL || DECL_FIELD_ABI_IGNORED (t))
continue;
mode = hfa_element_mode (TREE_TYPE (t), 1);
Jakub
RE: [PATCH] aarch64: don't emit bti j after NOTE_INSN_DELETED_LABEL [PR94748]
> -Original Message-
> From: Szabolcs Nagy
> Sent: 28 April 2020 16:51
> To: gcc-patches@gcc.gnu.org
> Cc: Richard Earnshaw ; Kyrylo Tkachov
> ; Sudakshina Das
> Subject: [PATCH] aarch64: don't emit bti j after NOTE_INSN_DELETED_LABEL
> [PR94748]
>
> It was previously discussed that indirect branches cannot go to
> NOTE_INSN_DELETED_LABEL so inserting a landing pad is unnecessary.
> See https://gcc.gnu.org/pipermail/gcc-patches/2019-May/522625.html
>
> Before the patch a bti j was inserted after the label in
>
> __attribute__((target("branch-protection=bti")))
> int foo (void)
> {
> label:
> return 0;
> }
>
> This is not necessary and weakens the security protection.
Ok if testing is successful.
Thanks,
Kyrill
>
> gcc/ChangeLog:
>
> 2020-04-28 Szabolcs Nagy
>
> PR target/94748
> * config/aarch64/aarch64-bti-insert.c (rest_of_insert_bti): Remove
> the check for NOTE_INSN_DELETED_LABEL.
>
> gcc/testsuite/ChangeLog:
>
> 2020-04-28 Szabolcs Nagy
>
> PR target/94748
> * gcc.target/aarch64/pr94748.c: New test.
> ---
> gcc/config/aarch64/aarch64-bti-insert.c| 8 ++--
> gcc/testsuite/gcc.target/aarch64/pr94748.c | 10 ++
> 2 files changed, 12 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/pr94748.c
>
> diff --git a/gcc/config/aarch64/aarch64-bti-insert.c
> b/gcc/config/aarch64/aarch64-bti-insert.c
> index aa091c308f6..57663ee23b4 100644
> --- a/gcc/config/aarch64/aarch64-bti-insert.c
> +++ b/gcc/config/aarch64/aarch64-bti-insert.c
> @@ -139,14 +139,10 @@ rest_of_insert_bti (void)
> insn = NEXT_INSN (insn))
> {
> /* If a label is marked to be preserved or can be a non-local goto
> - target, it must be protected with a BTI J. The same applies to
> - NOTE_INSN_DELETED_LABEL since they are basically labels that
> might
> - be referenced via variables or constant pool. */
> - if ((LABEL_P (insn)
> + target, it must be protected with a BTI J. */
> + if (LABEL_P (insn)
> && (LABEL_PRESERVE_P (insn)
> || bb->flags & BB_NON_LOCAL_GOTO_TARGET))
> - || (NOTE_P (insn)
> - && NOTE_KIND (insn) == NOTE_INSN_DELETED_LABEL))
> {
> bti_insn = gen_bti_j ();
> emit_insn_after (bti_insn, insn);
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr94748.c
> b/gcc/testsuite/gcc.target/aarch64/pr94748.c
> new file mode 100644
> index 000..2a2850d2fac
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr94748.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +
> +__attribute__ ((target("branch-protection=bti")))
> +int foo ()
> +{
> +label:
> + return 0;
> +}
> +
> +/* { dg-final { scan-assembler-not {hint (36|38) // bti (j|jc)} } } */
> --
> 2.17.1
Re: [PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
On Tue, Apr 28, 2020 at 05:47:27PM +0200, Richard Biener wrote: > On April 28, 2020 4:04:58 PM GMT+02:00, Jakub Jelinek via Gcc-patches > wrote: > >Hi! > > > >On Tue, Apr 28, 2020 at 08:53:31AM -0400, Jason Merrill wrote: > >> That sounds good. > > > >So like this? Or better name for the new macro? > > I think you miss a hunk for lto/ to compare the flag for tree merging. You're right, * lto-common.c (compare_tree_sccs_1): Handle DECL_FIELD_ABI_IGNORED. --- gcc/lto/lto-common.c.jj 2020-04-17 14:18:44.357438048 +0200 +++ gcc/lto/lto-common.c2020-04-28 17:53:04.809821198 +0200 @@ -1179,6 +1179,7 @@ compare_tree_sccs_1 (tree t1, tree t2, t compare_values (DECL_PACKED); compare_values (DECL_NONADDRESSABLE_P); compare_values (DECL_PADDING_P); + compare_values (DECL_FIELD_ABI_IGNORED); compare_values (DECL_OFFSET_ALIGN); } else if (code == VAR_DECL) added to my copy of the patch. Jakub
Re: [PATCH v2] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
On 4/28/20 10:42 AM, Jakub Jelinek wrote:
On Tue, Apr 28, 2020 at 10:16:24AM -0500, Bill Schmidt via Gcc-patches wrote:
I think this looks good. My only comment would be to please add some
comments in the test cases about the purpose, or at least to explain
the regexes in the scan-assembler-* directives, to save us all some
mental cycles in the future.
Ok, below in the updated patch:
Thanks! Perfect.
Bill
2020-04-28 Jakub Jelinek
PR target/94707
* tree-core.h (tree_decl_common): Note decl_flag_0 used for
DECL_FIELD_ABI_IGNORED.
* tree.h (DECL_FIELD_ABI_IGNORED): Define.
* calls.h (cxx17_empty_base_field_p): Change into a temporary
macro, check DECL_FIELD_ABI_IGNORED flag with no "no_unique_address"
attribute.
* calls.c (cxx17_empty_base_field_p): Remove.
* tree-streamer-out.c (pack_ts_decl_common_value_fields): Handle
DECL_FIELD_ABI_IGNORED.
* tree-streamer-in.c (unpack_ts_decl_common_value_fields): Likewise.
* lto-streamer-out.c (hash_tree): Likewise.
* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, use DECL_FIELD_ABI_IGNORED instead of
cxx17_empty_base_field_p, if "no_unique_address" attribute is
present, propagate that to the caller too.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* config/rs6000/rs6000.c (rs6000_special_round_type_align,
darwin_rs6000_special_round_type_align): Skip DECL_FIELD_ABI_IGNORED
fields.
* class.c (build_base_field): Set DECL_FIELD_ABI_IGNORED on C++17 empty
base artificial FIELD_DECLs.
(layout_class_type): Set DECL_FIELD_ABI_IGNORED on empty class
field_poverlapping_p FIELD_DECLs.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
* g++.target/powerpc/pr94707-5.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/tree-core.h.jj 2020-04-08 18:15:36.936946772 +0200
+++ gcc/tree-core.h 2020-04-28 15:14:06.598814022 +0200
@@ -1709,7 +1709,8 @@ struct GTY(()) tree_decl_common {
unsigned lang_flag_8 : 1;
/* In VAR_DECL and PARM_DECL, this is DECL_REGISTER
- IN TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P. */
+ In TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
+ In FIELD_DECL, this is DECL_FIELD_ABI_IGNORED. */
unsigned decl_flag_0 : 1;
/* In FIELD_DECL, this is DECL_BIT_FIELD
In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
--- gcc/tree.h.jj 2020-04-08 18:15:36.939946727 +0200
+++ gcc/tree.h 2020-04-28 15:13:07.579695258 +0200
@@ -2750,6 +2750,13 @@ extern void decl_value_expr_insert (tree
/* In a FIELD_DECL, indicates this field should be bit-packed. */
#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->base.u.bits.packed_flag)
+/* In a FIELD_DECL, indicates this field should be ignored for ABI decisions
+ like passing/returning containing struct by value.
+ Set for C++17 empty base artificial FIELD_DECLs as well as
+ empty [[no_unique_address]] non-static data members. */
+#define DECL_FIELD_ABI_IGNORED(NODE) \
+ (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
+
/* Nonzero in a FIELD_DECL means it is a bit field, and must be accessed
specially. */
#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK
(NODE)->decl_common.decl_flag_1)
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 15:26:29.221724466 +0200
@@ -135,6 +135,9 @@ extern tree get_attr_nonstring_decl (tre
extern void maybe_warn_nonstring_arg (tree, tree);
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
-extern bool cxx17_empty_base_field_p (const_tree);
+/* FIXME: Remove after all backends are converted. */
+#define cxx17_empty_base_field_p(t) \
+ (DECL_FIELD_ABI_IGNORED (t) \
+ && !lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)))
#endif // GCC_CALLS_H
--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
+++ gcc/calls.c 2020-04-28 15:26:42.276529517 +0200
@@ -6261,23 +6261,5 @@ must_pass_va_arg_in_stack (tree type)
return targetm.calls.must_pass_in_stack (arg);
}
-/* Return true if FIELD is the C++17 empty base field that should
- be ignored for ABI calling convention decisions in order to
- maintain ABI compatibility between C++14 and earlier, which doesn't
- add this FIELD to classes with empty bases, and C++17 and later
- which
Re: [PATCH] rtl cse: Fix PR94740, ICE on testsuite/gcc.dg/sso/t5.c with -mcpu=future -mpcrel -O1
On 4/28/20 2:38 AM, Richard Sandiford wrote: > case RTX_BIN_ARITH: > case RTX_COMM_ARITH: > op0 = simplify_replace_fn_rtx (XEXP (x, 0), old_rtx, fn, data); > op1 = simplify_replace_fn_rtx (XEXP (x, 1), old_rtx, fn, data); > if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1)) > return x; > return simplify_gen_binary (code, mode, op0, op1); Is there a reason you use simplify_replace_fn_rtx here, rather than just using op0 = simplify_rtx (XEXP (x, 0))? Ditto for op1. Does simplify_replace_fn_rtx do something that simplify_rtx doesn't? Peter
Re: [stage1] [PATCH] Merge dg-options and dg-additional-options if len <= 120 chars.
Hi,
On Thu, 16 Apr 2020 at 09:50, Martin Liška wrote:
>
> On 4/14/20 1:43 PM, Jakub Jelinek wrote:
> > Roughly, yes. A few extra in testcases don't hurt necessarily, but say 160
> > chars or more is clearly too much.
>
> All right, I made a limit of 120 characters for the changes.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed in next stage1?
This will introduce a regression on arm:
gcc.target/arm/armv8_2-fp16-move-1.c
Because of:
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -mfloat-abi=hard" } */
/* { dg-add-options arm_v8_2a_fp16_scalar } */
-/* { dg-additional-options "-mfloat-abi=hard" } */
Indeed arm_v8_2a_fp16_scalar inserts -mfloat-abi=softfp in some cases,
so the original version overrides it with -mfloat-abi=hard.
With your patch, the order is reversed, hence the error.
There may be similar problems on other targets?
Thanks,
Christophe
> Thanks,
> Martin
Re: [PATCH] ia64: Adjust the C++14 vs. C++17 ABI thing for [[no_unique_address]] too [PR94706]
On Tue, 2020-04-28 at 17:51 +0200, Jakub Jelinek wrote: > Hi! > > Untested. If the rs6000+generic change makes it in, is this ok for trunk > too? > > 2020-04-28 Jakub Jelinek > > PR target/94706 > * config/ia64/ia64.c (hfa_element_mode): Use DECL_FIELD_ABI_IGNORED > instead of cxx17_empty_base_field_p. OK once the prereqs are approved. jeff >
Re: [PATCH 1/8] testsuite: Fix -mfloat-abi order in arm_v8_2a_bf16_neon_ok and arm_v8_2a_i8mm_ok_nocache
On Mon, 27 Apr 2020 at 22:58, Mike Stump wrote: > > On Apr 27, 2020, at 12:43 PM, Christophe Lyon via Gcc-patches > wrote: > > It seems it's not possible to write these tests so that they works in > > all combinations of toolchain configuration and options used for testing :-( > > So, generally, you can have each change in configuration reflected in a > preprocessor symbol and each command line change also reflected. Then, in > the test case you merely interrogate all the state as necessary to decide. > If that weren't enough, then, we'd have some other mechanism that we could > interrogate with if, and then you'd be off and running. > > [ ducks ] > > Think > > if (__builtin_arm_have_featurea()) > // test case for featurea. > else ... > > I can appreciate if the support necessary for the if or the #ifdef is > missing, but, it could be added. I'm not sure to understand: the dg-require-effective-targets check flag combinations independently of each other, but in this case they are not independent.
Re: [PATCH] libstdc++: Fix subrange::advance and subrange::prev (LWG 3433)
On 27/04/20 17:34 -0400, Patrick Palka via Libstdc++ wrote:
This implements the proposed resolution of LWG 3433, which fixes
subrange::advance when called with a negative argument.
Tested on x86_64-pc-linux-gnu, does this look OK to commit?
libstdc++-v3/ChangeLog:
LWG 3433 subrange::advance(n) has UB when n < 0
* include/std/ranges (subrange::prev): Fix typo.
(subrange::advance): Handle a negative argument as per the proposed
resolution of LWG 3433.
* testsuite/std/ranges/subrange/lwg3433.cc: New test.
---
libstdc++-v3/include/std/ranges | 25 +++--
.../testsuite/std/ranges/subrange/lwg3433.cc | 96 +++
2 files changed, 111 insertions(+), 10 deletions(-)
create mode 100644 libstdc++-v3/testsuite/std/ranges/subrange/lwg3433.cc
diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 8f91598c26e..565366a8d2f 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -353,23 +353,28 @@ namespace ranges
requires bidirectional_iterator<_It>
{
auto __tmp = *this;
- __tmp.advance(--__n);
+ __tmp.advance(-__n);
return __tmp;
}
constexpr subrange&
advance(iter_difference_t<_It> __n)
{
+ // We incorporate the proposed resolution of LWG 3433 here,
+ // avoiding undefined behavior when __n < 0.
Please replace or extend this comment with our convention for marking
issue resolutions:
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// 3343. subrange::advance(n) has UB when n < 0
OK for trunk with that change, thanks.
Re: [PATCH] middle-end, rs6000: Handle empty [[no_unique_address]] fields like empty bases on powerpc64le-linux [PR94707]
Hi!
On Tue, Apr 28, 2020 at 01:38:52PM +0200, Jakub Jelinek wrote:
> Ok, I've tried:
> struct X { };
> struct Y { int : 0; };
> struct Z { int : 0; Y y; };
> struct U : public X { X q; };
> struct A { float a, b, c, d; };
> struct B : public X { float a, b, c, d; };
> struct C : public Y { float a, b, c, d; };
> struct D : public Z { float a, b, c, d; };
> struct E : public U { float a, b, c, d; };
> struct F { [[no_unique_address]] X x; float a, b, c, d; };
> struct G { [[no_unique_address]] Y y; float a, b, c, d; };
> struct H { [[no_unique_address]] Z z; float a, b, c, d; };
> struct I { [[no_unique_address]] U u; float a, b, c, d; };
> struct J { float a, b; [[no_unique_address]] X x; float c, d; };
> struct K { float a, b; [[no_unique_address]] Y y; float c, d; };
> struct L { float a, b; [[no_unique_address]] Z z; float c, d; };
> struct M { float a, b; [[no_unique_address]] U u; float c, d; };
> #define T(S, s) extern S s; extern void foo##s (S); int bar##s () { foo##s
> (s); return 0; }
> T (A, a)
> T (B, b)
> T (C, c)
> T (D, d)
> T (E, e)
> T (F, f)
> T (G, g)
> T (H, h)
> T (I, i)
> T (J, j)
> T (K, k)
> T (L, l)
> T (M, m)
> testcase on powerpc64-linux. Results:
You mean powerpc64le-linux here (I hope!)
> G++ 9 -std=c++14 A, B, C passed in fprs, the rest in gprs
> G++ 9 -std=c++17 A passed in fprs, the rest in gprs
> current trunk -std=c++14 & 17 A, B, C passed in fprs, the rest in gprs
> patched trunk -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the rest in
> gprs
> clang++ [*] -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the rest in
> gprs
> [*] clang version 11.0.0 (g...@github.com:llvm/llvm-project.git
> 5c352e69e76a26e4eda075e20aa6a9bb7686042c)
>
> Is that what we want? I think it matches the stated intent of P0840R2 or
> what Jason/Jonathan said, and doing something different like e.g. not
> treating C, G and K as homogenous because of the int : 0 in empty bases
> or in zero sized [[no_unique_address] fields would be quite hard to
> implement (because for C++14 the FIELD_DECL just isn't there).
I have no idea what "no_unique_address" is (what the name is, mostly?)
It seems to me that we would be *much* better off saying what we want it
to *do*, if we are going to depend on that effect anyway!
Segher
Re: [committed] analyzer: remove -Wanalyzer-use-of-uninitialized-value for GCC 10
On Tue, Apr 28, 2020 at 10:37 AM David Malcolm via Gcc-patches wrote: > I'm working on a rewrite of the region_model code for GCC 11 that I > hope will fix these issues, and allow this warning to be reintroduced. If that's the case, why remove the warning just to add it back? You could leave it in but have it do nothing for GCC 10.
Re: [PATCH] middle-end, rs6000: Handle empty [[no_unique_address]] fields like empty bases on powerpc64le-linux [PR94707]
On Tue, Apr 28, 2020 at 11:32:02AM -0500, Segher Boessenkool wrote: > > testcase on powerpc64-linux. Results: > > You mean powerpc64le-linux here (I hope!) Yes, sorry. > > G++ 9 -std=c++14A, B, C passed in fprs, the rest in gprs > > G++ 9 -std=c++17A passed in fprs, the rest in gprs > > current trunk -std=c++14 & 17 A, B, C passed in fprs, the rest in gprs > > patched trunk -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the > > rest in gprs > > clang++ [*] -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the rest in > > gprs > > [*] clang version 11.0.0 (g...@github.com:llvm/llvm-project.git > > 5c352e69e76a26e4eda075e20aa6a9bb7686042c) > > > > Is that what we want? I think it matches the stated intent of P0840R2 or > > what Jason/Jonathan said, and doing something different like e.g. not > > treating C, G and K as homogenous because of the int : 0 in empty bases > > or in zero sized [[no_unique_address] fields would be quite hard to > > implement (because for C++14 the FIELD_DECL just isn't there). > > I have no idea what "no_unique_address" is (what the name is, mostly?) > It seems to me that we would be *much* better off saying what we want it > to *do*, if we are going to depend on that effect anyway! See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0840r2.html Unlike C, in C++ even fields with empty types are required to have non-zero sizes (at least one byte), because addresses of different fields can't be equal. This attribute says that it can have the same address as other fields. The reason why elfv2 homogenous arg passing cares is mainly the /* There must be no padding. */ if (wi::to_wide (TYPE_SIZE (type)) != count * GET_MODE_BITSIZE (*modep)) return -1; because the type of those fields, which because of the C++ rules must be non-zero size and only contain padding byte(s), appears to contain this padding, but because of the attribute DECL_SIZE is actually bitsize_zero and thus there is no padding. Jakub
Re: [PATCH] rtl cse: Fix PR94740, ICE on testsuite/gcc.dg/sso/t5.c with -mcpu=future -mpcrel -O1
Peter Bergner writes: > On 4/28/20 2:38 AM, Richard Sandiford wrote: >> case RTX_BIN_ARITH: >> case RTX_COMM_ARITH: >> op0 = simplify_replace_fn_rtx (XEXP (x, 0), old_rtx, fn, data); >> op1 = simplify_replace_fn_rtx (XEXP (x, 1), old_rtx, fn, data); >> if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1)) >> return x; >> return simplify_gen_binary (code, mode, op0, op1); > > Is there a reason you use simplify_replace_fn_rtx here, rather than > just using op0 = simplify_rtx (XEXP (x, 0))? Ditto for op1. > Does simplify_replace_fn_rtx do something that simplify_rtx doesn't? I was just quoting code from simplify_replace_fn_rtx as an example of something that handles a similar situation. The recursive calls would be different for cse_process_notes_1. Sorry for the confusion :-) Richard
Re: [PATCH] middle-end, rs6000: Handle empty [[no_unique_address]] fields like empty bases on powerpc64le-linux [PR94707]
On Tue, Apr 28, 2020 at 06:45:05PM +0200, Jakub Jelinek wrote: > On Tue, Apr 28, 2020 at 11:32:02AM -0500, Segher Boessenkool wrote: > > > G++ 9 -std=c++14 A, B, C passed in fprs, the rest in gprs > > > G++ 9 -std=c++17 A passed in fprs, the rest in gprs > > > current trunk -std=c++14 & 17 A, B, C passed in fprs, the rest in gprs > > > patched trunk -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the > > > rest in gprs > > > clang++ [*] -std=c++14 & 17 A, B, C, F, G, J, K passed in fprs, the > > > rest in gprs > > > [*] clang version 11.0.0 (g...@github.com:llvm/llvm-project.git > > > 5c352e69e76a26e4eda075e20aa6a9bb7686042c) > > > > > > Is that what we want? I think it matches the stated intent of P0840R2 or > > > what Jason/Jonathan said, and doing something different like e.g. not > > > treating C, G and K as homogenous because of the int : 0 in empty bases > > > or in zero sized [[no_unique_address] fields would be quite hard to > > > implement (because for C++14 the FIELD_DECL just isn't there). > > > > I have no idea what "no_unique_address" is (what the name is, mostly?) > > See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0840r2.html > Unlike C, in C++ even fields with empty types are required to have non-zero > sizes (at least one byte), because addresses of different fields can't be > equal. This attribute says that it can have the same address as other > fields. The reason why elfv2 homogenous arg passing cares is mainly the > /* There must be no padding. */ > if (wi::to_wide (TYPE_SIZE (type)) > != count * GET_MODE_BITSIZE (*modep)) > return -1; > because the type of those fields, which because of the C++ rules must > be non-zero size and only contain padding byte(s), appears to contain this > padding, but because of the attribute DECL_SIZE is actually bitsize_zero > and thus there is no padding. So the attribute says an object of this struct can have the same address as another object of this struct. But that is not what the backend code uses it for! > > It seems to me that we would be *much* better off saying what we want it > > to *do*, if we are going to depend on that effect anyway! ^^^ I have said it before as well, but it is just getting worse. This kind of frontend code has no business in a backend (all *other* backends might need something similar (or different!), too!) Segher
Re: [PATCH] middle-end, rs6000: Handle empty [[no_unique_address]] fields like empty bases on powerpc64le-linux [PR94707]
On Tue, Apr 28, 2020 at 12:01:15PM -0500, Segher Boessenkool wrote: > So the attribute says an object of this struct can have the same address > as another object of this struct. But that is not what the backend code > uses it for! There is a FAQ at the start of the paper that says various intentions of that and one of them seems to be that it would be interoperable with the empty bases, dunno if e.g. using empty bases in C++17 and then in C++20 code use these instead, I think Jonathan could say more. > > > It seems to me that we would be *much* better off saying what we want it > > > to *do*, if we are going to depend on that effect anyway! > > ^^^ I have said it before as well, but it is just getting worse. This > kind of frontend code has no business in a backend (all *other* backends > might need something similar (or different!), too!) While the C++17 empty base artificial field might look like that case, as it is an artificially added field needed for the FE, though for the -Wpsabi diagnostics we still need the backends now to see it and decide if there is an ABI change or not, for these [[no_unique_address]] fields, they are normal user fields, so I don't see how they could be hidden from the middle-end, they need to go into debug info, one can take their address, etc. By having this single DECL_FIELD_ABI_IGNORED macro, we can mark all the FIELD_DECLs that the backends should ignore, if further cases are ever needed, and hopefully for those we won't need any -Wpsabi warnings because we should get the ABI right immediately. Similarly to how the backends ignore FUNCTION_DECLs, TYPE_DECLs and whatever else the FEs stick into TYPE_FIELDS. Jakub
Re: [committed] libphobos: Add power*-*-linux* as a supported target
Hi! On Sun, Apr 26, 2020 at 11:35:21AM +0200, Iain Buclaw wrote: > This patch adds power*-*-linux* as a supported target for libphobos. Many thanks for doing this! A problem though: libphobos/libdruntime is built for -m32 as well, but that builds libphobos/libdruntime/config/powerpc64/callwithstack.S, which cannot work (that file has all kinds of 64-bit only constructs and instructions in it). > diff --git a/libphobos/configure b/libphobos/configure > index c2b49132fda..c923417532f 100755 > --- a/libphobos/configure > +++ b/libphobos/configure > @@ -13991,9 +13991,10 @@ fi > ;; >mips*) druntime_target_cpu_parsed="mips" > ;; > - powerpc) druntime_target_cpu_parsed="powerpc" > + powerpc|powerpcle) > + druntime_target_cpu_parsed="powerpc" > ;; > - powerpc64) > + powerpc64|powerpc64le) > druntime_target_cpu_parsed="powerpc64" > ;; >i[34567]86|x86_64) We are a biarch target, so both powerpc-* and powerpc64-* configurations can do both those configs (potentially, and the default for many configs). Segher
Re: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
On Tue, 28 Apr 2020, Patrick Palka wrote:
> On Tue, 28 Apr 2020, Jason Merrill wrote:
> > On 4/28/20 9:48 AM, Patrick Palka wrote:
> > > When printing the substituted parameter list of a requires-expression as
> > > part of the "in requirements with ..." context line during concepts
> > > diagnostics, we weren't considering that substitution into a parameter
> > > pack can yield zero or multiple parameters.
> > >
> > > Since this patch affects only concepts diagnostics, so far I tested with
> > > RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer ICE
> > > with the unreduced testcase in the PR. Is this OK to commit after a
> > > full bootstrap and regtest?
> >
> > OK.
>
> Thanks for the review.
>
> >
> > Though I wonder about printing the dependent form and template arguments
> > instead. Do you have an opinion?
>
> I was going to say that on the one hand, it's convenient to have the
> substituted form printed since it would let us to see through
> complicated dependent type aliases, but it seems we don't strip type
> aliases when pretty printing a parameter and its type with '%q#D'
> anyway. And I can't think of any other possible advantage of printing
> the substituted form.
>
> So IMHO printing the dependent form and template arguments instead would
> be better here. I'll try to write a patch for this today.
Like so, tested so far with RUNTESTFLAGS="dg.exp=*concepts*" and also
verified we no longer ICE with the unreduced testcase in the PR. Does
the following look OK to commit after a full bootstrap and regtest?
-- >8 --
Subject: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
When printing the substituted parameter list of a requires-expression as
part of the "in requirements with ..." context line during concepts
diagnostics, we weren't considering that substitution into a parameter
pack can yield zero or multiple parameters.
This patch changes the way we print the parameter list of a
requires-expression in print_requires_expression_info. We now print the
dependent form of the parameter list (along with its template parameter
mapping) instead of printing its substituted form. Besides being an
improvement in its own, this also sidesteps the above parameter pack
expansion issue altogether.
Tested with RUNTESTFLAGS="dg.exp=*concepts*" and also verified we longer
ICE with the unreduced testcase in the PR. Does this look OK to commit
after a bootstrap and regtest?
gcc/cp/ChangeLog:
PR c++/94808
* error.c (print_requires_expression_info): Print the dependent
form of the parameter list with its template parameter mapping,
rather than printing the substituted form.
gcc/testsuite/ChangeLog:
PR c++/94808
* g++.dg/concepts/diagnostic12.C: New test.
* g++.dg/concepts/diagnostic5.C: Adjust expected diagnostic.
---
gcc/cp/error.c | 16
gcc/testsuite/g++.dg/concepts/diagnostic12.C | 16
gcc/testsuite/g++.dg/concepts/diagnostic5.C | 4 ++--
3 files changed, 22 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 98c163db572..46970f9b699 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -3746,7 +3746,6 @@ print_requires_expression_info (diagnostic_context
*context, tree constr, tree a
map = tsubst_parameter_mapping (map, args, tf_none, NULL_TREE);
if (map == error_mark_node)
return;
- args = get_mapped_args (map);
print_location (context, cp_expr_loc_or_input_loc (expr));
pp_verbatim (context->printer, "in requirements ");
@@ -3756,19 +3755,12 @@ print_requires_expression_info (diagnostic_context
*context, tree constr, tree a
pp_verbatim (context->printer, "with ");
while (parms)
{
- tree next = TREE_CHAIN (parms);
-
- TREE_CHAIN (parms) = NULL_TREE;
- cp_unevaluated u;
- tree p = tsubst (parms, args, tf_none, NULL_TREE);
- pp_verbatim (context->printer, "%q#D", p);
- TREE_CHAIN (parms) = next;
-
- if (next)
+ pp_verbatim (context->printer, "%q#D", parms);
+ if (TREE_CHAIN (parms))
pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
-
- parms = next;
+ parms = TREE_CHAIN (parms);
}
+ pp_cxx_parameter_mapping ((cxx_pretty_printer *)context->printer, map);
pp_verbatim (context->printer, "\n");
}
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic12.C
b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
new file mode 100644
index 000..a757342f754
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
@@ -0,0 +1,16 @@
+// PR c++/94808
+// { dg-do compile { target concepts } }
+
+template
+ concept c1 = requires (T t, Args... args) { *t; };
+// { dg-message "in requirements with .T t., .Args ... args. .with.* Args =
\{\}" "" { target *-*-* } .-1 }
+
+static_assert(c1); // { dg-error "failed" }
+
+void f(...);
[committed] analyzer: fix ICE copying struct [PR 94816]
PR analyzer/94816 reports an ICE when attempting to copy a struct
containing a field for which add_region_for_type for fails (on
an OFFSET_TYPE): the region for the src field comes from
make_region_for_unexpected_tree_code which gives it a NULL type, and
then the copy calls add_region_for_type which unconditionally
dereferences the NULL type.
This patch fixes the ICE by checking for NULL types in
add_region_for_type.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to master as r10-8015-g5eae0ac76dcb6aac1d1d6c4edd8852e0035792e4
gcc/analyzer/ChangeLog:
PR analyzer/94816
* engine.cc (impl_region_model_context::on_unexpected_tree_code):
Handle NULL tree.
* region-model.cc (region_model::add_region_for_type): Handle
NULL type.
* region-model.h
(test_region_model_context::on_unexpected_tree_code): Handle NULL
tree.
gcc/testsuite/ChangeLog:
PR analyzer/94816
* g++.dg/analyzer/pr94816.C: New test.
---
gcc/analyzer/engine.cc | 2 +-
gcc/analyzer/region-model.cc| 9 ++---
gcc/analyzer/region-model.h | 2 +-
gcc/testsuite/g++.dg/analyzer/pr94816.C | 13 +
4 files changed, 21 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/analyzer/pr94816.C
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 880e70fb2ba..c73d493a3d8 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -699,7 +699,7 @@ impl_region_model_context::on_unexpected_tree_code (tree t,
logger * const logger = get_logger ();
if (logger)
logger->log ("unhandled tree code: %qs in %qs at %s:%i",
-get_tree_code_name (TREE_CODE (t)),
+t ? get_tree_code_name (TREE_CODE (t)) : "(null)",
loc.get_impl_location ().m_function,
loc.get_impl_location ().m_file,
loc.get_impl_location ().m_line);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 22049a34d29..0794be9a583 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -6448,10 +6448,13 @@ region_id
region_model::add_region_for_type (region_id parent_rid, tree type,
region_model_context *ctxt)
{
- gcc_assert (TYPE_P (type));
+ if (type)
+{
+ gcc_assert (TYPE_P (type));
- if (region *new_region = make_region_for_type (parent_rid, type))
-return add_region (new_region);
+ if (region *new_region = make_region_for_type (parent_rid, type))
+ return add_region (new_region);
+}
/* If we can't handle TYPE, return a placeholder region, and stop
exploring this path. */
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index ad3dd1d13ef..6d427c4c654 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -2205,7 +2205,7 @@ public:
FINAL OVERRIDE
{
internal_error ("unhandled tree code: %qs",
- get_tree_code_name (TREE_CODE (t)));
+ t ? get_tree_code_name (TREE_CODE (t)) : "(null)");
}
private:
diff --git a/gcc/testsuite/g++.dg/analyzer/pr94816.C
b/gcc/testsuite/g++.dg/analyzer/pr94816.C
new file mode 100644
index 000..e241a44e376
--- /dev/null
+++ b/gcc/testsuite/g++.dg/analyzer/pr94816.C
@@ -0,0 +1,13 @@
+/* { dg-additional-options "-O" } */
+
+struct jr;
+
+struct ch {
+ int jr::*rx;
+};
+
+ch
+ad ()
+{
+ return ch ();
+}
--
2.21.0
[PATCH] libphobos: Fix builds for powerpc64 with multilib
Hi,
This patch should fix builds on PPC with multilib enabled.
Multilibs should not have been split up as two logically different CPU,
so at configure time, powerpc64 was being detected, but none of the
32-bit support files were being compiled in.
Segher, is this OK?
Immediately to hand, I only have a powerpc64 cross compiler to test that
both switchcontext.S and callwithstack.S compiler with -m32 and -m64.
Regards
Iain.
---
libphobos/ChangeLog:
* configure: Regenerated.
* libdruntime/Makefile.am (DRUNTIME_SOURCES_CONFIGURED): Add both
switchcontext.S and callwithstack.S if DRUNTIME_CPU_POWERPC.
(DRUNTIME_CPU_POWERPC): Add config/powerpc64/callwithstack.S.
* libdruntime/Makefile.in: Regenerated.
* libdruntime/config/powerpc/switchcontext.S: Add __PPC__ guards.
* libdruntime/config/powerpc64/callwithstack.S: Add __PPC64__ guards.
* m4/druntime/cpu.m4 (DRUNTIME_CPU_SOURCES): Define DRUNTIME_CPU_POWER
for all powerpc biarchs. Remove DRUNTIME_CPU_POWER64 conditional.
---
libphobos/configure | 23 +-
libphobos/libdruntime/Makefile.am | 6 +-
libphobos/libdruntime/Makefile.in | 77 +--
.../config/powerpc/switchcontext.S| 4 +
.../config/powerpc64/callwithstack.S | 4 +
libphobos/m4/druntime/cpu.m4 | 7 +-
6 files changed, 52 insertions(+), 69 deletions(-)
diff --git a/libphobos/configure b/libphobos/configure
index 98d8dc255c1..e461c7442b2 100755
--- a/libphobos/configure
+++ b/libphobos/configure
@@ -692,8 +692,6 @@ DRUNTIME_CPU_SYSTEMZ_FALSE
DRUNTIME_CPU_SYSTEMZ_TRUE
DRUNTIME_CPU_X86_FALSE
DRUNTIME_CPU_X86_TRUE
-DRUNTIME_CPU_POWERPC64_FALSE
-DRUNTIME_CPU_POWERPC64_TRUE
DRUNTIME_CPU_POWERPC_FALSE
DRUNTIME_CPU_POWERPC_TRUE
DRUNTIME_CPU_MIPS_FALSE
@@ -11649,7 +11647,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 11652 "configure"
+#line 11650 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -11755,7 +11753,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 11758 "configure"
+#line 11756 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -13991,12 +13989,9 @@ fi
;;
mips*) druntime_target_cpu_parsed="mips"
;;
- powerpc|powerpcle)
+ powerpc*)
druntime_target_cpu_parsed="powerpc"
;;
- powerpc64|powerpc64le)
- druntime_target_cpu_parsed="powerpc64"
- ;;
i[34567]86|x86_64)
druntime_target_cpu_parsed="x86"
;;
@@ -14039,14 +14034,6 @@ else
DRUNTIME_CPU_POWERPC_FALSE=
fi
- if test "$druntime_target_cpu_parsed" = "powerpc64"; then
- DRUNTIME_CPU_POWERPC64_TRUE=
- DRUNTIME_CPU_POWERPC64_FALSE='#'
-else
- DRUNTIME_CPU_POWERPC64_TRUE='#'
- DRUNTIME_CPU_POWERPC64_FALSE=
-fi
-
if test "$druntime_target_cpu_parsed" = "x86"; then
DRUNTIME_CPU_X86_TRUE=
DRUNTIME_CPU_X86_FALSE='#'
@@ -15605,10 +15592,6 @@ if test -z "${DRUNTIME_CPU_POWERPC_TRUE}" && test -z
"${DRUNTIME_CPU_POWERPC_FAL
as_fn_error $? "conditional \"DRUNTIME_CPU_POWERPC\" was never defined.
Usually this means the macro was only invoked conditionally." "$LINENO" 5
fi
-if test -z "${DRUNTIME_CPU_POWERPC64_TRUE}" && test -z
"${DRUNTIME_CPU_POWERPC64_FALSE}"; then
- as_fn_error $? "conditional \"DRUNTIME_CPU_POWERPC64\" was never defined.
-Usually this means the macro was only invoked conditionally." "$LINENO" 5
-fi
if test -z "${DRUNTIME_CPU_X86_TRUE}" && test -z "${DRUNTIME_CPU_X86_FALSE}";
then
as_fn_error $? "conditional \"DRUNTIME_CPU_X86\" was never defined.
Usually this means the macro was only invoked conditionally." "$LINENO" 5
diff --git a/libphobos/libdruntime/Makefile.am
b/libphobos/libdruntime/Makefile.am
index e1f47d36f90..1b1c5689e7e 100644
--- a/libphobos/libdruntime/Makefile.am
+++ b/libphobos/libdruntime/Makefile.am
@@ -81,10 +81,8 @@ if DRUNTIME_CPU_MIPS
DRUNTIME_SOURCES_CONFIGURED += config/mips/switchcontext.S
endif
if DRUNTIME_CPU_POWERPC
-DRUNTIME_SOURCES_CONFIGURED += config/powerpc/switchcontext.S
-endif
-if DRUNTIME_CPU_POWERPC64
-DRUNTIME_SOURCES_CONFIGURED += config/powerpc64/callwithstack.S
+DRUNTIME_SOURCES_CONFIGURED += config/powerpc/switchcontext.S \
+ config/powerpc64/callwithstack.S
endif
if DRUNTIME_CPU_X86
if DRUNTIME_OS_MINGW
diff --git a/libphobos/libdruntime/Makefile.in
b/libphobos/libdruntime/Makefile.in
index 53402842cb4..04e58bb9034 100644
--- a/libphobos/libdruntime/Makefile.in
+++ b/libphobos/libdruntime/Makefile.in
@@ -123,12 +123,13 @@ target_triplet = @target@
@DRUNTIME_CPU_AARCH64_TRUE@am__append_11 = config/aarch64/switchcontext.S
@DRUNTIME_CPU_ARM_TRUE@am__append_12 = config/arm/switchcontext.
Re: [committed] d: Merge upstream dmd 09db0c41e, druntime e68a5ae3.
On Sun, Apr 26, 2020 at 10:43:53PM +0200, Iain Buclaw wrote:
> +// The layout of the type is:
> +//
> +// [1| 7 | 56 ][ 8| 56 ]
> +// [S| Exp | Fraction (hi) ][ Unused | Fraction (low) ]
> +//
> +// We can get the least significant bits by subtracting the
> IEEE
> +// double precision portion from the real value.
> >>>
> >>> That's not correct. There is no "Unused" field, and the lower fraction
> >>> is not always an immediate extension of the higher fraction.
> >>>
> >>> (It's not 1,7,56 -- it is 1,11,52).
> > All bits are significant to the value, there are no unused bits, for
> > most values. The sign and exponent of the second number are very much
> > relevant, in general.
> >
> > I didn't look at your actual implementation, just this comment, but it
> > sounds like your tests miss a lot of cases, if no problems were found?
>
> All these tests pass. That is, the computed compile-time byte layout
> (the result of this function) is the same as the layout at run-time.
Then either it tests not nearly enough, or it does not implement what the
comment says.
> // check subnormal storage edge case for Quadruple
> testNumberConvert!("real.min_normal/2UL^^56");
> testNumberConvert!("real.min_normal/19");
> testNumberConvert!("real.min_normal/17");
IBM long double has quite different edge cases than IEEE QP float.
> /**True random values*/
> testNumberConvert!("-0x9.0f7ee55df77618fp-13829L");
> testNumberConvert!("0x7.36e6e2640120d28p+8797L");
> testNumberConvert!("-0x1.05df6ce4702ccf8p+15835L");
> testNumberConvert!("0x9.54bb0d88806f714p-7088L");
None of these are valid double-double numbers (they all underflow or
overflow).
> /**Big overflow or underflow*/
> testNumberConvert!("cast(double)-0x9.0f7ee55df77618fp-13829L");
> testNumberConvert!("cast(double)0x7.36e6e2640120d28p+8797L");
> testNumberConvert!("cast(double)-0x1.05df6ce4702ccf8p+15835L");
> testNumberConvert!("cast(double)0x9.54bb0d88806f714p-7088L");
> testNumberConvert!("cast(float)-0x9.0f7ee55df77618fp-13829L");
> testNumberConvert!("cast(float)0x7.36e6e2640120d28p+8797L");
(Exactly like these).
Segher
Re: [PATCH] libphobos: Fix builds for powerpc64 with multilib
Hi! On Tue, Apr 28, 2020 at 07:58:37PM +0200, Iain Buclaw wrote: > This patch should fix builds on PPC with multilib enabled. > > Multilibs should not have been split up as two logically different CPU, > so at configure time, powerpc64 was being detected, but none of the > 32-bit support files were being compiled in. > > Segher, is this OK? If it fixes the bootstrap breakage, that is good :-) > * configure: Regenerated. "Regenerate." (Not passive). > diff --git a/libphobos/libdruntime/config/powerpc/switchcontext.S > b/libphobos/libdruntime/config/powerpc/switchcontext.S > index 5470f9c4ca3..82ee542064b 100644 > --- a/libphobos/libdruntime/config/powerpc/switchcontext.S > +++ b/libphobos/libdruntime/config/powerpc/switchcontext.S > @@ -24,6 +24,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively. > If not, see > > #include "../common/threadasm.S" > > +#if defined( __ppc__ ) || defined( __PPC__ ) || defined( __powerpc__ ) What is this for? Everything in libphobos/libdruntime/config/powerpc/ is for PowerPC anyway? Or is this meant to select "not 64 bit"? That is not what these macros mean. (What target defines "__ppc__" btw? It's not standard). Looks good, okay for trunk with those nits fixed somehow. Thanks! Segher
Re: [PATCH] c-attribs.c: Fix use of uninitialized variable nunits
On Tue, Apr 28, 2020 at 11:33:31AM +0200, Richard Biener wrote: > On Tue, Apr 28, 2020 at 10:03 AM Stefan Schulze Frielinghaus via > Gcc-patches wrote: > > > > In function handle_vector_size_attribute local variable nunits is > > supposed to be initialized by function type_valid_for_vector_size. > > However, in case ARGS is null the function may return with a non-null > > value and leave nunits uninitialized. This results in warning/error: > > > > gcc/poly-int.h: In function 'tree_node* > > handle_vector_size_attribute(tree_node**, tree, tree, int, bool*)': > > gcc/poly-int.h:330:3: error: 'nunits' may be used uninitialized in this > > function [-Werror=maybe-uninitialized] > > 330 | ((void) (&(RES).coeffs[0] == (C *) 0), \ > > | ^ > > gcc/c-family/c-attribs.c:3695:26: note: 'nunits' was declared here > > 3695 | unsigned HOST_WIDE_INT nunits; > > | > > > > This is fixed by also checking whether ARGS is null or not. > > > > Bootstrapped and regtested on S/390. Ok for master? > > I think it's better to assert that it is not null for example by adding a > nonnull attribute? Can you check if that works? If it doesn't the > patch is OK. Yes, that works, too. Please find an updated version attached. If you think it is useful I could also add a gcc_assert (!args) for minimal testing. In function handle_vector_size_attribute local variable nunits is supposed to be initialized by function type_valid_for_vector_size. However, in case ARGS is null the function may return with a non-null value and leave nunits uninitialized. This results in warning/error: gcc/poly-int.h: In function 'tree_node* handle_vector_size_attribute(tree_node**, tree, tree, int, bool*)': gcc/poly-int.h:330:3: error: 'nunits' may be used uninitialized in this function [-Werror=maybe-uninitialized] 330 | ((void) (&(RES).coeffs[0] == (C *) 0), \ | ^ gcc/c-family/c-attribs.c:3695:26: note: 'nunits' was declared here 3695 | unsigned HOST_WIDE_INT nunits; | Added attribute nonnull for argument args in order to state the invariant and to silence warning. gcc/c-family/ChangeLog: 2020-04-28 Stefan Schulze Frielinghaus * c-attribs.c (handle_vector_size_attribute): Add attribute nonnull for argument args in order to state invariant and to silence warning of uninitialized variable usage. --- gcc/c-family/c-attribs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index ac936d5..e49a74c4048 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -117,7 +117,7 @@ static tree handle_tm_attribute (tree *, tree, tree, int, bool *); static tree handle_tm_wrap_attribute (tree *, tree, tree, int, bool *); static tree handle_novops_attribute (tree *, tree, tree, int, bool *); static tree handle_vector_size_attribute (tree *, tree, tree, int, - bool *); + bool *) ATTRIBUTE_NONNULL(3); static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *); static tree handle_nonstring_attribute (tree *, tree, tree, int, bool *); static tree handle_nothrow_attribute (tree *, tree, tree, int, bool *); -- 2.25.3
[PATCH] wwwdocs: Added mentioning of TX3 chip to the list of the processors
This adds mentioning of Marvell ThunderX3 chip to the list of supported processors. --- htdocs/gcc-10/changes.html | 1 + 1 file changed, 1 insertion(+) diff --git a/htdocs/gcc-10/changes.html b/htdocs/gcc-10/changes.html index 41c2dc0..b37b74d 100644 --- a/htdocs/gcc-10/changes.html +++ b/htdocs/gcc-10/changes.html @@ -655,6 +655,7 @@ typedef svbool_t pred512 __attribute__((arm_sve_vector_bits(512))); Arm Cortex-A65 (cortex-a65). Arm Cortex-A65AE (cortex-a65ae). Arm Cortex-A34 (cortex-a34). +Marvell ThunderX3 (thunderx3t110). The GCC identifiers can be used as arguments to the -mcpu or -mtune options, -- 2.7.4
[pushed] c++: Redeclaration of implicit operator== [PR94583]
My last patch rejected a namespace-scope declaration of the
implicitly-declared friend operator== before the class, but redeclaring it
after the class should be OK.
Tested x86_64-pc-linux-gnu, applying to trunk.
gcc/cp/ChangeLog
2020-04-28 Jason Merrill
PR c++/94583
* decl.c (use_eh_spec_block): Check nothrow type after
DECL_DEFAULTED_FN.
* pt.c (maybe_instantiate_noexcept): Call synthesize_method for
DECL_MAYBE_DELETED fns here.
* decl2.c (mark_used): Not here.
* method.c (get_defaulted_eh_spec): Reject DECL_MAYBE_DELETED here.
---
gcc/cp/cp-tree.h | 2 +-
gcc/cp/decl.c | 4 ++--
gcc/cp/decl2.c| 11 ---
gcc/cp/method.c | 11 ---
gcc/cp/pt.c | 12
gcc/testsuite/g++.dg/cpp2a/spaceship-synth7.C | 9 +
6 files changed, 28 insertions(+), 21 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/spaceship-synth7.C
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index fff0016b3aa..e115a8a1d25 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -3169,7 +3169,7 @@ struct GTY(()) lang_decl {
(LANG_DECL_FN_CHECK (NODE)->has_dependent_explicit_spec_p)
/* Nonzero for a defaulted FUNCTION_DECL for which we haven't decided yet if
- it's deleted. */
+ it's deleted; we will decide in synthesize_method. */
#define DECL_MAYBE_DELETED(NODE) \
(LANG_DECL_FN_CHECK (NODE)->maybe_deleted)
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index cf855dae909..4c0ae1cfa2e 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -16503,7 +16503,6 @@ use_eh_spec_block (tree fn)
{
return (flag_exceptions && flag_enforce_eh_specs
&& !processing_template_decl
- && !type_throw_all_p (TREE_TYPE (fn))
/* We insert the EH_SPEC_BLOCK only in the original
function; then, it is copied automatically to the
clones. */
@@ -16516,7 +16515,8 @@ use_eh_spec_block (tree fn)
not creating the EH_SPEC_BLOCK we save a little memory,
and we avoid spurious warnings about unreachable
code. */
- && !DECL_DEFAULTED_FN (fn));
+ && !DECL_DEFAULTED_FN (fn)
+ && !type_throw_all_p (TREE_TYPE (fn)));
}
/* Helper function to push ARGS into the current lexical scope. DECL
diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index ac65529a01d..8d3ac31a0c9 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -5512,17 +5512,6 @@ mark_used (tree decl, tsubst_flags_t complain)
if (TREE_CODE (decl) == CONST_DECL)
used_types_insert (DECL_CONTEXT (decl));
- if (TREE_CODE (decl) == FUNCTION_DECL
- && DECL_MAYBE_DELETED (decl))
-{
- /* ??? Switch other defaulted functions to use DECL_MAYBE_DELETED? */
- gcc_assert (special_function_p (decl) == sfk_comparison);
-
- ++function_depth;
- synthesize_method (decl);
- --function_depth;
-}
-
if (TREE_CODE (decl) == FUNCTION_DECL
&& !maybe_instantiate_noexcept (decl, complain))
return false;
diff --git a/gcc/cp/method.c b/gcc/cp/method.c
index 2fb0de288a2..fb2dd47013f 100644
--- a/gcc/cp/method.c
+++ b/gcc/cp/method.c
@@ -2411,16 +2411,13 @@ synthesized_method_walk (tree ctype,
special_function_kind sfk, bool const_p,
tree
get_defaulted_eh_spec (tree decl, tsubst_flags_t complain)
{
+ /* For DECL_MAYBE_DELETED this should already have been handled by
+ synthesize_method. */
+ gcc_assert (!DECL_MAYBE_DELETED (decl));
+
if (DECL_CLONED_FUNCTION_P (decl))
decl = DECL_CLONED_FUNCTION (decl);
special_function_kind sfk = special_function_p (decl);
- if (sfk == sfk_comparison)
-{
- /* We're in synthesize_method. Start with NULL_TREE, build_comparison_op
-will adjust as needed. */
- gcc_assert (decl == current_function_decl);
- return NULL_TREE;
-}
tree ctype = DECL_CONTEXT (decl);
tree parms = FUNCTION_FIRST_USER_PARMTYPE (decl);
tree parm_type = TREE_VALUE (parms);
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 66308a2d295..ba22d9ec538 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -25180,6 +25180,18 @@ maybe_instantiate_noexcept (tree fn, tsubst_flags_t
complain)
&& (!flag_noexcept_type || type_dependent_expression_p (fn)))
return true;
+ if (DECL_MAYBE_DELETED (fn))
+{
+ if (fn == current_function_decl)
+ /* We're in start_preparsed_function, keep going. */
+ return true;
+
+ ++function_depth;
+ synthesize_method (fn);
+ --function_depth;
+ return !DECL_MAYBE_DELETED (fn);
+}
+
if (DECL_CLONED_FUNCTION_P (fn))
fn = DECL_CLONED_FUNCTION (fn);
diff --git a/gcc/testsuite/g++.dg/cpp2a/spaceship-synth7.C
b/gcc/testsuite/g++.dg/cpp2a/spaceship-synth7.C
new file mode 100644
index 000..86b661ca6e0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/spaceship-synth
Re: [pushed] c++: implicit operator== with previous decl [PR94583]
On 4/25/20 6:54 PM, Marek Polacek wrote:
On Sat, Apr 25, 2020 at 12:17:18AM -0400, Jason Merrill via Gcc-patches wrote:
P2085 clarified that a defaulted comparison operator must be the first
declaration of the function. Rejecting that avoids the ICE trying to
compare the noexcept-specifications.
Tested x86_64-pc-linux-gnu, applying to trunk.
gcc/cp/ChangeLog
2020-04-24 Jason Merrill
PR c++/94583
* decl.c (redeclaration_error_message): Reject defaulted comparison
operator that has been previously declared.
---
gcc/cp/decl.c | 8
gcc/testsuite/g++.dg/cpp2a/spaceship-synth6.C | 11 +++
2 files changed, 19 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/spaceship-synth6.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index c8c2f080763..31b5884ca3a 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -2972,6 +2972,14 @@ redeclaration_error_message (tree newdecl, tree olddecl)
}
}
+ /* [class.compare.default]: A definition of a comparison operator as
+defaulted that appears in a class shall be the first declaration of
+that function. */
+ special_function_kind sfk = special_function_p (olddecl);
+ if (sfk == sfk_comparison && DECL_DEFAULTED_FN (newdecl))
+ return G_("comparison operator %q+D defaulted after "
+ "its first declaration");
+
This one still ICEs:
namespace std { struct strong_ordering { }; }
struct Q {
friend std::strong_ordering operator<=>(const Q&, const Q&) = default;
};
bool operator==(const Q&, const Q&) noexcept;
Fixed, thanks.
Jason
Re: [PATCH] libphobos: Fix builds for powerpc64 with multilib
On 28/04/2020 20:25, Segher Boessenkool wrote: > Hi! > > On Tue, Apr 28, 2020 at 07:58:37PM +0200, Iain Buclaw wrote: >> This patch should fix builds on PPC with multilib enabled. >> >> Multilibs should not have been split up as two logically different CPU, >> so at configure time, powerpc64 was being detected, but none of the >> 32-bit support files were being compiled in. >> >> Segher, is this OK? > > If it fixes the bootstrap breakage, that is good :-) > >> * configure: Regenerated. > > "Regenerate." (Not passive). > >> diff --git a/libphobos/libdruntime/config/powerpc/switchcontext.S >> b/libphobos/libdruntime/config/powerpc/switchcontext.S >> index 5470f9c4ca3..82ee542064b 100644 >> --- a/libphobos/libdruntime/config/powerpc/switchcontext.S >> +++ b/libphobos/libdruntime/config/powerpc/switchcontext.S >> @@ -24,6 +24,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively. >> If not, see >> >> #include "../common/threadasm.S" >> >> +#if defined( __ppc__ ) || defined( __PPC__ ) || defined( __powerpc__ ) > > What is this for? Everything in libphobos/libdruntime/config/powerpc/ > is for PowerPC anyway? Or is this meant to select "not 64 bit"? That > is not what these macros mean. > I had pulled it from the previous file when everything used to be squashed into one mega source file. On a closer look, the order matters here: #if defined(__PPC64__) #elif defined( __ppc__ ) || defined( __PPC__ ) || defined( __powerpc__ ) #endif I couldn't say where __ppc__ came from I'm afraid, but I'll remove it. Having now looked at how S390/S390x handled multilib, I'll do this in a slightly different way then. Iain.
Re: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
On 4/28/20 1:41 PM, Patrick Palka wrote:
On Tue, 28 Apr 2020, Patrick Palka wrote:
On Tue, 28 Apr 2020, Jason Merrill wrote:
On 4/28/20 9:48 AM, Patrick Palka wrote:
When printing the substituted parameter list of a requires-expression as
part of the "in requirements with ..." context line during concepts
diagnostics, we weren't considering that substitution into a parameter
pack can yield zero or multiple parameters.
Since this patch affects only concepts diagnostics, so far I tested with
RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer ICE
with the unreduced testcase in the PR. Is this OK to commit after a
full bootstrap and regtest?
OK.
Thanks for the review.
Though I wonder about printing the dependent form and template arguments
instead. Do you have an opinion?
I was going to say that on the one hand, it's convenient to have the
substituted form printed since it would let us to see through
complicated dependent type aliases, but it seems we don't strip type
aliases when pretty printing a parameter and its type with '%q#D'
anyway. And I can't think of any other possible advantage of printing
the substituted form.
So IMHO printing the dependent form and template arguments instead would
be better here. I'll try to write a patch for this today.
Like so, tested so far with RUNTESTFLAGS="dg.exp=*concepts*" and also
verified we no longer ICE with the unreduced testcase in the PR. Does
the following look OK to commit after a full bootstrap and regtest?
-- >8 --
Subject: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
When printing the substituted parameter list of a requires-expression as
part of the "in requirements with ..." context line during concepts
diagnostics, we weren't considering that substitution into a parameter
pack can yield zero or multiple parameters.
This patch changes the way we print the parameter list of a
requires-expression in print_requires_expression_info. We now print the
dependent form of the parameter list (along with its template parameter
mapping) instead of printing its substituted form. Besides being an
improvement in its own, this also sidesteps the above parameter pack
expansion issue altogether.
Tested with RUNTESTFLAGS="dg.exp=*concepts*" and also verified we longer
ICE with the unreduced testcase in the PR. Does this look OK to commit
after a bootstrap and regtest?
gcc/cp/ChangeLog:
PR c++/94808
* error.c (print_requires_expression_info): Print the dependent
form of the parameter list with its template parameter mapping,
rather than printing the substituted form.
gcc/testsuite/ChangeLog:
PR c++/94808
* g++.dg/concepts/diagnostic12.C: New test.
* g++.dg/concepts/diagnostic5.C: Adjust expected diagnostic.
---
gcc/cp/error.c | 16
gcc/testsuite/g++.dg/concepts/diagnostic12.C | 16
gcc/testsuite/g++.dg/concepts/diagnostic5.C | 4 ++--
3 files changed, 22 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 98c163db572..46970f9b699 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -3746,7 +3746,6 @@ print_requires_expression_info (diagnostic_context
*context, tree constr, tree a
map = tsubst_parameter_mapping (map, args, tf_none, NULL_TREE);
if (map == error_mark_node)
return;
- args = get_mapped_args (map);
print_location (context, cp_expr_loc_or_input_loc (expr));
pp_verbatim (context->printer, "in requirements ");
@@ -3756,19 +3755,12 @@ print_requires_expression_info (diagnostic_context
*context, tree constr, tree a
pp_verbatim (context->printer, "with ");
while (parms)
{
- tree next = TREE_CHAIN (parms);
-
- TREE_CHAIN (parms) = NULL_TREE;
- cp_unevaluated u;
- tree p = tsubst (parms, args, tf_none, NULL_TREE);
- pp_verbatim (context->printer, "%q#D", p);
- TREE_CHAIN (parms) = next;
-
- if (next)
+ pp_verbatim (context->printer, "%q#D", parms);
+ if (TREE_CHAIN (parms))
pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
-
- parms = next;
+ parms = TREE_CHAIN (parms);
}
+ pp_cxx_parameter_mapping ((cxx_pretty_printer *)context->printer, map);
pp_verbatim (context->printer, "\n");
}
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic12.C
b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
new file mode 100644
index 000..a757342f754
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
@@ -0,0 +1,16 @@
+// PR c++/94808
+// { dg-do compile { target concepts } }
+
+template
+ concept c1 = requires (T t, Args... args) { *t; };
+// { dg-message "in requirements with .T t., .Args ... args. .with.* Args = \{\}"
"" { target *-*-* } .-1 }
Doesn't this print a binding for T?
+
+static_assert(c1); // { dg-error "failed" }
+
+void f
Re: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
On Tue, 28 Apr 2020, Jason Merrill wrote:
> On 4/28/20 1:41 PM, Patrick Palka wrote:
> > On Tue, 28 Apr 2020, Patrick Palka wrote:
> >
> > > On Tue, 28 Apr 2020, Jason Merrill wrote:
> > > > On 4/28/20 9:48 AM, Patrick Palka wrote:
> > > > > When printing the substituted parameter list of a requires-expression
> > > > > as
> > > > > part of the "in requirements with ..." context line during concepts
> > > > > diagnostics, we weren't considering that substitution into a parameter
> > > > > pack can yield zero or multiple parameters.
> > > > >
> > > > > Since this patch affects only concepts diagnostics, so far I tested
> > > > > with
> > > > > RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer
> > > > > ICE
> > > > > with the unreduced testcase in the PR. Is this OK to commit after a
> > > > > full bootstrap and regtest?
> > > >
> > > > OK.
> > >
> > > Thanks for the review.
> > >
> > > >
> > > > Though I wonder about printing the dependent form and template arguments
> > > > instead. Do you have an opinion?
> > >
> > > I was going to say that on the one hand, it's convenient to have the
> > > substituted form printed since it would let us to see through
> > > complicated dependent type aliases, but it seems we don't strip type
> > > aliases when pretty printing a parameter and its type with '%q#D'
> > > anyway. And I can't think of any other possible advantage of printing
> > > the substituted form.
> > >
> > > So IMHO printing the dependent form and template arguments instead would
> > > be better here. I'll try to write a patch for this today.
> >
> > Like so, tested so far with RUNTESTFLAGS="dg.exp=*concepts*" and also
> > verified we no longer ICE with the unreduced testcase in the PR. Does
> > the following look OK to commit after a full bootstrap and regtest?
> >
> > -- >8 --
> >
> > Subject: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
> >
> > When printing the substituted parameter list of a requires-expression as
> > part of the "in requirements with ..." context line during concepts
> > diagnostics, we weren't considering that substitution into a parameter
> > pack can yield zero or multiple parameters.
> >
> > This patch changes the way we print the parameter list of a
> > requires-expression in print_requires_expression_info. We now print the
> > dependent form of the parameter list (along with its template parameter
> > mapping) instead of printing its substituted form. Besides being an
> > improvement in its own, this also sidesteps the above parameter pack
> > expansion issue altogether.
> >
> > Tested with RUNTESTFLAGS="dg.exp=*concepts*" and also verified we longer
> > ICE with the unreduced testcase in the PR. Does this look OK to commit
> > after a bootstrap and regtest?
> >
> > gcc/cp/ChangeLog:
> >
> > PR c++/94808
> > * error.c (print_requires_expression_info): Print the dependent
> > form of the parameter list with its template parameter mapping,
> > rather than printing the substituted form.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/94808
> > * g++.dg/concepts/diagnostic12.C: New test.
> > * g++.dg/concepts/diagnostic5.C: Adjust expected diagnostic.
> > ---
> > gcc/cp/error.c | 16
> > gcc/testsuite/g++.dg/concepts/diagnostic12.C | 16
> > gcc/testsuite/g++.dg/concepts/diagnostic5.C | 4 ++--
> > 3 files changed, 22 insertions(+), 14 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
> >
> > diff --git a/gcc/cp/error.c b/gcc/cp/error.c
> > index 98c163db572..46970f9b699 100644
> > --- a/gcc/cp/error.c
> > +++ b/gcc/cp/error.c
> > @@ -3746,7 +3746,6 @@ print_requires_expression_info (diagnostic_context
> > *context, tree constr, tree a
> > map = tsubst_parameter_mapping (map, args, tf_none, NULL_TREE);
> > if (map == error_mark_node)
> > return;
> > - args = get_mapped_args (map);
> > print_location (context, cp_expr_loc_or_input_loc (expr));
> > pp_verbatim (context->printer, "in requirements ");
> > @@ -3756,19 +3755,12 @@ print_requires_expression_info (diagnostic_context
> > *context, tree constr, tree a
> > pp_verbatim (context->printer, "with ");
> > while (parms)
> > {
> > - tree next = TREE_CHAIN (parms);
> > -
> > - TREE_CHAIN (parms) = NULL_TREE;
> > - cp_unevaluated u;
> > - tree p = tsubst (parms, args, tf_none, NULL_TREE);
> > - pp_verbatim (context->printer, "%q#D", p);
> > - TREE_CHAIN (parms) = next;
> > -
> > - if (next)
> > + pp_verbatim (context->printer, "%q#D", parms);
> > + if (TREE_CHAIN (parms))
> > pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
> > -
> > - parms = next;
> > + parms = TREE_CHAIN (parms);
> > }
> > + pp_cxx_parameter_mapping ((cxx_pretty_printer *)context->printer, map);
> > pp_verbatim (context->printer, "\n")
Re: [PATCH] libphobos: Fix builds for powerpc64 with multilib
On 28/04/2020 20:43, Iain Buclaw wrote:
> On 28/04/2020 20:25, Segher Boessenkool wrote:
>> Hi!
>>
>> On Tue, Apr 28, 2020 at 07:58:37PM +0200, Iain Buclaw wrote:
>>>
>>> +#if defined( __ppc__ ) || defined( __PPC__ ) || defined( __powerpc__ )
>>
>> What is this for? Everything in libphobos/libdruntime/config/powerpc/
>> is for PowerPC anyway? Or is this meant to select "not 64 bit"? That
>> is not what these macros mean.
>>
>
> I had pulled it from the previous file when everything used to be
> squashed into one mega source file. On a closer look, the order matters
> here:
>
> #if defined(__PPC64__)
>
> #elif defined( __ppc__ ) || defined( __PPC__ ) || defined( __powerpc__ )
>
> #endif
>
> I couldn't say where __ppc__ came from I'm afraid, but I'll remove it.
>
> Having now looked at how S390/S390x handled multilib, I'll do this in a
> slightly different way then.
>
Actually, no, what I'm doing is fine. I've removed the config/powerpc64
directory, putting both 32 and 64 bit support in the same location.
Updated the guards to be defined(__PPC64__) and !defined(__PPC64__)
respectively. And nits addressed.
Iain
---
libphobos/ChangeLog:
PR d/94825
* configure: Regenerate.
* libdruntime/Makefile.am (DRUNTIME_SOURCES_CONFIGURED): Add both
switchcontext.S and callwithstack.S if DRUNTIME_CPU_POWERPC.
* libdruntime/Makefile.in: Regenerate.
* libdruntime/config/powerpc/switchcontext.S: Add !__PPC64__ guards.
* libdruntime/config/powerpc64/callwithstack.S: Add __PPC64__ guards.
* m4/druntime/cpu.m4 (DRUNTIME_CPU_SOURCES): Define DRUNTIME_CPU_POWER
for all powerpc biarchs. Remove DRUNTIME_CPU_POWER64 conditional.
---
libphobos/configure | 23 +---
libphobos/libdruntime/Makefile.am | 6 +-
libphobos/libdruntime/Makefile.in | 104 --
.../{powerpc64 => powerpc}/callwithstack.S| 4 +
.../config/powerpc/switchcontext.S| 4 +
libphobos/m4/druntime/cpu.m4 | 7 +-
6 files changed, 62 insertions(+), 86 deletions(-)
rename libphobos/libdruntime/config/{powerpc64 => powerpc}/callwithstack.S
(98%)
diff --git a/libphobos/configure b/libphobos/configure
index 98d8dc255c1..e461c7442b2 100755
--- a/libphobos/configure
+++ b/libphobos/configure
@@ -692,8 +692,6 @@ DRUNTIME_CPU_SYSTEMZ_FALSE
DRUNTIME_CPU_SYSTEMZ_TRUE
DRUNTIME_CPU_X86_FALSE
DRUNTIME_CPU_X86_TRUE
-DRUNTIME_CPU_POWERPC64_FALSE
-DRUNTIME_CPU_POWERPC64_TRUE
DRUNTIME_CPU_POWERPC_FALSE
DRUNTIME_CPU_POWERPC_TRUE
DRUNTIME_CPU_MIPS_FALSE
@@ -11649,7 +11647,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 11652 "configure"
+#line 11650 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -11755,7 +11753,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 11758 "configure"
+#line 11756 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -13991,12 +13989,9 @@ fi
;;
mips*) druntime_target_cpu_parsed="mips"
;;
- powerpc|powerpcle)
+ powerpc*)
druntime_target_cpu_parsed="powerpc"
;;
- powerpc64|powerpc64le)
- druntime_target_cpu_parsed="powerpc64"
- ;;
i[34567]86|x86_64)
druntime_target_cpu_parsed="x86"
;;
@@ -14039,14 +14034,6 @@ else
DRUNTIME_CPU_POWERPC_FALSE=
fi
- if test "$druntime_target_cpu_parsed" = "powerpc64"; then
- DRUNTIME_CPU_POWERPC64_TRUE=
- DRUNTIME_CPU_POWERPC64_FALSE='#'
-else
- DRUNTIME_CPU_POWERPC64_TRUE='#'
- DRUNTIME_CPU_POWERPC64_FALSE=
-fi
-
if test "$druntime_target_cpu_parsed" = "x86"; then
DRUNTIME_CPU_X86_TRUE=
DRUNTIME_CPU_X86_FALSE='#'
@@ -15605,10 +15592,6 @@ if test -z "${DRUNTIME_CPU_POWERPC_TRUE}" && test -z
"${DRUNTIME_CPU_POWERPC_FAL
as_fn_error $? "conditional \"DRUNTIME_CPU_POWERPC\" was never defined.
Usually this means the macro was only invoked conditionally." "$LINENO" 5
fi
-if test -z "${DRUNTIME_CPU_POWERPC64_TRUE}" && test -z
"${DRUNTIME_CPU_POWERPC64_FALSE}"; then
- as_fn_error $? "conditional \"DRUNTIME_CPU_POWERPC64\" was never defined.
-Usually this means the macro was only invoked conditionally." "$LINENO" 5
-fi
if test -z "${DRUNTIME_CPU_X86_TRUE}" && test -z "${DRUNTIME_CPU_X86_FALSE}";
then
as_fn_error $? "conditional \"DRUNTIME_CPU_X86\" was never defined.
Usually this means the macro was only invoked conditionally." "$LINENO" 5
diff --git a/libphobos/libdruntime/Makefile.am
b/libphobos/libdruntime/Makefile.am
index e1f47d36f90..e1dc24c660b 100644
--- a/libphobos/libdruntime/Makefile.am
+++ b/libphobos/libdruntime/Makefile.am
@@ -81,10 +81,8 @@ if DRUNTIME_CPU_MIPS
DRUNTIME_SOURCES_CONFIGURED += config/mips/switchcontext.S
endif
if DRUNTIME_CPU_POWER
Re: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
On Tue, 28 Apr 2020, Patrick Palka wrote:
> On Tue, 28 Apr 2020, Jason Merrill wrote:
>
> > On 4/28/20 1:41 PM, Patrick Palka wrote:
> > > On Tue, 28 Apr 2020, Patrick Palka wrote:
> > >
> > > > On Tue, 28 Apr 2020, Jason Merrill wrote:
> > > > > On 4/28/20 9:48 AM, Patrick Palka wrote:
> > > > > > When printing the substituted parameter list of a
> > > > > > requires-expression
> > > > > > as
> > > > > > part of the "in requirements with ..." context line during concepts
> > > > > > diagnostics, we weren't considering that substitution into a
> > > > > > parameter
> > > > > > pack can yield zero or multiple parameters.
> > > > > >
> > > > > > Since this patch affects only concepts diagnostics, so far I tested
> > > > > > with
> > > > > > RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer
> > > > > > ICE
> > > > > > with the unreduced testcase in the PR. Is this OK to commit after a
> > > > > > full bootstrap and regtest?
> > > > >
> > > > > OK.
> > > >
> > > > Thanks for the review.
> > > >
> > > > >
> > > > > Though I wonder about printing the dependent form and template
> > > > > arguments
> > > > > instead. Do you have an opinion?
> > > >
> > > > I was going to say that on the one hand, it's convenient to have the
> > > > substituted form printed since it would let us to see through
> > > > complicated dependent type aliases, but it seems we don't strip type
> > > > aliases when pretty printing a parameter and its type with '%q#D'
> > > > anyway. And I can't think of any other possible advantage of printing
> > > > the substituted form.
> > > >
> > > > So IMHO printing the dependent form and template arguments instead would
> > > > be better here. I'll try to write a patch for this today.
> > >
> > > Like so, tested so far with RUNTESTFLAGS="dg.exp=*concepts*" and also
> > > verified we no longer ICE with the unreduced testcase in the PR. Does
> > > the following look OK to commit after a full bootstrap and regtest?
> > >
> > > -- >8 --
> > >
> > > Subject: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
> > >
> > > When printing the substituted parameter list of a requires-expression as
> > > part of the "in requirements with ..." context line during concepts
> > > diagnostics, we weren't considering that substitution into a parameter
> > > pack can yield zero or multiple parameters.
> > >
> > > This patch changes the way we print the parameter list of a
> > > requires-expression in print_requires_expression_info. We now print the
> > > dependent form of the parameter list (along with its template parameter
> > > mapping) instead of printing its substituted form. Besides being an
> > > improvement in its own, this also sidesteps the above parameter pack
> > > expansion issue altogether.
> > >
> > > Tested with RUNTESTFLAGS="dg.exp=*concepts*" and also verified we longer
> > > ICE with the unreduced testcase in the PR. Does this look OK to commit
> > > after a bootstrap and regtest?
> > >
> > > gcc/cp/ChangeLog:
> > >
> > > PR c++/94808
> > > * error.c (print_requires_expression_info): Print the dependent
> > > form of the parameter list with its template parameter mapping,
> > > rather than printing the substituted form.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > PR c++/94808
> > > * g++.dg/concepts/diagnostic12.C: New test.
> > > * g++.dg/concepts/diagnostic5.C: Adjust expected diagnostic.
> > > ---
> > > gcc/cp/error.c | 16
> > > gcc/testsuite/g++.dg/concepts/diagnostic12.C | 16
> > > gcc/testsuite/g++.dg/concepts/diagnostic5.C | 4 ++--
> > > 3 files changed, 22 insertions(+), 14 deletions(-)
> > > create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
> > >
> > > diff --git a/gcc/cp/error.c b/gcc/cp/error.c
> > > index 98c163db572..46970f9b699 100644
> > > --- a/gcc/cp/error.c
> > > +++ b/gcc/cp/error.c
> > > @@ -3746,7 +3746,6 @@ print_requires_expression_info (diagnostic_context
> > > *context, tree constr, tree a
> > > map = tsubst_parameter_mapping (map, args, tf_none, NULL_TREE);
> > > if (map == error_mark_node)
> > > return;
> > > - args = get_mapped_args (map);
> > > print_location (context, cp_expr_loc_or_input_loc (expr));
> > > pp_verbatim (context->printer, "in requirements ");
> > > @@ -3756,19 +3755,12 @@ print_requires_expression_info (diagnostic_context
> > > *context, tree constr, tree a
> > > pp_verbatim (context->printer, "with ");
> > > while (parms)
> > > {
> > > - tree next = TREE_CHAIN (parms);
> > > -
> > > - TREE_CHAIN (parms) = NULL_TREE;
> > > - cp_unevaluated u;
> > > - tree p = tsubst (parms, args, tf_none, NULL_TREE);
> > > - pp_verbatim (context->printer, "%q#D", p);
> > > - TREE_CHAIN (parms) = next;
> > > -
> > > - if (next)
> > > + pp_verbatim (context->printer, "%q#D", parms);
> > > + if (TREE_CHAIN (parms))
>
Re: [RFC] Clarify -ffinite-math-only documentation
On 4/27/20 9:08 AM, Matthias Kretz wrote: @item -ffinite-math-only @opindex ffinite-math-only -Allow optimizations for floating-point arithmetic that assume -that arguments and results are not NaNs or +-Infs. +Assume that floating-point types in the language do not have representations for +NaNs and +-Inf. Whether floating-point hardware supports and acts on NaNs and ++-Inf is not affected. The behavior of a program that uses a NaN or +-Inf value +as function argument, macro argument, or operand is undefined. I'm chiming in here a bit late, but as documentation maintainer, I'm not happy with the proposed new wording either, because it doesn't tell users what this option is *for*. To expand on the existing language a bit, its purpose is to allow GCC to compile floating-point arithmetic expressions to hardware instructions that might not comply with the IEEE 754 requirements for handling of NaNs and Inf, and perhaps secondarily to elide tests for NaNs and +-Inf in inlined code. The proposed language "Assume that..." is ambiguous; who/what should assume this, and why? I think it would be fine to say explicitly that such cases have undefined behavior if you use this option, and also to change "arguments" in the second line to "operands" to avoid confusion with function or macro arguments, which isn't the point. I'll defer to the subject-matter experts on the exact wording for "undefined" and any other user-visible consequences of compiling with this option. -Sandra
Re: [PATCH] var-tracking.c: Fix possible use of uninitialized variable pre
On Tue, Apr 28, 2020 at 11:44:58AM +0200, Richard Biener wrote:
> On Tue, Apr 28, 2020 at 11:28 AM Stefan Schulze Frielinghaus via
> Gcc-patches wrote:
> >
> > While bootstrapping GCC on S/390 the following warning/error is raised:
> >
> > gcc/var-tracking.c:10239:34: error: 'pre' may be used uninitialized in this
> > function [-Werror=maybe-uninitialized]
> > 10239 | VTI (bb)->out.stack_adjust += pre;
> > | ^
> >
> > The lines of interest are:
> >
> > HOST_WIDE_INT pre, post = 0;
> > // ...
> > if (!frame_pointer_needed)
> > {
> > insn_stack_adjust_offset_pre_post (insn, &pre, &post);
> > // ...
> > }
> >
> > // ...
> > adjust_insn (bb, insn);
> >
> > if (!frame_pointer_needed && pre)
> > VTI (bb)->out.stack_adjust += pre;
> >
> > Both if statements depend on global variable frame_pointer_needed. In
> > function
> > insn_stack_adjust_offset_pre_post local variable pre is initialized. The
> > problematic part is the function call between both if statements. Since
> > adjust_insn also calls functions which are defined in a different
> > compilation
> > unit, we are not able to prove that global variable frame_pointer_needed is
> > not
> > altered by adjust_insn and its siblings. Thus we must assume that
> > frame_pointer_needed may be true before the call and false afterwards which
> > renders the warning true (admitted the location hint is not totally
> > perfect).
> > By initialising pre we silence the warning.
> >
> > Bootstrapped and regtested on S/390. Ok for master?
>
> I think even better would be to move the pre declaration inside
>
> FOR_BB_INSNS_SAFE (bb, insn, next)
> {
> if (INSN_P (insn))
> {
>
> initialized with zero and then elide the !frame_pointer_needed part
> of
>
> if (!frame_pointer_needed && pre)
>
> can you check that works? OK if it does.
That works, too. Removed !frame_pointer_needed from pre as well as post
part of the two if statements. Please find attached an updated version
of the patch.
Regtest is running over night. Assuming it succeeds I will push if no
one objects.
While bootstrapping GCC on S/390 the following warning/error is raised:
gcc/var-tracking.c:10239:34: error: 'pre' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
10239 | VTI (bb)->out.stack_adjust += pre;
| ^
The lines of interest are:
HOST_WIDE_INT pre, post = 0;
// ...
if (!frame_pointer_needed)
{
insn_stack_adjust_offset_pre_post (insn, &pre, &post);
// ...
}
// ...
adjust_insn (bb, insn);
if (!frame_pointer_needed && pre)
VTI (bb)->out.stack_adjust += pre;
Both if statements depend on global variable frame_pointer_needed. In function
insn_stack_adjust_offset_pre_post local variable pre is initialized. The
problematic part is the function call between both if statements. Since
adjust_insn also calls functions which are defined in a different compilation
unit, we are not able to prove that global variable frame_pointer_needed is not
altered by adjust_insn and its siblings. Thus we must assume that
frame_pointer_needed may be true before the call and false afterwards which
renders the warning true (admitted the location hint is not totally perfect).
By initialising pre we silence the warning.
gcc/ChangeLog:
2020-04-28 Stefan Schulze Frielinghaus
* var-tracking.c (vt_initialize): Move variables pre and post
into inner block and initialize both in order to fix warning
about uninitialized use. Remove unnecessary checks for
frame_pointer_needed.
---
gcc/var-tracking.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index 0d39326aa63..fc861a0d8ce 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -10171,7 +10171,6 @@ vt_initialize (void)
FOR_EACH_BB_FN (bb, cfun)
{
rtx_insn *insn;
- HOST_WIDE_INT pre, post = 0;
basic_block first_bb, last_bb;
if (MAY_HAVE_DEBUG_BIND_INSNS)
@@ -10216,6 +10215,8 @@ vt_initialize (void)
{
if (INSN_P (insn))
{
+ HOST_WIDE_INT pre = 0, post = 0;
+
if (!frame_pointer_needed)
{
insn_stack_adjust_offset_pre_post (insn, &pre, &post);
@@ -10235,7 +10236,7 @@ vt_initialize (void)
cselib_hook_called = false;
adjust_insn (bb, insn);
- if (!frame_pointer_needed && pre)
+ if (pre)
VTI (bb)->out.stack_adjust += pre;
if (DEBUG_MARKER_INSN_P (insn))
@@ -10262,7 +10263,7 @@ vt_initialize (void)
add_with_sets (insn, 0, 0);
cancel_changes (0);
- if (!frame_pointer_needed && post)
+
[PATCH v3] c++, middle-end, rs6000: Fix C++17 ABI incompatibilities during class layout and [[no_unique_address]] handling [PR94707]
On Tue, Apr 28, 2020 at 05:42:00PM +0200, Jakub Jelinek wrote:
> Ok, below in the updated patch:
This is what I've successfully bootstrapped/regtested on powerpc64le-linux
(last posted patch with the lto-common.c addition included).
Jason has already approved the non-rs6000 parts, so are those ok for trunk too?
2020-04-28 Jakub Jelinek
PR target/94707
* tree-core.h (tree_decl_common): Note decl_flag_0 used for
DECL_FIELD_ABI_IGNORED.
* tree.h (DECL_FIELD_ABI_IGNORED): Define.
* calls.h (cxx17_empty_base_field_p): Change into a temporary
macro, check DECL_FIELD_ABI_IGNORED flag with no "no_unique_address"
attribute.
* calls.c (cxx17_empty_base_field_p): Remove.
* tree-streamer-out.c (pack_ts_decl_common_value_fields): Handle
DECL_FIELD_ABI_IGNORED.
* tree-streamer-in.c (unpack_ts_decl_common_value_fields): Likewise.
* lto-streamer-out.c (hash_tree): Likewise.
* config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Rename
cxx17_empty_base_seen to empty_base_seen, change type to int *,
adjust recursive calls, use DECL_FIELD_ABI_IGNORED instead of
cxx17_empty_base_field_p, if "no_unique_address" attribute is
present, propagate that to the caller too.
(rs6000_discover_homogeneous_aggregate): Adjust
rs6000_aggregate_candidate caller, emit different diagnostics
when c++17 empty base fields are present and when empty
[[no_unique_address]] fields are present.
* config/rs6000/rs6000.c (rs6000_special_round_type_align,
darwin_rs6000_special_round_type_align): Skip DECL_FIELD_ABI_IGNORED
fields.
* class.c (build_base_field): Set DECL_FIELD_ABI_IGNORED on C++17 empty
base artificial FIELD_DECLs.
(layout_class_type): Set DECL_FIELD_ABI_IGNORED on empty class
field_poverlapping_p FIELD_DECLs.
* lto-common.c (compare_tree_sccs_1): Handle DECL_FIELD_ABI_IGNORED.
* g++.target/powerpc/pr94707-1.C: New test.
* g++.target/powerpc/pr94707-2.C: New test.
* g++.target/powerpc/pr94707-3.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
* g++.target/powerpc/pr94707-5.C: New test.
* g++.target/powerpc/pr94707-4.C: New test.
--- gcc/tree-core.h.jj 2020-04-08 18:15:36.936946772 +0200
+++ gcc/tree-core.h 2020-04-28 15:14:06.598814022 +0200
@@ -1709,7 +1709,8 @@ struct GTY(()) tree_decl_common {
unsigned lang_flag_8 : 1;
/* In VAR_DECL and PARM_DECL, this is DECL_REGISTER
- IN TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P. */
+ In TRANSLATION_UNIT_DECL, this is TRANSLATION_UNIT_WARN_EMPTY_P.
+ In FIELD_DECL, this is DECL_FIELD_ABI_IGNORED. */
unsigned decl_flag_0 : 1;
/* In FIELD_DECL, this is DECL_BIT_FIELD
In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
--- gcc/tree.h.jj 2020-04-08 18:15:36.939946727 +0200
+++ gcc/tree.h 2020-04-28 15:13:07.579695258 +0200
@@ -2750,6 +2750,13 @@ extern void decl_value_expr_insert (tree
/* In a FIELD_DECL, indicates this field should be bit-packed. */
#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->base.u.bits.packed_flag)
+/* In a FIELD_DECL, indicates this field should be ignored for ABI decisions
+ like passing/returning containing struct by value.
+ Set for C++17 empty base artificial FIELD_DECLs as well as
+ empty [[no_unique_address]] non-static data members. */
+#define DECL_FIELD_ABI_IGNORED(NODE) \
+ (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0)
+
/* Nonzero in a FIELD_DECL means it is a bit field, and must be accessed
specially. */
#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_1)
--- gcc/calls.h.jj 2020-04-27 14:31:09.123020831 +0200
+++ gcc/calls.h 2020-04-28 15:26:29.221724466 +0200
@@ -135,6 +135,9 @@ extern tree get_attr_nonstring_decl (tre
extern void maybe_warn_nonstring_arg (tree, tree);
extern bool get_size_range (tree, tree[2], bool = false);
extern rtx rtx_for_static_chain (const_tree, bool);
-extern bool cxx17_empty_base_field_p (const_tree);
+/* FIXME: Remove after all backends are converted. */
+#define cxx17_empty_base_field_p(t) \
+ (DECL_FIELD_ABI_IGNORED (t) \
+ && !lookup_attribute ("no_unique_address", DECL_ATTRIBUTES (t)))
#endif // GCC_CALLS_H
--- gcc/calls.c.jj 2020-04-27 14:31:09.117020922 +0200
+++ gcc/calls.c 2020-04-28 15:26:42.276529517 +0200
@@ -6261,23 +6261,5 @@ must_pass_va_arg_in_stack (tree type)
return targetm.calls.must_pass_in_stack (arg);
}
-/* Return true if FIELD is the C++17 empty base field that should
- be ignored for ABI calling convention decisions in order to
- maintain ABI compatibility between C++14 and earlier, which doesn't
- add this FIELD to classes with empty bases, and C++17 and later
- which does. */
-
-bool
-cxx17_empty_base_field_p (const_tree
[PATCH] c++: Satisfaction caching of inherited ctor [PR94819]
As observed in PR94719, an inherited constructor for an instantiation of
a constructor template confusingly has as its DECL_INHERITED_CTOR the
TEMPLATE_DECL of the constructor template rather than the particular
instantiation of the template.
This means two inherited constructors for two different instantiations
of the same constructor template will have the same DECL_INHERITED_CTOR.
And since in satisfy_declaration_constraints our decl satisfaction cache
is keyed off of the result of strip_inheriting_ctors, we may end up
conflating the satisfaction value of the two inherited constructors'
constraints.
This patch fixes this issue by using the original tree, not the result
of strip_inheriting_ctors, as the key to the decl satisfaction cache.
Passes 'make check-c++', does this look OK to commit after a full
bootstrap and regtest?
gcc/cp/ChangeLog:
PR c++/94819
* constraint.cc (satisfy_declaration_constraints): Use saved_t
instead of t as the key to decl_satisfied_cache.
gcc/testsuite/ChangeLog:
PR c++/94819
* g++.dg/cpp2a/concepts-inherit-ctor10.C: New test.
* g++.dg/cpp2a/concepts-inherit-ctor11.C: New test.
---
gcc/cp/constraint.cc | 4 ++--
.../g++.dg/cpp2a/concepts-inherit-ctor10.C| 18
.../g++.dg/cpp2a/concepts-inherit-ctor11.C| 21 +++
3 files changed, 41 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-inherit-ctor10.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-inherit-ctor11.C
diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 06161b8c8c4..866b0f51b05 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -2752,7 +2752,7 @@ satisfy_declaration_constraints (tree t, subst_info info)
info.in_decl = t;
if (info.quiet ())
-if (tree *result = hash_map_safe_get (decl_satisfied_cache, t))
+if (tree *result = hash_map_safe_get (decl_satisfied_cache, saved_t))
return *result;
/* Get the normalized constraints. */
@@ -2787,7 +2787,7 @@ satisfy_declaration_constraints (tree t, subst_info info)
}
if (info.quiet ())
-hash_map_safe_put (decl_satisfied_cache, t, result);
+hash_map_safe_put (decl_satisfied_cache, saved_t, result);
return result;
}
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-inherit-ctor10.C
b/gcc/testsuite/g++.dg/cpp2a/concepts-inherit-ctor10.C
new file mode 100644
index 000..387c07ae6b2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-inherit-ctor10.C
@@ -0,0 +1,18 @@
+// PR c++/94819
+// { dg-do compile { target concepts } }
+
+struct dna4 {};
+struct rna4 {};
+
+struct alphabet_tuple_base {
+template
+requires __is_same(component_type, rna4)
+alphabet_tuple_base(component_type) {}
+};
+
+struct structured_rna : alphabet_tuple_base {
+using alphabet_tuple_base::alphabet_tuple_base;
+};
+
+structured_rna t2{dna4{}}; // { dg-error "no match" }
+structured_rna t3{rna4{}}; // { dg-bogus "no match" }
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-inherit-ctor11.C
b/gcc/testsuite/g++.dg/cpp2a/concepts-inherit-ctor11.C
new file mode 100644
index 000..947edd84e53
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-inherit-ctor11.C
@@ -0,0 +1,21 @@
+// PR c++/94819
+// { dg-do compile { target concepts } }
+
+struct dna4 {};
+struct rna4 {};
+
+template
+struct alphabet_tuple_base {
+template
+requires __is_same(component_type, component_types)
+alphabet_tuple_base(component_type) {}
+};
+
+template
+struct structured_rna : alphabet_tuple_base {
+using base_type = alphabet_tuple_base;
+using base_type::base_type;
+};
+
+structured_rna t2{dna4{}}; // { dg-error "no match" }
+structured_rna t3{rna4{}}; // { dg-bogus "no match" }
--
2.26.2.266.ge870325ee8
[committed] Fix length of H8/SX multiply patterns
This is a minor H8 specific bugfix. The H8/SX multiply instructions are all 4
bytes in length, but the machine description claims they are 2 bytes in length.
This can cause GCC to emit a short branch when a long branch was actually
needed.
Sadly the assembler didn't complain and instead the branch goes to the wrong
place and all hell breaks loose when you try to execute the code.
My tester tripped over this while testing the H8 port.
Committed to the trunk.
Jeff
commit b3346399a245e9f262792f76f67f8e2dfa7e064c
Author: Jeff Law
Date: Tue Apr 28 16:34:45 2020 -0400
Fix some testsuite failures for H8/SX multilibs where short branches
where used when long branches were necessary.
* config/h8300/h8300.md (H8/SX mult patterns): All H8/SX specific
multiply patterns are 4 bytes long.
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 905d2b8a2f5..9c947299964 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2020-04-28 Jeff Law
+
+ * config/h8300/h8300.md (H8/SX mult patterns): All H8/SX specific
+ multiply patterns are 4 bytes long.
+
2020-04-28 Kyrylo Tkachov
* config/arm/arm-cpus.in (cortex-m55): Remove +nofp option.
diff --git a/gcc/config/h8300/h8300.md b/gcc/config/h8300/h8300.md
index fdd2d8b02d7..3e5cdbeeebe 100644
--- a/gcc/config/h8300/h8300.md
+++ b/gcc/config/h8300/h8300.md
@@ -1146,7 +1146,7 @@
(match_operand:HSI 2 "reg_or_nibble_operand" "r IP4>X")))]
"TARGET_H8300SX"
{ return mode == HImode ? "muls.w\\t%T2,%T0" : "muls.l\\t%S2,%S0"; }
- [(set_attr "length" "2")
+ [(set_attr "length" "4")
(set_attr "cc" "set_zn")])
(define_insn "smulsi3_highpart"
@@ -1159,7 +1159,7 @@
(const_int 32]
"TARGET_H8300SXMUL"
"muls/u.l\\t%S2,%S0"
- [(set_attr "length" "2")
+ [(set_attr "length" "4")
(set_attr "cc" "set_zn")])
(define_insn "umulsi3_highpart"
@@ -1172,7 +1172,7 @@
(const_int 32]
"TARGET_H8300SX"
"mulu/u.l\\t%S2,%S0"
- [(set_attr "length" "2")
+ [(set_attr "length" "4")
(set_attr "cc" "none_0hit")])
;; This is a "bridge" instruction. Combine can't cram enough insns
