Re: Re: typeof and operands in named address spaces
On Sun, Nov 15, 2020 at 11:53 AM Uecker, Martin wrote: > > > > On Wed, Nov 04, 2020 at 07:31:42PM +0100, Uros Bizjak wrote: > > > Hello! > > > > > > I was looking at the recent linux patch series [1] where segment > > > qualifiers (named address spaces) were introduced to handle percpu > > > variables. In the patch [2], the author mentions that: > > > > > > --q-- > > > Unfortunately, gcc does not provide a way to remove segment > > > qualifiers, which is needed to use typeof() to create local instances > > > of the per-cpu variable. For this reason, do not use the segment > > > qualifier for per-cpu variables, and do casting using the segment > > > qualifier instead. > > > --/q-- > > > > C in general does not provide means to strip qualifiers. We recently had > > a _lot_ of 'fun' trying to strip volatile from a type, see here: > > > > https://lore.kernel.org/lkml/875zimp0ay@mpe.ellerman.id.au > > > > which resulted in the current __unqual_scalar_typeof() hack. > > > > If we're going to do compiler extentions here, can we pretty please have > > a sane means of modifying qualifiers in general? > > Another way to drop qualifiers is using a cast. So you > can use typeof twice: > > typeof((typeof(_var))_var) tmp__; > > This also works for non-scalars but this is a GCC extension. > > > WG14 plans to standardize typeof. I would like to hear opinion > whether we should have typeof drop qualifiers or not. > > Currently, it does not do this on all compilers I tested > (except _Atomic on GCC) and there are also use cases for > keeping qualifiers. This is an argument for keeping qualifiers > should we standardize it, but then we need a way to drop > qualifiers. > > > lvalue conversion drops qualifers in C. In GCC, this is not > implemented correctly as it is unobvervable in standard C > (but it using typeof). > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702 > > A have a working patch in preparation to change this. Then you > could use > > typeof( ((void)0, x) ) > > to drop qualifiers. But this would then > also do array-to-pointer conversion. I am not sure > whether this is a problem. > > > For fun, I tried to come up with a standard+typeof-compliant > macro that drops qualifiers for all types without doing > array-to-pointer conversion > > https://github.com/uecker/unqual/blob/main/unqual.c > > but recursing into multi-dim. array types causes > a macro-explosion (but maybe multi-dim arrays are > also not too important) > > > Of course, we could also introduce a new feature for > dropping qualifiers. Thoughts? Just add a new qualifier that un-qualifies? _Unqual volatile T x; is T with volatile (evenually) removed. Or just a way to drop all using _Unqual? _Unqual T x; removing all qualifiers from T. Or add a special _Unqual_all to achieve that. I think removing a specific qualification is useful. Leaves cases like _Unqual volatile volatile T x; to be specified (that is ordering and cancellation of the unqual and qual variants of qualifiers). Richard. > > Best, > Martin > > > > _ > >
Re: Detect EAF flags in ipa-modref
On Nov 15 2020, Jan Hubicka wrote: >> See PR97840. > Thanks, > this is a false positive where we fail to discover that pointed-to type > is empty on non-x86_64 targets. This is triggered by better alias > analysis caused by non-escape discovery. > > While this is not a full fix (I hope someone with more experience on > C++ types and warnings will set up) I think it may be useful to avoid > warning on unused parameter. > > Bootstrapped/regtested x86_64-linux, OK? That doesn't fix anything. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: cache optimization through samping hardware event
On 11/10/20 8:03 AM, 172060045 wrote: Hi, Recently, I was interested in GCC AutoFDO optimization, which works by sampling specific PMU event on production machines and using those profiles to guide optimization. In this way, information such as cache miss can also be obtained through sampling, so can we implement feedback-directed cache optimization according to this idea? Hello. AutoFDO support in GCC is currently is quite bad shape. The following tool [1] is supposed to generate a reasonable .gcda file that GCC can consume. I know Bin spent some time working on that. Hopefully, he can tell what was his conclusion autoFDO? ARMv8.2 provides SPE features, which can obtain accurate LLC miss, TLB miss, branch miss and remote access information through perf, it may be helpful to the idea. Is any one doing relevant work?It would be grateful if someone could offer any advices, thx! I can guide you with generation of the .gcda profile files, but you will need to understand and transform a perf profile to a GCC gcda profile. Martin [1] https://github.com/google/autofdo
Re: Detect EAF flags in ipa-modref
> On Nov 15 2020, Jan Hubicka wrote:
>
> >> See PR97840.
> > Thanks,
> > this is a false positive where we fail to discover that pointed-to type
> > is empty on non-x86_64 targets. This is triggered by better alias
> > analysis caused by non-escape discovery.
> >
> > While this is not a full fix (I hope someone with more experience on
> > C++ types and warnings will set up) I think it may be useful to avoid
> > warning on unused parameter.
> >
> > Bootstrapped/regtested x86_64-linux, OK?
>
> That doesn't fix anything.
It does silence the warning if you remove inline from function
declaration (as creduce while minimizing the testcase - the minimized
testcase was undefined that is why I did not include it at the end).
I now implemented one by hand.
The reason is that gimple_call_arg_flags clears EAF_UNUSED
on symbols that !binds_to_current_def_p because we are worried that
symbol will be interposed by different version of the body with
same semantics that will actually read the arg.
This is bit paranoid check since we optimize things like *a == *a early
but with clang *a will trap if a==NULL.
Richi, I think we can add "safe" parameter to gimple_call_arg_flags and
bypass this logic when we use it for warnings only (having body that
does not use the value is quite strong hint that it is unused by the
function).
I played with bit more testcases and found that we also want to disable
warning for const functions and sometimes EAF_UNUSED flag is dropped
becaue of clobber that is not necessary to do. If function only clobber
the target it can be considered unused past inlining.
I am testing this improved patch and plan to commit if there are no
complains, but still we need to handle binds_to_current_def.
On the other direction, Martin, I think we may also warn for args
that are !EAF_UNUSED and !EAF_NOCLOBBER. This will catch some cases
where user did not add "const" specifier to the declaration but
parameter is detected to be readonly.
I also noticed that we do not detect EAF_UNUSED for fully unused
parameters (with no SSA names associated with them). I will fix that
incrementally.
Honza
PR middle-end/97840
* ipa-modref.c (analyze_ssa_name_flags): Skip clobbers if inlining
is done.
* tree-ssa-uninit.c (maybe_warn_pass_by_reference): Make stmt gcall;
skip const calls and unused arguments.
(warn_uninitialized_vars): Update prototype.
gcc/testsuite/ChangeLog:
2020-11-16 Jan Hubicka
* g++.dg/warn/unit-2.C: New test.
diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
index 4a43c50aa66..08fcc36a321 100644
--- a/gcc/ipa-modref.c
+++ b/gcc/ipa-modref.c
@@ -1333,7 +1331,14 @@ analyze_ssa_name_flags (tree name, vec
&known_flags, int depth)
/* Handle *name = exp. */
else if (assign
&& memory_access_to (gimple_assign_lhs (assign), name))
- flags &= ~(EAF_UNUSED | EAF_NOCLOBBER);
+ {
+ /* In general we can not ignore clobbers because they are
+barriers for code motion, however after inlining it is safe to
+do because local optimization passes do not consider clobbers
+from other functions. Similar logic is in ipa-pure-const.c.
*/
+ if (!cfun->after_inlining && !gimple_clobber_p (assign))
+ flags &= ~(EAF_UNUSED | EAF_NOCLOBBER);
+ }
/* ASM statements etc. */
else if (!assign)
{
diff --git a/gcc/testsuite/g++.dg/warn/unit-2.C
b/gcc/testsuite/g++.dg/warn/unit-2.C
new file mode 100644
index 000..30f3ceae191
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/unit-2.C
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wmaybe-uninitialized" } */
+struct a {int a;};
+__attribute__ ((noinline))
+void
+nowarn (const struct a *ptr)
+{
+ if (ptr)
+asm volatile ("");
+}
+void
+test()
+{
+ struct a ptr;
+ nowarn (&ptr);
+}
+__attribute__ ((noinline))
+int
+nowarn2 (const struct a *ptr, const struct a ptr2)
+{
+ return ptr != 0 || ptr2.a;
+}
+int mem;
+int
+test2()
+{
+ struct a ptr,ptr2={0};
+ return nowarn2 (&ptr, ptr2);
+}
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index f23514395e0..c94831bfb75 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -443,7 +443,7 @@ maybe_warn_operand (ao_ref &ref, gimple *stmt, tree lhs,
tree rhs,
access implying read access to those objects. */
static void
-maybe_warn_pass_by_reference (gimple *stmt, wlimits &wlims)
+maybe_warn_pass_by_reference (gcall *stmt, wlimits &wlims)
{
if (!wlims.wmaybe_uninit)
return;
@@ -457,6 +457,10 @@ maybe_warn_pass_by_reference (gimple *stmt, wlimits &wlims)
if (!fntype)
return;
+ /* Const function do not read their arguments. */
+ if (gimple_call_flags (stmt) & ECF_CONST)
+return;
+
const built_in_function fncode
= (fndecl && gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)
? DECL_FUNCTION_CODE (fndecl) :
Re: Re: typeof and operands in named address spaces
( restoring at least linux-toolcha...@vger.kernel.org, since that seems
to have gone missing )
On Mon, Nov 16, 2020 at 10:11:50AM +0100, Richard Biener wrote:
> On Sun, Nov 15, 2020 at 11:53 AM Uecker, Martin
> wrote:
> > > On Wed, Nov 04, 2020 at 07:31:42PM +0100, Uros Bizjak wrote:
> > > > Hello!
> > > >
> > > > I was looking at the recent linux patch series [1] where segment
> > > > qualifiers (named address spaces) were introduced to handle percpu
> > > > variables. In the patch [2], the author mentions that:
> > > >
> > > > --q--
> > > > Unfortunately, gcc does not provide a way to remove segment
> > > > qualifiers, which is needed to use typeof() to create local instances
> > > > of the per-cpu variable. For this reason, do not use the segment
> > > > qualifier for per-cpu variables, and do casting using the segment
> > > > qualifier instead.
> > > > --/q--
> > >
> > > C in general does not provide means to strip qualifiers. We recently had
> > > a _lot_ of 'fun' trying to strip volatile from a type, see here:
> > >
> > > https://lore.kernel.org/lkml/875zimp0ay@mpe.ellerman.id.au
> > >
> > > which resulted in the current __unqual_scalar_typeof() hack.
> > >
> > > If we're going to do compiler extentions here, can we pretty please have
> > > a sane means of modifying qualifiers in general?
> >
> > Another way to drop qualifiers is using a cast. So you
> > can use typeof twice:
> >
> > typeof((typeof(_var))_var) tmp__;
> >
> > This also works for non-scalars but this is a GCC extension.
> >
> >
> > WG14 plans to standardize typeof. I would like to hear opinion
> > whether we should have typeof drop qualifiers or not.
> >
> > Currently, it does not do this on all compilers I tested
> > (except _Atomic on GCC) and there are also use cases for
> > keeping qualifiers. This is an argument for keeping qualifiers
> > should we standardize it, but then we need a way to drop
> > qualifiers.
> >
> >
> > lvalue conversion drops qualifers in C. In GCC, this is not
> > implemented correctly as it is unobvervable in standard C
> > (but it using typeof).
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702
> >
> > A have a working patch in preparation to change this. Then you
> > could use
> >
> > typeof( ((void)0, x) )
Neat, that actually already works with clang. And I suppose we can use
the above GCC extention until such time as that GCC is fixed.
See below..
> > to drop qualifiers. But this would then
> > also do array-to-pointer conversion. I am not sure
> > whether this is a problem.
I don't _think_ so, but..
> > Of course, we could also introduce a new feature for
> > dropping qualifiers. Thoughts?
> Just add a new qualifier that un-qualifies?
>
> _Unqual volatile T x;
>
> is T with volatile (evenually) removed. Or just a way to drop
> all using _Unqual?
>
> _Unqual T x;
>
> removing all qualifiers from T. Or add a special _Unqual_all
> to achieve that. I think removing a specific qualification is
> useful. Leaves cases like
>
> _Unqual volatile volatile T x;
>
> to be specified (that is ordering and cancellation of the
> unqual and qual variants of qualifiers).
I rather like this, however I think I'd prefer the syntax be something
like:
_Unqual T x;
for removing all qualifiers, and:
_Unqual(volatile) volatile T X;
for stripping specific qualifiers. The syntax as proposed above seems
very error prone to me.
---
Subject: compiler: Improve __unqual_typeof()
Improve our __unqual_scalar_typeof() implementation by relying on C
dropping qualifiers for lvalue convesions. There is one small catch in
that GCC is currently known broken in this respect, however it happens
to have a C language extention that achieves the very same, it drops
qualifiers on casts.
This gets rid of the _Generic() usage and should improve compile times
(less preprocessor output) as well as increases the capabilities of the
macros.
XXX: I've only verified the below actually compiles, I've not verified
the generated code is actually 'correct'.
Suggested-by: "Uecker, Martin"
Signed-off-by: Peter Zijlstra (Intel)
---
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 74c6c0486eed..3c5cb52c12f9 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -156,3 +156,11 @@
#else
#define __diag_GCC_8(s)
#endif
+
+/*
+ * GCC has a bug where lvalue conversion doesn't drop qualifiers, use a GCC
+ * extention instead. GCC drops qualifiers on a cast, so use a double typeof().
+ *
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702
+ */
+#define __unqual_typeof(type) typeof( (typeof(type))type )
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index ac3fa37a84f9..4a6e2caab17b 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -250,27 +250,14 @@ struct ftrace_likely_data {
/* Are two types/vars the same type (ignoring qualifiers)? */
#define __same_type(a, b) __builtin_type
Re: `PatchSet' API mismatch causing a total `contrib/mklog.py' failure?
On Mon, 16 Nov 2020, Martin Liška wrote:
> > I have decided to give your `contrib/mklog.py' script a hit and, well,
> > ahem, I guess I must be doing something utterly silly, but no matter what
> > kind of a diff I hand to the script it does not produce anything unless I
> > apply a patch like below to it, in which case the output produced is as
> > expected.
>
> Hm, it should not be really needed. See:
>
> $ git show > 1
> $ ipython
> $ from unidiff import PatchSet
> $ PatchSet(open('1'))
> Out[2]: , gcc/doc/invoke.texi>]>
Hmm,
$ ipython
-bash: ipython: command not found
$
> > What's going on here -- has the API of `PatchSet' changed so much at one
> > point? Can we do anything to prevent someone else from tripping over this
> > issue?
>
> Can you please show how do you use the script and what's the output?
Sure, let's pick your recent change:
$ git show 2935ff7eb7ac | contrib/mklog.py
$
Nothing, as I wrote. With my tweaked version I instead get:
$ git show 2935ff7eb7ac | ../mklog.py
ChangeLog:
* gcc/testsuite/g++.dg/ubsan/pr61272.C:
$
which is not perfect as it failed to pick ChangeLog from gcc/testsuite/,
but at least it's a starting point.
Let's retry with `ipython' installed now:
$ git show 2935ff7eb7ac > 1
$ ipython
$ Python 2.7.16 (default, Oct 10 2019, 22:02:15)
Type "copyright", "credits" or "license" for more information.
IPython 5.8.0 -- An enhanced Interactive Python.
? -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help -> Python's own help system.
object? -> Details about 'object', use 'object??' for extra details.
In [1]: from unidiff import PatchSet
In [2]: PatchSet(open('1'))
Out[2]: ]>
Same with `ipython3' except for:
Python 3.7.3 (default, Jul 25 2020, 13:03:44)
Please note however that your script effectively does:
In [3]: PatchSet(open('1').read())
Out[3]:
which my tweak changes into your proposed sequence.
Maciej no less confused
Re: Re: typeof and operands in named address spaces
On Mon, Nov 16, 2020 at 12:10:56PM +0100, Peter Zijlstra wrote: > > > Another way to drop qualifiers is using a cast. So you > > > can use typeof twice: > > > > > > typeof((typeof(_var))_var) tmp__; > > > > > > This also works for non-scalars but this is a GCC extension. FWIW, clang seems to support this extension as well..
Re: `PatchSet' API mismatch causing a total `contrib/mklog.py' failure?
On 11/16/20 12:17 PM, Maciej W. Rozycki wrote:
On Mon, 16 Nov 2020, Martin Liška wrote:
I have decided to give your `contrib/mklog.py' script a hit and, well,
ahem, I guess I must be doing something utterly silly, but no matter what
kind of a diff I hand to the script it does not produce anything unless I
apply a patch like below to it, in which case the output produced is as
expected.
Hm, it should not be really needed. See:
$ git show > 1
$ ipython
$ from unidiff import PatchSet
$ PatchSet(open('1'))
Out[2]: , ]>
Hmm,
$ ipython
-bash: ipython: command not found
$
What's going on here -- has the API of `PatchSet' changed so much at one
point? Can we do anything to prevent someone else from tripping over this
issue?
Can you please show how do you use the script and what's the output?
Sure, let's pick your recent change:
$ git show 2935ff7eb7ac | contrib/mklog.py
$
Nothing, as I wrote. With my tweaked version I instead get:
$ git show 2935ff7eb7ac | ../mklog.py
ChangeLog:
* gcc/testsuite/g++.dg/ubsan/pr61272.C:
$
which is not perfect as it failed to pick ChangeLog from gcc/testsuite/,
but at least it's a starting point.
Hm, I see proper output:
$ git show 2935ff7eb7ac | contrib/mklog.py
gcc/testsuite/ChangeLog:
* g++.dg/ubsan/pr61272.C:
Let's retry with `ipython' installed now:
$ git show 2935ff7eb7ac > 1
$ ipython
$ Python 2.7.16 (default, Oct 10 2019, 22:02:15)
Type "copyright", "credits" or "license" for more information.
Please no python2, it's dead and buried really deep in the ground :)
IPython 5.8.0 -- An enhanced Interactive Python.
? -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help -> Python's own help system.
object? -> Details about 'object', use 'object??' for extra details.
In [1]: from unidiff import PatchSet
In [2]: PatchSet(open('1'))
Out[2]: ]>
Same with `ipython3' except for:
Python 3.7.3 (default, Jul 25 2020, 13:03:44)
What tells:
unidiff.VERSION
?
Please note however that your script effectively does:
In [3]: PatchSet(open('1').read())
Out[3]:
which my tweak changes into your proposed sequence.
So you're saying that python3 and your .read change fixes that?
Thanks,
Martin
Maciej no less confused
Re: `PatchSet' API mismatch causing a total `contrib/mklog.py' failure?
On Mon, 16 Nov 2020 at 11:51, Martin Liška wrote:
>
> On 11/16/20 12:17 PM, Maciej W. Rozycki wrote:
> > On Mon, 16 Nov 2020, Martin Liška wrote:
> >
> >>>I have decided to give your `contrib/mklog.py' script a hit and, well,
> >>> ahem, I guess I must be doing something utterly silly, but no matter what
> >>> kind of a diff I hand to the script it does not produce anything unless I
> >>> apply a patch like below to it, in which case the output produced is as
> >>> expected.
> >>
> >> Hm, it should not be really needed. See:
> >>
> >> $ git show > 1
> >> $ ipython
> >> $ from unidiff import PatchSet
> >> $ PatchSet(open('1'))
> >> Out[2]: , >> gcc/doc/invoke.texi>]>
> >
> > Hmm,
> >
> > $ ipython
> > -bash: ipython: command not found
> > $
> >
> >>>What's going on here -- has the API of `PatchSet' changed so much at
> >>> one
> >>> point? Can we do anything to prevent someone else from tripping over this
> >>> issue?
> >>
> >> Can you please show how do you use the script and what's the output?
> >
> > Sure, let's pick your recent change:
> >
> > $ git show 2935ff7eb7ac | contrib/mklog.py
> > $
> >
> > Nothing, as I wrote. With my tweaked version I instead get:
> >
> > $ git show 2935ff7eb7ac | ../mklog.py
> > ChangeLog:
> >
> > * gcc/testsuite/g++.dg/ubsan/pr61272.C:
> >
> > $
> >
> > which is not perfect as it failed to pick ChangeLog from gcc/testsuite/,
> > but at least it's a starting point.
>
> Hm, I see proper output:
>
> $ git show 2935ff7eb7ac | contrib/mklog.py
> gcc/testsuite/ChangeLog:
>
> * g++.dg/ubsan/pr61272.C:
FWIW I also get the expected output using Python 3.8.6 and unidiff 0.6.0.
Re: Re: typeof and operands in named address spaces
Am Montag, den 16.11.2020, 12:10 +0100 schrieb Peter Zijlstra: > ( restoring at least linux-toolcha...@vger.kernel.org, since that > seems > to have gone missing ) > > On Mon, Nov 16, 2020 at 10:11:50AM +0100, Richard Biener wrote: > > On Sun, Nov 15, 2020 at 11:53 AM Uecker, Martin > > wrote: > > > > On Wed, Nov 04, 2020 at 07:31:42PM +0100, Uros Bizjak wrote: > > > > > Hello! > > > > > > > > > > I was looking at the recent linux patch series [1] where > > > > > segment > > > > > qualifiers (named address spaces) were introduced to handle > > > > > percpu > > > > > variables. In the patch [2], the author mentions that: > > > > > > > > > > --q-- > > > > > Unfortunately, gcc does not provide a way to remove segment > > > > > qualifiers, which is needed to use typeof() to create local > > > > > instances > > > > > of the per-cpu variable. For this reason, do not use the > > > > > segment > > > > > qualifier for per-cpu variables, and do casting using the > > > > > segment > > > > > qualifier instead. > > > > > --/q-- > > > > > > > > C in general does not provide means to strip qualifiers. We > > > > recently had > > > > a _lot_ of 'fun' trying to strip volatile from a type, see > > > > here: > > > > > > > > > > > > https://lore.kernel.org/lkml/875zimp0ay@mpe.ellerman.id.au > > > > > > > > which resulted in the current __unqual_scalar_typeof() hack. > > > > > > > > If we're going to do compiler extentions here, can we pretty > > > > please have > > > > a sane means of modifying qualifiers in general? > > > > > > Another way to drop qualifiers is using a cast. So you > > > can use typeof twice: > > > > > > typeof((typeof(_var))_var) tmp__; > > > > > > This also works for non-scalars but this is a GCC extension. (That casts drop qualifiers is standard C. The extensions are 'typeof' and that casts can be used for non-scalar types.) > > > > > > WG14 plans to standardize typeof. I would like to hear opinion > > > whether we should have typeof drop qualifiers or not. > > > > > > Currently, it does not do this on all compilers I tested > > > (except _Atomic on GCC) and there are also use cases for > > > keeping qualifiers. This is an argument for keeping qualifiers > > > should we standardize it, but then we need a way to drop > > > qualifiers. > > > > > > > > > lvalue conversion drops qualifers in C. In GCC, this is not > > > implemented correctly as it is unobvervable in standard C > > > (but it using typeof). > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97702 > > > > > > A have a working patch in preparation to change this. Then you > > > could use > > > > > > typeof( ((void)0, x) ) > > Neat, that actually already works with clang. And I suppose we can > use > the above GCC extention until such time as that GCC is fixed. > > See below.. > > > > to drop qualifiers. But this would then > > > also do array-to-pointer conversion. I am not sure > > > whether this is a problem. > > I don't _think_ so, but.. > > > > Of course, we could also introduce a new feature for > > > dropping qualifiers. Thoughts? > > Just add a new qualifier that un-qualifies? > > > > _Unqual volatile T x; > > > > is T with volatile (evenually) removed. Or just a way to drop > > all using _Unqual? > > > > _Unqual T x; > > > > removing all qualifiers from T. Or add a special _Unqual_all > > to achieve that. I think removing a specific qualification is > > useful. Leaves cases like > > > > _Unqual volatile volatile T x; > > > > to be specified (that is ordering and cancellation of the > > unqual and qual variants of qualifiers). > > I rather like this, however I think I'd prefer the syntax be > something > like: > > _Unqual T x; > > for removing all qualifiers, and: > > _Unqual(volatile) volatile T X; > > for stripping specific qualifiers. The syntax as proposed above seems > very error prone to me. There is also the idea of adding 'auto' to C which would drop qualifiers. (__auto_type does not on GCC but on some other compilers) Best, Martin > > > --- > Subject: compiler: Improve __unqual_typeof() > > Improve our __unqual_scalar_typeof() implementation by relying on C > dropping qualifiers for lvalue convesions. There is one small catch > in > that GCC is currently known broken in this respect, however it > happens > to have a C language extention that achieves the very same, it drops > qualifiers on casts. > > This gets rid of the _Generic() usage and should improve compile > times > (less preprocessor output) as well as increases the capabilities of > the > macros. > > XXX: I've only verified the below actually compiles, I've not > verified > the generated code is actually 'correct'. > > Suggested-by: "Uecker, Martin" > Signed-off-by: Peter Zijlstra (Intel) > --- > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler- > gcc.h > index 74c6c0486eed..3c5cb52c12f9 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.
Re: Detect EAF flags in ipa-modref
On Mon, 16 Nov 2020, Jan Hubicka wrote:
> > On Nov 15 2020, Jan Hubicka wrote:
> >
> > >> See PR97840.
> > > Thanks,
> > > this is a false positive where we fail to discover that pointed-to type
> > > is empty on non-x86_64 targets. This is triggered by better alias
> > > analysis caused by non-escape discovery.
> > >
> > > While this is not a full fix (I hope someone with more experience on
> > > C++ types and warnings will set up) I think it may be useful to avoid
> > > warning on unused parameter.
> > >
> > > Bootstrapped/regtested x86_64-linux, OK?
> >
> > That doesn't fix anything.
>
> It does silence the warning if you remove inline from function
> declaration (as creduce while minimizing the testcase - the minimized
> testcase was undefined that is why I did not include it at the end).
> I now implemented one by hand.
>
> The reason is that gimple_call_arg_flags clears EAF_UNUSED
> on symbols that !binds_to_current_def_p because we are worried that
> symbol will be interposed by different version of the body with
> same semantics that will actually read the arg.
> This is bit paranoid check since we optimize things like *a == *a early
> but with clang *a will trap if a==NULL.
>
> Richi, I think we can add "safe" parameter to gimple_call_arg_flags and
> bypass this logic when we use it for warnings only (having body that
> does not use the value is quite strong hint that it is unused by the
> function).
Eh, please not.
>
> I played with bit more testcases and found that we also want to disable
> warning for const functions and sometimes EAF_UNUSED flag is dropped
> becaue of clobber that is not necessary to do. If function only clobber
> the target it can be considered unused past inlining.
>
> I am testing this improved patch and plan to commit if there are no
> complains, but still we need to handle binds_to_current_def.
>
> On the other direction, Martin, I think we may also warn for args
> that are !EAF_UNUSED and !EAF_NOCLOBBER. This will catch some cases
> where user did not add "const" specifier to the declaration but
> parameter is detected to be readonly.
>
> I also noticed that we do not detect EAF_UNUSED for fully unused
> parameters (with no SSA names associated with them). I will fix that
> incrementally.
Make sure to not apply it based on that reason to aggregates ;)
> Honza
>
> PR middle-end/97840
> * ipa-modref.c (analyze_ssa_name_flags): Skip clobbers if inlining
> is done.
> * tree-ssa-uninit.c (maybe_warn_pass_by_reference): Make stmt gcall;
> skip const calls and unused arguments.
> (warn_uninitialized_vars): Update prototype.
>
> gcc/testsuite/ChangeLog:
>
> 2020-11-16 Jan Hubicka
>
> * g++.dg/warn/unit-2.C: New test.
>
> diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
> index 4a43c50aa66..08fcc36a321 100644
> --- a/gcc/ipa-modref.c
> +++ b/gcc/ipa-modref.c
> @@ -1333,7 +1331,14 @@ analyze_ssa_name_flags (tree name, vec
> &known_flags, int depth)
> /* Handle *name = exp. */
> else if (assign
> && memory_access_to (gimple_assign_lhs (assign), name))
> - flags &= ~(EAF_UNUSED | EAF_NOCLOBBER);
> + {
> + /* In general we can not ignore clobbers because they are
> + barriers for code motion, however after inlining it is safe to
> + do because local optimization passes do not consider clobbers
> + from other functions. Similar logic is in ipa-pure-const.c.
> */
> + if (!cfun->after_inlining && !gimple_clobber_p (assign))
> + flags &= ~(EAF_UNUSED | EAF_NOCLOBBER);
> + }
> /* ASM statements etc. */
> else if (!assign)
> {
> diff --git a/gcc/testsuite/g++.dg/warn/unit-2.C
> b/gcc/testsuite/g++.dg/warn/unit-2.C
> new file mode 100644
> index 000..30f3ceae191
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/unit-2.C
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wmaybe-uninitialized" } */
> +struct a {int a;};
> +__attribute__ ((noinline))
> +void
> +nowarn (const struct a *ptr)
> +{
> + if (ptr)
> +asm volatile ("");
> +}
> +void
> +test()
> +{
> + struct a ptr;
> + nowarn (&ptr);
> +}
> +__attribute__ ((noinline))
> +int
> +nowarn2 (const struct a *ptr, const struct a ptr2)
> +{
> + return ptr != 0 || ptr2.a;
> +}
> +int mem;
> +int
> +test2()
> +{
> + struct a ptr,ptr2={0};
> + return nowarn2 (&ptr, ptr2);
> +}
> diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> index f23514395e0..c94831bfb75 100644
> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -443,7 +443,7 @@ maybe_warn_operand (ao_ref &ref, gimple *stmt, tree lhs,
> tree rhs,
> access implying read access to those objects. */
>
> static void
> -maybe_warn_pass_by_reference (gimple *stmt, wlimits &wlims)
> +maybe_warn_pass_by_reference (gcall *stmt, wlimits &wlims)
> {
>if (!wlims.wmaybe_uninit)
> return;
> @@ -457,6
Re: Detect EAF flags in ipa-modref
> > > > Richi, I think we can add "safe" parameter to gimple_call_arg_flags and > > bypass this logic when we use it for warnings only (having body that > > does not use the value is quite strong hint that it is unused by the > > function). > > Eh, please not. OK, I do not care that much as long as we do not have false positives everywhere :) Hadling EAF_UNUSED and CONST functions is necessary so we do not get false positive caused by us optimizing code out. In this case of not trusing EAF_UNUSED flag we will not optimize, so I do not really care. Martin, we collected very many warnings when building with configure --with-build-config=bootstrap-lto.mk This patch fixes some of them, but there are many others, can you take a look? For the testcase in PR I think it is enough to use default_is_empty_type to disable the warning, but it is not clear to me why the code uses default_is_empty_record at first place. > > > > > I played with bit more testcases and found that we also want to disable > > warning for const functions and sometimes EAF_UNUSED flag is dropped > > becaue of clobber that is not necessary to do. If function only clobber > > the target it can be considered unused past inlining. > > > > I am testing this improved patch and plan to commit if there are no > > complains, but still we need to handle binds_to_current_def. > > > > On the other direction, Martin, I think we may also warn for args > > that are !EAF_UNUSED and !EAF_NOCLOBBER. This will catch some cases > > where user did not add "const" specifier to the declaration but > > parameter is detected to be readonly. > > > > I also noticed that we do not detect EAF_UNUSED for fully unused > > parameters (with no SSA names associated with them). I will fix that > > incrementally. > > Make sure to not apply it based on that reason to aggregates ;) Sure, we already have detection of unused params in ipa-prop, so I guess we want is_giple_ref (parm) && !default_def to imply EAF_UNUSED. Honza
Re: `PatchSet' API mismatch causing a total `contrib/mklog.py' failure?
On Mon, 16 Nov 2020, Martin Liška wrote:
> > Let's retry with `ipython' installed now:
> >
> > $ git show 2935ff7eb7ac > 1
> > $ ipython
> > $ Python 2.7.16 (default, Oct 10 2019, 22:02:15)
> > Type "copyright", "credits" or "license" for more information.
>
> Please no python2, it's dead and buried really deep in the ground :)
Well, I only followed your instructions as you gave them. For Python 3
it's `ipython3' and `ipython' is Python 2 here. As I say, a standard
Debian distribution.
> > IPython 5.8.0 -- An enhanced Interactive Python.
> > ? -> Introduction and overview of IPython's features.
> > %quickref -> Quick reference.
> > help -> Python's own help system.
> > object? -> Details about 'object', use 'object??' for extra details.
> >
> > In [1]: from unidiff import PatchSet
> >
> > In [2]: PatchSet(open('1'))
> > Out[2]: ]>
> >
> > Same with `ipython3' except for:
> >
> > Python 3.7.3 (default, Jul 25 2020, 13:03:44)
>
> What tells:
> unidiff.VERSION
> ?
In [1]: from unidiff import PatchSet
In [2]: unidiff.VERSION
---
NameError Traceback (most recent call last)
in ()
> 1 unidiff.VERSION
NameError: name 'unidiff' is not defined
I noted however in my original message that the package version is 0.5.4
(or 0.5.2 for the other system).
> > Please note however that your script effectively does:
> >
> > In [3]: PatchSet(open('1').read())
> > Out[3]:
> >
> > which my tweak changes into your proposed sequence.
>
> So you're saying that python3 and your .read change fixes that?
It does.
Maciej
Re: `PatchSet' API mismatch causing a total `contrib/mklog.py' failure?
On Mon, 16 Nov 2020 at 12:49, Maciej W. Rozycki wrote:
>
> On Mon, 16 Nov 2020, Martin Liška wrote:
>
> > > Let's retry with `ipython' installed now:
> > >
> > > $ git show 2935ff7eb7ac > 1
> > > $ ipython
> > > $ Python 2.7.16 (default, Oct 10 2019, 22:02:15)
> > > Type "copyright", "credits" or "license" for more information.
> >
> > Please no python2, it's dead and buried really deep in the ground :)
>
> Well, I only followed your instructions as you gave them. For Python 3
> it's `ipython3' and `ipython' is Python 2 here. As I say, a standard
> Debian distribution.
ipython is irrelevant to the mklog.py script, which doesn't use it.
It's what Martin was using on the command-line, but is not actually
relevant to the issue at hand.
> > > IPython 5.8.0 -- An enhanced Interactive Python.
> > > ? -> Introduction and overview of IPython's features.
> > > %quickref -> Quick reference.
> > > help -> Python's own help system.
> > > object? -> Details about 'object', use 'object??' for extra details.
> > >
> > > In [1]: from unidiff import PatchSet
> > >
> > > In [2]: PatchSet(open('1'))
> > > Out[2]: ]>
> > >
> > > Same with `ipython3' except for:
> > >
> > > Python 3.7.3 (default, Jul 25 2020, 13:03:44)
> >
> > What tells:
> > unidiff.VERSION
> > ?
>
> In [1]: from unidiff import PatchSet
>
> In [2]: unidiff.VERSION
> ---
> NameError Traceback (most recent call last)
> in ()
> > 1 unidiff.VERSION
>
> NameError: name 'unidiff' is not defined
You only imported unidiff.PatchSet, not unidiff.VERSION. Either import
that, or the whole of unidiff.
Without ipython:
echo 'import unidiff ; print(unidiff.VERSION)' | python3
The contrib/mklog.py script requires Python 3, which doesn't seem
unreasonable in 2020. Its shebang makes that clear:
/usr/bin/env python3
Re: Re: typeof and operands in named address spaces
On Mon, Nov 16, 2020 at 12:23:17PM +, Uecker, Martin wrote: > > > > Another way to drop qualifiers is using a cast. So you > > > > can use typeof twice: > > > > > > > > typeof((typeof(_var))_var) tmp__; > > > > > > > > This also works for non-scalars but this is a GCC extension. > > (That casts drop qualifiers is standard C. The extensions > are 'typeof' and that casts can be used for non-scalar types.) Ah, I'll clarify. Thanks!
GCC 11.0.0 Status Report (2020-11-16), Stage 3 in effect now
Status == GCC trunk which eventually will become GCC 11 is now in Stage 3 which means open for general bugfixing. We have accumulated quite a number of regressions, a lot of the untriaged and eventually stale. Please help in cleaning up. Quality Data Priority # Change from last report --- --- P1 37 + 4 P2 257 + 1 P3 94 + 20 P4 184 - 1 P5 24 --- --- Total P1-P3 388 + 25 Total 596 + 24 Previous Report === https://gcc.gnu.org/pipermail/gcc/2020-October/234007.html
Re: `PatchSet' API mismatch causing a total `contrib/mklog.py' failure?
On Mon, 16 Nov 2020, Jonathan Wakely wrote: > ipython is irrelevant to the mklog.py script, which doesn't use it. > It's what Martin was using on the command-line, but is not actually > relevant to the issue at hand. I am aware of that, what's the point discussing this stuff? It wasn't me who started with ipython. > > In [1]: from unidiff import PatchSet > > > > In [2]: unidiff.VERSION > > --- > > NameError Traceback (most recent call last) > > in () > > > 1 unidiff.VERSION > > > > NameError: name 'unidiff' is not defined > > You only imported unidiff.PatchSet, not unidiff.VERSION. Either import > that, or the whole of unidiff. I wasn't told to. FWIW I don't intend to study Python programming on this occassion as I'm quite happy with the programming languages I have already learnt by now, including many flavours of assembly, thank you very much. I would rather leave Python-fu to other people. > Without ipython: > > echo 'import unidiff ; print(unidiff.VERSION)' | python3 Well: $ echo 'import unidiff ; print(unidiff.VERSION)' | python3 0.5.2 $ and $ echo 'import unidiff ; print(unidiff.VERSION)' | python3 0.5.4 $ respectively, but I wrote it twice already in previous messages, including the original one. I now feel like talking to a call centre. :( > The contrib/mklog.py script requires Python 3, which doesn't seem > unreasonable in 2020. Its shebang makes that clear: > /usr/bin/env python3 Again, what's the point discussing it? Let's keep to the facts and those are that the script does not work with my system which is not unreasonably old (released last Sep for unidiff 0.5.4; Jan 2019 for unidiff 0.5.2) or odd (a standard Debian distribution). Either the script needs to be fixed (and I did post a suggestion; maybe either syntax works with a later version of unidiff? -- as I say I am no Python expert), preferably, to support systems like mine out of the box, or to do whatever dance is required to report any unmet requirements so as not to confuse people. Thank you for your input anyway. Maciej
Re: `PatchSet' API mismatch causing a total `contrib/mklog.py' failure?
On 11/16/20 7:55 PM, Maciej W. Rozycki wrote: On Mon, 16 Nov 2020, Jonathan Wakely wrote: ipython is irrelevant to the mklog.py script, which doesn't use it. It's what Martin was using on the command-line, but is not actually relevant to the issue at hand. I am aware of that, what's the point discussing this stuff? It wasn't me who started with ipython. In [1]: from unidiff import PatchSet In [2]: unidiff.VERSION --- NameError Traceback (most recent call last) in () > 1 unidiff.VERSION NameError: name 'unidiff' is not defined You only imported unidiff.PatchSet, not unidiff.VERSION. Either import that, or the whole of unidiff. I wasn't told to. Hello. I've just pushed fix that supports unidiff 0.5.4. FWIW I don't intend to study Python programming on this occassion as I'm quite happy with the programming languages I have already learnt by now, including many flavours of assembly, thank you very much. I would rather leave Python-fu to other people. Without ipython: echo 'import unidiff ; print(unidiff.VERSION)' | python3 Well: $ echo 'import unidiff ; print(unidiff.VERSION)' | python3 0.5.2 $ and $ echo 'import unidiff ; print(unidiff.VERSION)' | python3 0.5.4 $ respectively, but I wrote it twice already in previous messages, including the original one. I now feel like talking to a call centre. :( The contrib/mklog.py script requires Python 3, which doesn't seem unreasonable in 2020. Its shebang makes that clear: /usr/bin/env python3 Again, what's the point discussing it? Let's keep to the facts and those are that the script does not work with my system which is not unreasonably old (released last Sep for unidiff 0.5.4; Jan 2019 for unidiff 0.5.2) or odd (a standard Debian distribution). No, 0.5.4 was released in May 2017 and 0.5.2 in February 2016 ([1]). That said, it's reasonable old. Either the script needs to be fixed (and I did post a suggestion; maybe either syntax works with a later version of unidiff? -- as I say I am no Python expert), preferably, to support systems like mine out of the box, or to do whatever dance is required to report any unmet requirements so as not to confuse people. Thank you for your input anyway. Maciej Martin [1] https://github.com/matiasb/python-unidiff/releases
Re: Detect EAF flags in ipa-modref
On 11/16/20 1:44 PM, Jan Hubicka wrote: Martin, we collected very many warnings when building with configure --with-build-config=bootstrap-lto.mk This patch fixes some of them, but there are many others, can you take a look? Hello. I guess you mean Martin Jambor, or me? Please CC :) Martin
Re: Detect EAF flags in ipa-modref
> On 11/16/20 1:44 PM, Jan Hubicka wrote: > > Martin, we collected very many warnings when building with > > configure --with-build-config=bootstrap-lto.mk > > This patch fixes some of them, but there are many others, can you take a > > look? > > Hello. > > I guess you mean Martin Jambor, or me? Please CC :) Martin Sebor, added to CC (since he did most of work on tree-ssa-uninit recently) I filled some PRs on the isues now. Honza > > Martin
Re: `PatchSet' API mismatch causing a total `contrib/mklog.py' failure?
On Mon, 16 Nov 2020, Martin Liška wrote: > I've just pushed fix that supports unidiff 0.5.4. Great, thanks! > > Again, what's the point discussing it? Let's keep to the facts and those > > are that the script does not work with my system which is not unreasonably > > old (released last Sep for unidiff 0.5.4; Jan 2019 for unidiff 0.5.2) or > > odd (a standard Debian distribution). > > No, 0.5.4 was released in May 2017 and 0.5.2 in February 2016 ([1]). > That said, it's reasonable old. Well, still used by a current distribution, and no 4.5 let alone 3 years is not a very long period in a software release lifecycle for a secondary component used with a stable development environment and not the point of said development. You need to combine the lifespan of both the distribution *and* all the components, every new version of which will have had to go through suitable verification and considered stable enough to be accepted into the distribution. [And yet zillions of bugs slip through verification and cause a major headache with any deployment of a new distribution version, to say nothing of random default configuration changes package authors and/or builders seem to make on a whim. I still haven't shaken out all the issues found in the installation I made last month for the effort I have recently been into, and the less time I spend on fixing the environment the more time I have available to actually do something productive, so obviously I am not going to upgrade more often than absolutely necessary.] Anyway, as I say, thank you for taking my fix. Maciej
Re: CSE deletes valid REG_EQUAL?
On 11/13/20 6:55 AM, Segher Boessenkool wrote: > Hi guys, > > On Thu, Nov 12, 2020 at 09:10:14PM -0700, Jeff Law wrote: >> On 11/12/20 7:02 PM, Xionghu Luo via Gcc wrote: >>> The output shows "REQ_EQUAL r118:DI+0x66546b64" is deleted by >>> df_remove_dead_eq_notes, >>> but r120:DI is not REG_DEAD here, so is it correct here to check insn use >>> and find that >>> r118:DI is dead then do the delete? >> It doesn't matter where the death occurs, any REG_DEAD note will cause >> the REG_EQUAL note to be removed. So given the death note for r118, >> then any REG_EQUAL note that references r118 will be removed. This is >> overly pessimistic as the note may still be valid/useful at some >> points. See >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92291 > The note even *always* is valid where it is! As you say, the REG_EQUAL > is only (necessarily) valid at *that* insn. And this insn *sets* the > reg (that is the only case when a REG_EQ* is allowed at all), so if the > eq reg is dead (i.e. unused) the whole insn is, and the insn can be > removed. > > Removing all REG_EQUAL notes for regs that become dead anywhere seems > to just be a thinko? All pseudos are dead somewhere! (Yeah okay, > infinite loops, but other than that.) Yea, but the code which wipes the notes probably has no sense of where in the RTL stream the note is valid and where it is not. So it does the fairly dumb thing and just ends up wiping them all away because as you noted, most pseudos have a death somewhere. One might argue that the code is OK as-is, but just needs to be run later. After cse2 would be the most logical location since CSE is probably the heaviest user of the notes. But I'd worry that the problems referenced in c#2 of bz51505 could crop up in other contexts than just combine. jeff
