Re: [PTX] simplify movs
On 12/02/2015 04:09 PM, Nathan Sidwell wrote:
+/* Output a pattern for a move instruction. */
+
+const char *
+nvptx_output_mov_insn (rtx dst, rtx src)
+{
+ machine_mode dst_mode = GET_MODE (dst);
+ machine_mode dst_inner = (GET_CODE (dst) == SUBREG
+ ? GET_MODE (XEXP (dst, 0)) : dst_mode);
+ machine_mode src_inner = (GET_CODE (src) == SUBREG
+ ? GET_MODE (XEXP (src, 0)) : dst_mode);
+
+ if (REG_P (dst) && REGNO (dst) == NVPTX_RETURN_REGNUM && dst_mode == HImode)
+/* Special handling for the return register. It's never really an
+ HI object, and only occurs as the destination of a move
+ insn. */
+dst_inner = SImode;
+
+ if (src_inner == dst_inner)
+return "%.\tmov%t0\t%0, %1;";
+
+ if (CONSTANT_P (src))
+return (GET_MODE_CLASS (dst_inner) == MODE_INT
+ && GET_MODE_CLASS (src_inner) != MODE_FLOAT
+ ? "%.\tmov%t0\t%0, %1;" : "%.\tmov.b%T0\t%0, %1;");
Hi,
src_inner uses dst_mode rather than GET_MODE (src). I'm trying to
understand if that is intentional or not.
F.i., for this insn:
(insn 7 6 8 2
(set (reg:QI 67)
(const_int 1 [0x1])) 2 {*movqi_insn}
(nil))
...
when entering nvptx_output_mov_insn we have:
- GET_MODE (dst) == QI and GET_MODE (src) == VOID, but
- dst_inner == QI and src_inner == QI
So we handle this insn using this clause:
...
if (src_inner == dst_inner)
return "%.\tmov%t0\t%0, %1;";
...
rather than using the const handling clause:
...
if (CONSTANT_P (src))
return (GET_MODE_CLASS (dst_inner) == MODE_INT
&& GET_MODE_CLASS (src_inner) != MODE_FLOAT
? "%.\tmov%t0\t%0, %1;" : "%.\tmov.b%T0\t%0, %1;");
...
Using attached patch, we get dst_inner == QI and src_inner == VOID, and
the insn is handled by the const handling clause instead, and the same
string is returned as before.
I can imagine that src_inner uses dst_mode to avoid setting src_inner to
VOIDmode (in which case a comment explaining that would avoid the
impression of a copy-pasto). But AFAICT, it's not necessary.
Thanks,
- Tom
Fix src_inner in nvptx_output_mov_insn
---
gcc/config/nvptx/nvptx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c
index 4c35c16..6951e27 100644
--- a/gcc/config/nvptx/nvptx.c
+++ b/gcc/config/nvptx/nvptx.c
@@ -2146,10 +2146,11 @@ const char *
nvptx_output_mov_insn (rtx dst, rtx src)
{
machine_mode dst_mode = GET_MODE (dst);
+ machine_mode src_mode = GET_MODE (src);
machine_mode dst_inner = (GET_CODE (dst) == SUBREG
? GET_MODE (XEXP (dst, 0)) : dst_mode);
machine_mode src_inner = (GET_CODE (src) == SUBREG
- ? GET_MODE (XEXP (src, 0)) : dst_mode);
+ ? GET_MODE (XEXP (src, 0)) : src_mode);
rtx sym = src;
if (GET_CODE (sym) == CONST)
Re: MinGW compilation warnings in libiberty's waitpid.c
> From: DJ Delorie > Cc: gcc-patches@gcc.gnu.org, gdb-patc...@sourceware.org > Date: Fri, 19 May 2017 21:28:25 -0400 > > > Please try this patch, since my mingw environment is old: > > Index: libiberty/ChangeLog > === > --- libiberty/ChangeLog (revision 248307) > +++ libiberty/ChangeLog (working copy) > @@ -1,3 +1,7 @@ > +2017-05-19 Eli Zaretskii > + > + * configure.ac (*-*-mingw*): Don't build waitpid.c. > + > 2017-05-02 Iain Buclaw > > * d-demangle.c (dlang_hexdigit): New function. > Index: libiberty/configure.ac > === > --- libiberty/configure.ac (revision 248307) > +++ libiberty/configure.ac (working copy) > @@ -493,7 +493,6 @@ > AC_LIBOBJ([strnlen]) > AC_LIBOBJ([strverscmp]) > AC_LIBOBJ([vasprintf]) > -AC_LIBOBJ([waitpid]) > > for f in $funcs; do >case "$f" in > Hmm... no, this doesn't solve the problem. The expansion of AC_LIBOBJ for waitpid is gone from the configure script, but the value of LIBOBJS in libiberty/Makefile still includes waitpid.o. What else is related to this? One caveat: I needed to hack config/override.m4 to allow me to run autoconf 2.69 I have installed, because otherwise it insists on autoconf 2.64 which I don't have. I hope this isn't the reason for the incomplete solution.
New Danish PO file for 'gcc' (version 7.1.0)
Hello, gentle maintainer. This is a message from the Translation Project robot. A revised PO file for textual domain 'gcc' has been submitted by the Danish team of translators. The file is available at: http://translationproject.org/latest/gcc/da.po (This file, 'gcc-7.1.0.da.po', has just now been sent to you in a separate email.) All other PO files for your package are available in: http://translationproject.org/latest/gcc/ Please consider including all of these in your next release, whether official or a pretest. Whenever you have a new distribution with a new version number ready, containing a newer POT file, please send the URL of that distribution tarball to the address below. The tarball may be just a pretest or a snapshot, it does not even have to compile. It is just used by the translators when they need some extra translation context. The following HTML page has been updated: http://translationproject.org/domain/gcc.html If any question arises, please contact the translation coordinator. Thank you for all your work, The Translation Project robot, in the name of your translation coordinator.
[Patch, Fortran, OOP] PR 80766: [7/8 Regression] ICE with type-bound procedure returning an array
Hi all,
the attached patch fixes an ICE-on-valid regression by making sure
that the relevant vtype symbol is resolved properly (for further
discussion see the PR).
The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk and 7-branch?
Cheers,
Janus
2017-05-21 Janus Weil
PR fortran/80766
* resolve.c (resolve_fl_derived): Make sure that vtype symbols are
properly resolved.
2017-05-21 Janus Weil
PR fortran/80766
* gfortran.dg/typebound_call_28.f90: New test.
Index: gcc/fortran/resolve.c
===
--- gcc/fortran/resolve.c (revision 248308)
+++ gcc/fortran/resolve.c (working copy)
@@ -13832,6 +13832,8 @@ resolve_fl_derived (gfc_symbol *sym)
gfc_symbol *vtab = gfc_find_derived_vtab (data->ts.u.derived);
gcc_assert (vtab);
vptr->ts.u.derived = vtab->ts.u.derived;
+ if (!resolve_fl_derived0 (vptr->ts.u.derived))
+ return false;
}
}
! { dg-do compile }
!
! PR 80766: [7/8 Regression] [OOP] ICE with type-bound procedure returning an array
!
! Contributed by Vladimir Fuka
module m1
type :: base
contains
procedure :: fun
end type
type, extends(base) :: child
end type
contains
function fun(o) result(res)
real :: res(3)
class(base) :: o
res = 0
end function
end module
module m2
contains
subroutine sub(o)
use m1
class(child) :: o
real :: res(3)
res = o%fun()
end subroutine
end module
Allow some NOP conversions in (X+CST1)+CST2 in match.pd
Hello,
generalizing a bit one transformation, to avoid a regression with another
patch I am working on. Handling conversions always gets messy :-( It would
have been easier to stick to scalars and wide_int, but since the existing
transformation handles vectors, I didn't want to regress.
Bootstrap+testsuite on powerpc64le-unknown-linux-gnu.
2017-05-22 Marc Glisse
gcc/
* match.pd ((A +- CST1) +- CST2): Allow some conversions.
* tree.c (drop_tree_overflow): Handle COMPLEX_CST and VECTOR_CST.
gcc/testsuite/
* gcc.dg/tree-ssa/addadd.c: New file.
--
Marc GlisseIndex: match.pd
===
--- match.pd (revision 248312)
+++ match.pd (working copy)
@@ -1265,29 +1265,53 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
(simplify
(minus @0 (plus:c @0 @1))
(negate @1))
(simplify
(minus @0 (minus @0 @1))
@1)
/* (A +- CST1) +- CST2 -> A + CST3 */
(for outer_op (plus minus)
(for inner_op (plus minus)
+ neg_inner_op (minus plus)
(simplify
- (outer_op (inner_op @0 CONSTANT_CLASS_P@1) CONSTANT_CLASS_P@2)
- /* If the constant operation overflows we cannot do the transform
- as we would introduce undefined overflow, for example
- with (a - 1) + INT_MIN. */
- (with { tree cst = const_binop (outer_op == inner_op
- ? PLUS_EXPR : MINUS_EXPR, type, @1, @2); }
- (if (cst && !TREE_OVERFLOW (cst))
- (inner_op @0 { cst; } ))
+ (outer_op (convert? (inner_op @0 CONSTANT_CLASS_P@1)) CONSTANT_CLASS_P@2)
+ (if (tree_nop_conversion_p (type, TREE_TYPE (@0)))
+ /* If one of the types wraps, use that one. */
+ (if (!ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type))
+ (if (outer_op == PLUS_EXPR)
+ (plus (convert @0) (inner_op @2 (convert @1)))
+ (minus (convert @0) (neg_inner_op @2 (convert @1
+ (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
+ || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)))
+ (if (outer_op == PLUS_EXPR)
+ (convert (plus @0 (inner_op (convert @2) @1)))
+ (convert (minus @0 (neg_inner_op (convert @2) @1
+ /* If the constant operation overflows we cannot do the transform
+ directly as we would introduce undefined overflow, for example
+ with (a - 1) + INT_MIN. */
+ (if (types_match (type, @0))
+ (with { tree cst = const_binop (outer_op == inner_op
+ ? PLUS_EXPR : MINUS_EXPR,
+ type, @1, @2); }
+ (if (cst && !TREE_OVERFLOW (cst))
+ (inner_op @0 { cst; } )
+ /* X+INT_MAX+1 is X-INT_MIN. */
+ (if (INTEGRAL_TYPE_P (type) && cst
+ && wi::eq_p (cst, wi::min_value (type)))
+ (neg_inner_op @0 { wide_int_to_tree (type, cst); })
+ /* Last resort, use some unsigned type. */
+ (with { tree utype = unsigned_type_for (type); }
+ (convert (inner_op
+ (convert:utype @0)
+ (convert:utype
+ { drop_tree_overflow (cst); }))
/* (CST1 - A) +- CST2 -> CST3 - A */
(for outer_op (plus minus)
(simplify
(outer_op (minus CONSTANT_CLASS_P@1 @0) CONSTANT_CLASS_P@2)
(with { tree cst = const_binop (outer_op, type, @1, @2); }
(if (cst && !TREE_OVERFLOW (cst))
(minus { cst; } @0)
/* CST1 - (CST2 - A) -> CST3 + A */
Index: testsuite/gcc.dg/tree-ssa/addadd.c
===
--- testsuite/gcc.dg/tree-ssa/addadd.c (nonexistent)
+++ testsuite/gcc.dg/tree-ssa/addadd.c (working copy)
@@ -0,0 +1,34 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int f(unsigned x){
+ x += 123;
+ int y = x;
+ y -= 99;
+ return y;
+}
+unsigned g(int x){
+ x += 123;
+ unsigned y = x;
+ y -= 99;
+ return y;
+}
+int h(int x){
+ x += __INT_MAX__;
+ x += 1;
+ return x;
+}
+int i(int x){
+ x += __INT_MAX__;
+ x += __INT_MAX__;
+ return x;
+}
+typedef int S __attribute__((vector_size(16)));
+void j(S*x){
+ *x += __INT_MAX__;
+ *x += __INT_MAX__;
+}
+
+/* { dg-final { scan-tree-dump-times " \\+ 24;" 2 "optimized" } } */
+/* { dg-final { scan-tree-dump-times "\\(unsigned int\\)" 2 "optimized" } } */
+/* { dg-final { scan-tree-dump-not "2147483647" "optimized" } } */
Index: tree.c
===
--- tree.c (revision 248312)
+++ tree.c (working copy)
@@ -13131,20 +13131,39 @@ drop_tree_overflow (tree t)
gcc_checking_assert (TREE_OVERFLOW (t));
/* For tree codes with a sharing machinery re-build the result. */
if (TREE_CODE (t) == INTEGER_CST)
return wide_int_to_tree (TREE_TYPE (t), t);
/* Otherwise, as all tcc_constants are possibly shared, copy the node
and drop the flag. */
t = copy_node (t);
TREE_OVERFLOW (t) = 0;
+
+ /* For constants that contain nested constants, drop the flag
+ from those as well. */
+ if (TREE_CODE (t) == COMPLEX_CST)
+{
+ if (TREE_OVERFLOW (TREE_REALPART (t)))
+ TREE_REALPART (t) = drop_tree_overflow (TREE_REALPART (t));
+ if (TREE_OVERFLOW (TREE_IMAGP
signed multiplication for pointer offsets
Hello,
when we have a double*p, computing p+n, we multiply n by 8 (size of
double) then add it to p. According to the comment just below the modified
lines in the attached patch, the multiplication is supposed to happen in a
signed type. However, that does not match the current code which uses
sizetype. This is causing missed optimizations, for instance, when
comparing p+m and p+n, we might end up comparing 8*m to 8*n, which is not
the same as comparing m and n if overflow can happen.
I tried this patch to see how much breaks. And actually, not that much
does. There are a few fragile testcases that want to match "q_. " but it
is now "q_10 ", or they expect 3 simplifications and get 5 (same final
code). One was confused by a cast in the middle of x+cst1+cst2. A couple
were hit by the lack of CSE between (x+1)*8, x*8+8, and some variants with
casts in the middle, but that's already an issue without the patch. A few
vectorization testcases failed because SCEV did not recognize a simple
increment of a variable with NOP casts in the middle, that would be worth
fixing. The patch actually fixed another vectorization testcase.
I guess it would help if pointer_plus switched to a signed second
argument, to be consistent with this and reduce the number of casts.
I am not proposing the patch as is, but is this the direction we want to
follow, or should I just fix the comment to match what the code does?
--
Marc GlisseIndex: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c (revision 248308)
+++ gcc/c-family/c-common.c (working copy)
@@ -3106,24 +3106,24 @@ pointer_int_sum (location_t loc, enum tr
because weird cases involving pointer arithmetic
can result in a sum or difference with different type args. */
ptrop = build_binary_op (EXPR_LOCATION (TREE_OPERAND (intop, 1)),
subcode, ptrop,
convert (int_type, TREE_OPERAND (intop, 1)), 1);
intop = convert (int_type, TREE_OPERAND (intop, 0));
}
/* Convert the integer argument to a type the same size as sizetype
so the multiply won't overflow spuriously. */
- if (TYPE_PRECISION (TREE_TYPE (intop)) != TYPE_PRECISION (sizetype)
- || TYPE_UNSIGNED (TREE_TYPE (intop)) != TYPE_UNSIGNED (sizetype))
-intop = convert (c_common_type_for_size (TYPE_PRECISION (sizetype),
- TYPE_UNSIGNED (sizetype)), intop);
+ if (TYPE_PRECISION (TREE_TYPE (intop)) != TYPE_PRECISION (ssizetype)
+ || TYPE_UNSIGNED (TREE_TYPE (intop)) != TYPE_UNSIGNED (ssizetype))
+intop = convert (c_common_type_for_size (TYPE_PRECISION (ssizetype),
+ TYPE_UNSIGNED (ssizetype)), intop);
/* Replace the integer argument with a suitable product by the object size.
Do this multiplication as signed, then convert to the appropriate type
for the pointer operation and disregard an overflow that occurred only
because of the sign-extension change in the latter conversion. */
{
tree t = fold_build2_loc (loc, MULT_EXPR, TREE_TYPE (intop), intop,
convert (TREE_TYPE (intop), size_exp));
intop = convert (sizetype, t);
if (TREE_OVERFLOW_P (intop) && !TREE_OVERFLOW (t))
Relax VIEW_CONVERT_EXPR - CONVERT_EXPR combination
Hello,
SRA uses char when scalarizing bool, and we end up with
_6 = u_1(D) == 0.0;
_7 = (unsigned char) _6;
_3 = VIEW_CONVERT_EXPR<_Bool>(_7);
which we currently do not simplify. I am not completely sure what happens
with types whose precision does not cover their size, I hope this is safe.
Bootstrap+testsuite on powerpc64le-unknown-linux-gnu.
2017-05-22 Marc Glisse
gcc/
* match.pd (view_convert (convert@0 @1)): Handle zero-extension.
gcc/testsuite/
* gcc.dg/tree-ssa/vce-1.c: New file.
--
Marc GlisseIndex: match.pd
===
--- match.pd (revision 248312)
+++ match.pd (working copy)
@@ -1798,27 +1798,30 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
/* For integral conversions with the same precision or pointer
conversions use a NOP_EXPR instead. */
(simplify
(view_convert @0)
(if ((INTEGRAL_TYPE_P (type) || POINTER_TYPE_P (type))
&& (INTEGRAL_TYPE_P (TREE_TYPE (@0)) || POINTER_TYPE_P (TREE_TYPE (@0)))
&& TYPE_PRECISION (type) == TYPE_PRECISION (TREE_TYPE (@0)))
(convert @0)))
-/* Strip inner integral conversions that do not change precision or size. */
+/* Strip inner integral conversions that do not change precision or size, or
+ zero-extend while keeping the same size (for bool-to-char). */
(simplify
(view_convert (convert@0 @1))
(if ((INTEGRAL_TYPE_P (TREE_TYPE (@0)) || POINTER_TYPE_P (TREE_TYPE (@0)))
&& (INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE (@1)))
- && (TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1)))
- && (TYPE_SIZE (TREE_TYPE (@0)) == TYPE_SIZE (TREE_TYPE (@1
+ && TYPE_SIZE (TREE_TYPE (@0)) == TYPE_SIZE (TREE_TYPE (@1))
+ && (TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1))
+ || (TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE (@1))
+ && TYPE_UNSIGNED (TREE_TYPE (@1)
(view_convert @1)))
/* Re-association barriers around constants and other re-association
barriers can be removed. */
(simplify
(paren CONSTANT_CLASS_P@0)
@0)
(simplify
(paren (paren@1 @0))
@1)
Index: testsuite/gcc.dg/tree-ssa/vce-1.c
===
--- testsuite/gcc.dg/tree-ssa/vce-1.c (nonexistent)
+++ testsuite/gcc.dg/tree-ssa/vce-1.c (working copy)
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+typedef struct { _Bool b; } A;
+_Bool f(double u){
+ A a;
+ if(u==0)
+a.b=1;
+ else
+a.b=0;
+ return a.b;
+}
+
+/* { dg-final { scan-tree-dump-not "VIEW_CONVERT_EXPR" "optimized" } } */
Re: [Patch, Fortran, OOP] PR 80766: [7/8 Regression] ICE with type-bound procedure returning an array
On 05/21/2017 09:14 AM, Janus Weil wrote: Hi all, the attached patch fixes an ICE-on-valid regression by making sure that the relevant vtype symbol is resolved properly (for further discussion see the PR). The patch regtests cleanly on x86_64-linux-gnu. Ok for trunk and 7-branch? Cheers, Janus 2017-05-21 Janus Weil PR fortran/80766 * resolve.c (resolve_fl_derived): Make sure that vtype symbols are properly resolved. 2017-05-21 Janus Weil PR fortran/80766 * gfortran.dg/typebound_call_28.f90: New test. OK to commit. I have been unable to commit another patch today. Server down or internet connection gone. I will try later. Hope its OK from your side of the pond. Jerry
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
On 05/19/2017 03:42 PM, Jason Merrill wrote: On Fri, May 19, 2017 at 4:07 PM, Martin Sebor wrote: On 05/19/2017 01:07 PM, Jason Merrill wrote: On Tue, May 16, 2017 at 5:39 PM, Martin Sebor wrote: On 05/16/2017 01:41 PM, Jason Merrill wrote: I'm still not convinced we need to consider standard-layout at all. I agree. The patch doesn't make use of is_standard_layout_p(). It defines its own helper called almost_std_layout_p() that combines trivial_type_p() with tests for non-public members, That's the part that seems unnecessary. Why do we care about non-public members? Because modifying them breaks encapsulation. Not if you're clearing/copying the object as a whole. If I take a legacy struct, make some of its members private, and define accessors and modifiers to manipulate those members and maintain invariants between them, I will want to check and adjust all code that changes objects of the struct in ways that might violate the invariants. For a trivial type, worrying about invariants doesn't make sense to me, since default-initialization won't establish any invariants. And bzero or memset(0) will have the same effect as value-initialization (if zero_init_p (type); we probably want to check that). If you're going to establish invariants, I would expect you to write a default constructor, which would make the class non-trivial. Thanks for the zero_init_p pointer! Let me add that to the patch along with Pedro's suggestion to use the current C++ terminology, retest and resubmit. In most cases you're right that defining the default constructor is the way to go. There is are a couple of use cases where a ctor tends to be avoided: when objects the class need to be initialized statically, and where they need to be PODs. GCC itself relies on the latter (e.g., some of the vec templates), apparently because it stores them in unions. It doesn't look tome like these vec templates maintain much of an invariant of any kind, but they easily could. An example of the static initialization case is an atomic class (not necessarily std::atomic though I think this would apply to it as well). Its objects usually need to be statically default- initializable (without running any ctors) but then they must be explicitly initialized by some sort of an init call to be made valid, and their state can only be accessed and modified via member functions (and so their state is private). Modifying them by any other means, including by memset or memcpy, is undefined. What common use case are you concerned about that isn't more appropriately expressed using the generated default or copy constructor or assignment operator? None; I am concerned about focusing the warning on code that is actually likely to be problematic. Hopefully the above explanation resolves that concern. If not, please let me know. Martin
Re: [PATCH] cfgcleanup: Ignore clobbers in bb_is_just_return
On 05/18/2017 02:58 PM, Segher Boessenkool wrote:
> The function bb_is_just_return finds if the BB it is asked about does
> just a return and nothing else. It currently does not allow clobbers
> in the block either, which we of course can allow just fine.
>
> This patch changes that. Bootstrapped and tested on powerpc64-linux
> {-m32,-m64} (and tested that the new test fails before the patch, too).
> Is this okay for trunk?
>
>
> Segher
>
>
> 2017-05-18 Segher Boessenkool
>
> * cfgcleanup.c (bb_is_just_return): Allow CLOBBERs.
>
> gcc/testsuite/
> * gcc.target/powerpc/conditional-return.c: New testcase.
OK.
jeff
Re: [PATCH] PR c++/77306 - Unable to specify visibility for explicit template instantiations
On Sat, Apr 22, 2017 at 4:46 PM, James Abbatiello wrote: > This is my first time attempting a contribution here so please point > out any mistakes. I've tested this on x86_64-pc-linux-gnu in a VM. Hello, It has been a few weeks. Can anybody give me some feedback here or tell me if I've done something wrong? https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00975.html -- James Abbatiello
Re: PR80806
On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote:
> Hi,
> The attached patch tries to fix PR80806 by warning when a variable is
> set using memset (and friends) but not used. I chose to warn in dse
> pass since dse would detect if the variable passed as 1st argument is
> a dead store. Does this approach look OK ?
>
> There were following fallouts observed during bootstrap build:
>
> * double-int.c (div_and_round_double):
> Warning emitted 'den' set but not used for following call to memset:
> memset (den, 0, sizeof den);
>
> I assume the warning is correct since there's immediately call to:
> encode (den, lden, hden);
>
> and encode overwrites all the contents of den.
> Should the above call to memset be removed from the source ?
Unsure. Yes, it's dead, but from a readability standpoint should it
stay? I don't know. This one isn't very clear.
>
> * tree-streamer.c (streamer_check_handled_ts_structures)
> The function defines a local array bool handled_p[LAST_TS_ENUM];
> and the warning is emitted for:
> memset (&handled_p, 0, sizeof (handled_p));
>
> That's because the function then initializes each element of the array
> handled_p to true
> making the memset call redundant.
> I am not sure if warning for the above case is a good idea ? The call
> to memset() seems deliberate, to initialize all elements to 0, and
> later assert checks if all the elements were explicitly set to true.
This one looks like it should stay to me. It's there for defensive
purposes to catch cases if programming errors.
>
> * value-prof.c (free_hist):
> Warns for the call to memset:
>
> static int
> free_hist (void **slot, void *data ATTRIBUTE_UNUSED)
> {
> histogram_value hist = *(histogram_value *) slot;
> free (hist->hvalue.counters);
> if (flag_checking)
> memset (hist, 0xab, sizeof (*hist));
> free (hist);
> return 1;
> }
>
> Assuming flag_checking was true, the call to memset would be dead
> anyway since it would be immediately freed ? Um, I don't understand
> the purpose of memset in the above function.
It's been like that from the day the code was introduced (initially
surrounded with an ENABLE_CHECKING. Given the call to free, the memset
is going to get removed.This one probably falls into the "should
just be removed" bucket.
>
> * testsuite fallout
> I verified regressing test-cases were not false positives and added
> -Wno-unused-but-set-variable.
I think the real question is do we want to warn here. I can certainly
see both sides of the issue.
jeff
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
On Sun, May 21, 2017 at 7:59 PM, Martin Sebor wrote: > On 05/19/2017 03:42 PM, Jason Merrill wrote: >> On Fri, May 19, 2017 at 4:07 PM, Martin Sebor wrote: >>> On 05/19/2017 01:07 PM, Jason Merrill wrote: On Tue, May 16, 2017 at 5:39 PM, Martin Sebor wrote: > On 05/16/2017 01:41 PM, Jason Merrill wrote: > >> I'm still not convinced we need to consider standard-layout at all. > > I agree. The patch doesn't make use of is_standard_layout_p(). > It defines its own helper called almost_std_layout_p() that > combines trivial_type_p() with tests for non-public members, That's the part that seems unnecessary. Why do we care about non-public members? >>> >>> >>> Because modifying them breaks encapsulation. >> >> >> Not if you're clearing/copying the object as a whole. >> >>> If I take a legacy struct, make some of its members private, >>> and define accessors and modifiers to manipulate those members >>> and maintain invariants between them, I will want to check and >>> adjust all code that changes objects of the struct in ways that >>> might violate the invariants. >> >> For a trivial type, worrying about invariants doesn't make sense to >> me, since default-initialization won't establish any invariants. And >> bzero or memset(0) will have the same effect as value-initialization >> (if zero_init_p (type); we probably want to check that). If you're >> going to establish invariants, I would expect you to write a default >> constructor, which would make the class non-trivial. > > Thanks for the zero_init_p pointer! Let me add that to the patch > along with Pedro's suggestion to use the current C++ terminology, > retest and resubmit. > > In most cases you're right that defining the default constructor > is the way to go. There is are a couple of use cases where a ctor > tends to be avoided: when objects the class need to be initialized > statically, and where they need to be PODs. GCC itself relies on > the latter (e.g., some of the vec templates), apparently because > it stores them in unions. It doesn't look tome like these vec > templates maintain much of an invariant of any kind, but they > easily could. > > An example of the static initialization case is an atomic class > (not necessarily std::atomic though I think this would apply to > it as well). Its objects usually need to be statically default- > initializable (without running any ctors) but then they must be > explicitly initialized by some sort of an init call to be made > valid, and their state can only be accessed and modified via > member functions (and so their state is private). Modifying > them by any other means, including by memset or memcpy, is > undefined. > >> >>> What common use case are you concerned about that isn't more >>> appropriately expressed using the generated default or copy >>> constructor or assignment operator? >> >> >> None; I am concerned about focusing the warning on code that is >> actually likely to be problematic. > > > Hopefully the above explanation resolves that concern. If not, > please let me know. I still think that we shouldn't warn about zeroing out a non-standard-layout object when value-initialization would produce the exact same result. If you want to keep the almost_std_layout_p check for the non-zero memset case, that's fine. Jason
Re: [PATCH 4/4] Set function alignment for M7 to 8 bytes.
> The gcc source tree is as desired, but I may have made > 2 inadvertent process mistakes: > * did not send a revised PATCH to gcc-patches That's OK since only a minor correction was requested. > * made the 4 changes with 1 commit That's as expected. -- Eric Botcazou
