Re: [PATCH] libcpp: Improve encapsulation of label_text
On Thu, 14 Jul 2022 at 22:31, David Malcolm wrote:
>
> On Thu, 2022-07-14 at 22:10 +0100, Jonathan Wakely wrote:
>
> Thanks for the patch.
>
> > I'm not sure if label_text::get () is the best name for the new
> > accessor. Other options include buffer () and c_str () but I don't
> > see a
> > compelling reason to prefer either of those.
>
> label_text::get should return a const char *, rather than a char *.
OK. That requires a tweak to pod_label_text, as it wants to store the
result of get() as a char*.
> I don't think anything uses label_text::is_owner; do we need that?
The pod_label_text constructor that transfers ownership from a
label_text uses it.
An alternative to adding the is_owner accessor is to make
pod_label_text a friend of label_text. Alternatively, move
pod_label_text into libcpp and make it label_text know how to convert
itself to a pod_label_text (setting the bool correctly). That might be
overkill given that pod_label_text is used exactly once in one place,
but then arguably so is the is_owner() accessor that's needed exactly
once.
>
> >
> > As a follow-up patch we could change label_text::m_buffer (and
> > pod_label_text::m_buffer) to be const char*, since those labels are
> > never modified once a label_text takes ownership of it. That would
> > make it clear to callers that they are not supposed to modify the
> > contents, and would remove the const_cast currently used by
> > label_text::borrow (), which is a code smell (returning a non-const
> > char* makes it look like it's OK to write into it, which is
> > definitely
> > not true for a borrowed string that was originally a string literal).
> > That would require using const_cast when passing the buffer to free
> > for
> > deallocation, but that's fine, and avoids the impression that the
> > object
> > holds a modifiable string buffer.
>
> I'm not sure I agree with your reasoning about the proposed follow-up,
You mean you don't want the member to be a const char*? Works for me.
My goal is to stop users of label_text modifying it, and now that they
can only get to it via the get() accessor, that's enforced by making
the accessor return const. Changing the member isn't necessary.
> but the patch below is OK for trunk, with the above nits fixed.
Thanks, pushed to trunk.
Here's the incremental diff between the reviewed patch and what I pushed:
diff --git a/gcc/diagnostic-show-locus.cc b/gcc/diagnostic-show-locus.cc
index bcaa52ec779..9d430b5189c 100644
--- a/gcc/diagnostic-show-locus.cc
+++ b/gcc/diagnostic-show-locus.cc
@@ -1877,7 +1877,8 @@ struct pod_label_text
{}
pod_label_text (label_text &&other)
- : m_buffer (other.get ()), m_caller_owned (other.is_owner ())
+ : m_buffer (const_cast (other.get ())),
+m_caller_owned (other.is_owner ())
{
other.release ();
}
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 193fec0a45f..9bdd5b9d30c 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1888,7 +1888,7 @@ public:
m_owned = false;
}
- char *get () const
+ const char *get () const
{
return m_buffer;
}
[COMMITTED] Use pp_vrange for ranges in dump_ssaname_info.
This changes the ad-hoc dumping of ranges in the gimple pretty printer
to use the pp_vrange utility function, which has the benefit of
handling all range types going forward and unifying the dumping code.
Instead of:
# RANGE [0, 51] NONZERO 0x3f
# RANGE ~[5, 10]
we would now get:
# RANGE [irange] long unsigned int [0, 51] NONZERO 0x3f
# RANGE [irange] int [-MIN, 4][11, MAX]
Tested on x86-64 Linux.
gcc/ChangeLog:
* gimple-pretty-print.cc (dump_ssaname_info): Use pp_vrange.
---
gcc/gimple-pretty-print.cc | 32
1 file changed, 4 insertions(+), 28 deletions(-)
diff --git a/gcc/gimple-pretty-print.cc b/gcc/gimple-pretty-print.cc
index ebd87b20a0a..f18baec438a 100644
--- a/gcc/gimple-pretty-print.cc
+++ b/gcc/gimple-pretty-print.cc
@@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. If not see
#include "ssa.h"
#include "cgraph.h"
#include "gimple-pretty-print.h"
+#include "value-range-pretty-print.h"
#include "internal-fn.h"
#include "tree-eh.h"
#include "gimple-iterator.h"
@@ -2335,35 +2336,10 @@ dump_ssaname_info (pretty_printer *buffer, tree node,
int spc)
if (!POINTER_TYPE_P (TREE_TYPE (node))
&& SSA_NAME_RANGE_INFO (node))
{
- wide_int min, max, nonzero_bits;
- value_range r;
-
+ Value_Range r (TREE_TYPE (node));
get_global_range_query ()->range_of_expr (r, node);
- value_range_kind range_type = r.kind ();
- if (!r.undefined_p ())
- {
- min = wi::to_wide (r.min ());
- max = wi::to_wide (r.max ());
- }
-
- // FIXME: Use irange::dump() instead.
- if (range_type == VR_VARYING)
- pp_printf (buffer, "# RANGE VR_VARYING");
- else if (range_type == VR_RANGE || range_type == VR_ANTI_RANGE)
- {
- pp_printf (buffer, "# RANGE ");
- pp_printf (buffer, "%s[", range_type == VR_RANGE ? "" : "~");
- pp_wide_int (buffer, min, TYPE_SIGN (TREE_TYPE (node)));
- pp_printf (buffer, ", ");
- pp_wide_int (buffer, max, TYPE_SIGN (TREE_TYPE (node)));
- pp_printf (buffer, "]");
- }
- nonzero_bits = get_nonzero_bits (node);
- if (nonzero_bits != -1)
- {
- pp_string (buffer, " NONZERO ");
- pp_wide_int (buffer, nonzero_bits, UNSIGNED);
- }
+ pp_string (buffer, "# RANGE ");
+ pp_vrange (buffer, &r);
newline_and_indent (buffer, spc);
}
}
--
2.36.1
[COMMITTED] Implement visitor pattern for vrange.
We frequently do operations on the various (upcoming) range types.
The cascading if/switch statements of is_a<> are getting annoying and
repetitive.
The classic visitor pattern provides a clean way to implement classes
handling various range types without the need for endless
conditionals. It also helps us keep polluting the vrange API with
functionality that should frankly live elsewhere.
In a follow-up patch I will add pretty printing facilities for vrange
and unify them with the dumping code. This is a prime candidate for
the pattern, as the code isn't performance sensitive. Other instances
(?? the dispatch code in range-ops ??) may still benefit from the hand
coded conditionals, since they elide vtables in favor of the
discriminator bit in vrange.
Tested on x86-64 Linux.
gcc/ChangeLog:
* value-range.cc (irange::accept): New.
(unsupported_range::accept): New.
* value-range.h (class vrange_visitor): New.
(class vrange): Add accept method.
(class unsupported_range): Same.
(class Value_Range): Same.
---
gcc/value-range.cc | 12
gcc/value-range.h | 11 +++
2 files changed, 23 insertions(+)
diff --git a/gcc/value-range.cc b/gcc/value-range.cc
index 528ed547ef3..8e6ec4cd740 100644
--- a/gcc/value-range.cc
+++ b/gcc/value-range.cc
@@ -30,6 +30,18 @@ along with GCC; see the file COPYING3. If not see
#include "fold-const.h"
#include "gimple-range.h"
+void
+irange::accept (const vrange_visitor &v) const
+{
+ v.visit (*this);
+}
+
+void
+unsupported_range::accept (const vrange_visitor &v) const
+{
+ v.visit (*this);
+}
+
// Convenience function only available for integers and pointers.
wide_int
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 0e341185f69..a7da8c5e900 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -73,6 +73,7 @@ class vrange
template friend bool is_a (vrange &);
friend class Value_Range;
public:
+ virtual void accept (const class vrange_visitor &v) const = 0;
virtual void set (tree, tree, value_range_kind = VR_RANGE);
virtual tree type () const;
virtual bool supports_type_p (tree type) const;
@@ -149,6 +150,7 @@ public:
// Misc methods.
virtual bool fits_p (const vrange &r) const override;
virtual void dump (FILE * = stderr) const override;
+ virtual void accept (const vrange_visitor &v) const override;
// Nonzero masks.
wide_int get_nonzero_bits () const;
@@ -251,6 +253,7 @@ class unsupported_range : public vrange
public:
unsupported_range ();
virtual void dump (FILE *) const override;
+ virtual void accept (const vrange_visitor &v) const override;
};
// is_a<> and as_a<> implementation for vrange.
@@ -298,6 +301,13 @@ is_a (vrange &v)
return v.m_discriminator == VR_IRANGE;
}
+class vrange_visitor
+{
+public:
+ virtual void visit (const irange &) const { }
+ virtual void visit (const unsupported_range &) const { }
+};
+
// This is a special int_range<1> with only one pair, plus
// VR_ANTI_RANGE magic to describe slightly more than can be described
// in one pair. It is described in the code as a "legacy range" (as
@@ -348,6 +358,7 @@ public:
bool zero_p () const { return m_vrange->zero_p (); }
wide_int lower_bound () const; // For irange/prange compatability.
wide_int upper_bound () const; // For irange/prange compatability.
+ void accept (const vrange_visitor &v) const { m_vrange->accept (v); }
private:
void init (tree type);
unsupported_range m_unsupported;
--
2.36.1
[COMMITTED] Convert vrange dumping facilities to pretty_printer.
We need to dump global ranges from the gimple pretty printer code, but
all the vrange dumping facilities work with FILE handles. This patch
converts all the dumping methods to work with pretty printers, and
provides a wrapper so the FILE * methods continue to work for
debugging. I also cleaned up the code a bit.
Tested on x86-64 Linux.
gcc/ChangeLog:
* Makefile.in (OBJS): Add value-range-pretty-print.o.
* pretty-print.h (pp_vrange): New.
* value-range.cc (vrange::dump): Call pp version.
(unsupported_range::dump): Move to its own file.
(dump_bound_with_infinite_markers): Same.
(irange::dump): Same.
(irange::dump_bitmasks): Same.
(vrange::debug): Remove.
* value-range.h: Remove virtual designation for dump methods.
Remove dump_bitmasks method.
* value-range-pretty-print.cc: New file.
* value-range-pretty-print.h: New file.
---
gcc/Makefile.in | 1 +
gcc/pretty-print.h | 7 ++
gcc/value-range-pretty-print.cc | 111 +++
gcc/value-range-pretty-print.h | 37 +++
gcc/value-range.cc | 113
gcc/value-range.h | 8 +--
6 files changed, 172 insertions(+), 105 deletions(-)
create mode 100644 gcc/value-range-pretty-print.cc
create mode 100644 gcc/value-range-pretty-print.h
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 3ae23702426..001506f8abf 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1711,6 +1711,7 @@ OBJS = \
value-query.o \
value-range.o \
value-range-equiv.o \
+ value-range-pretty-print.o \
value-range-storage.o \
value-relation.o \
value-prof.o \
diff --git a/gcc/pretty-print.h b/gcc/pretty-print.h
index fc588447460..d810e57bb14 100644
--- a/gcc/pretty-print.h
+++ b/gcc/pretty-print.h
@@ -340,6 +340,13 @@ pp_get_prefix (const pretty_printer *pp) { return
pp->prefix; }
pp_string (PP, pp_buffer (PP)->digit_buffer);\
} \
while (0)
+#define pp_vrange(PP, R) \
+ do \
+{ \
+ vrange_printer vrange_pp (PP); \
+ (R)->accept (vrange_pp); \
+} \
+ while (0)
#define pp_double(PP, F) pp_scalar (PP, "%f", F)
#define pp_pointer(PP, P) pp_scalar (PP, "%p", P)
diff --git a/gcc/value-range-pretty-print.cc b/gcc/value-range-pretty-print.cc
new file mode 100644
index 000..b795e92d8fb
--- /dev/null
+++ b/gcc/value-range-pretty-print.cc
@@ -0,0 +1,111 @@
+/* Pretty print support for value ranges.
+ Copyright (C) 2019-2022 Free Software Foundation, Inc.
+ Contributed by Aldy Hernandez .
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+
[PATCH] Add condition coverage profiling
This patch adds support in gcc+gcov for modified condition/decision
coverage (MC/DC) with the -fprofile-conditions flag. MC/DC is a type of
test/code coverage and it is particularly important in the avation and
automotive industries for safety-critical applications. MC/DC it is
required for or recommended by:
* DO-178C for the most critical software (Level A) in avionics
* IEC 61508 for SIL 4
* ISO 26262-6 for ASIL D
>From the SQLite webpage:
Two methods of measuring test coverage were described above:
"statement" and "branch" coverage. There are many other test
coverage metrics besides these two. Another popular metric is
"Modified Condition/Decision Coverage" or MC/DC. Wikipedia defines
MC/DC as follows:
* Each decision tries every possible outcome.
* Each condition in a decision takes on every possible outcome.
* Each entry and exit point is invoked.
* Each condition in a decision is shown to independently affect
the outcome of the decision.
In the C programming language where && and || are "short-circuit"
operators, MC/DC and branch coverage are very nearly the same thing.
The primary difference is in boolean vector tests. One can test for
any of several bits in bit-vector and still obtain 100% branch test
coverage even though the second element of MC/DC - the requirement
that each condition in a decision take on every possible outcome -
might not be satisfied.
https://sqlite.org/testing.html#mcdc
Wahlen, Heimdahl, and De Silva "Efficient Test Coverage Measurement for
MC/DC" describes an algorithm for adding instrumentation by carrying
over information from the AST, but my algorithm analyses the the control
flow graph to instrument for coverage. This has the benefit of being
programming language independent and faithful to compiler decisions
and transformations, although I have only tested it on constructs in C
and C++, see testsuite/gcc.misc-tests and testsuite/g++.dg.
Like Wahlen et al this implementation records coverage in fixed-size
bitsets which gcov knows how to interpret. This is very fast, but
introduces a limit on the number of terms in a single boolean
expression, the number of bits in a gcov_unsigned_type (which is
typedef'd to uint64_t), so for most practical purposes this would be
acceptable. This limitation is in the implementation and not the
algorithm, so support for more conditions can be added by also
introducing arbitrary-sized bitsets.
For space overhead, the instrumentation needs two accumulators
(gcov_unsigned_type) per condition in the program which will be written
to the gcov file. In addition, every function gets a pair of local
accumulators, but these accmulators are reused between conditions in the
same function.
For time overhead, there is a zeroing of the local accumulators for
every condition and one or two bitwise operation on every edge taken in
the an expression.
In action it looks pretty similar to the branch coverage. The -g short
opt carries no significance, but was chosen because it was an available
option with the upper-case free too.
gcov --conditions:
3: 17:void fn (int a, int b, int c, int d) {
3: 18:if ((a && (b || c)) && d)
decisions covered 3/8
condition 0 not covered (true false)
condition 1 not covered (true)
condition 2 not covered (true)
condition 3 not covered (true)
1: 19:x = 1;
-: 20:else
2: 21:x = 2;
3: 22:}
gcov --conditions --json-format:
"conditions": [
{
"not_covered_false": [
0
],
"count": 8,
"covered": 3,
"not_covered_true": [
0,
1,
2,
3
]
}
],
Some expressions, mostly those without else-blocks, are effectively
"rewritten" in the CFG construction making the algorithm unable to
distinguish them:
and.c:
if (a && b && c)
x = 1;
ifs.c:
if (a)
if (b)
if (c)
x = 1;
gcc will build the same graph for both these programs, and gcov will
report boths as 3-term expressions. It is vital that it is not
interpreted the other way around (which is consistent with the shape of
the graph) because otherwise the masking would be wrong for the and.c
program which is a more severe error. While surprising, users would
probably expect some minor rewriting of semantically-identical
expressions.
and.c.gcov:
#:2:if (a && b && c)
decisions covered 6/6
#:3:x = 1;
ifs.c.gcov:
#:2:if (a)
#:3:if (b)
#:4:if (c)
#:5:x = 1;
decisions covered 6/6
Adding else clauses alters the program (ifs.c can have 3 elses, and.c
only 1) and coverage becomes less surprising
ifs.c.gcov:
#:2:if (a)
decisions covered 2/2
Re: [PATCH] Add condition coverage profiling
On 15/07/2022 13:39, Jørgen Kvalsvik wrote: > This patch adds support in gcc+gcov for modified condition/decision > coverage (MC/DC) with the -fprofile-conditions flag. Hello, I have updated this patch based on the feedback from Sebastian and Martin. 1. The description of the masking_vector function has been updated. 2. New vocabulary for the output - decisions for, well, the decisions. It also writes at most one line per condition: decisions covered 1/4 condition 0 not covered (true false) condition 1 not covered (true) 3. I applied Sebastian's patch and so it should work free standing. Thanks, Jørgen
Re: [COMMITTED] Use pp_vrange for ranges in dump_ssaname_info.
On Fri, Jul 15, 2022 at 11:40 AM Aldy Hernandez wrote: > > This changes the ad-hoc dumping of ranges in the gimple pretty printer > to use the pp_vrange utility function, which has the benefit of > handling all range types going forward and unifying the dumping code. > > Instead of: > # RANGE [0, 51] NONZERO 0x3f > # RANGE ~[5, 10] > > we would now get: > > # RANGE [irange] long unsigned int [0, 51] NONZERO 0x3f > # RANGE [irange] int [-MIN, 4][11, MAX] BTW, these are the global ranges, that for some historical reason unknown to me, are visible with -fdump-tree-all-alias. Yes, alias :-). For example: : _1 = (long unsigned int) i_54; # RANGE [irange] long unsigned int [0, +INF] NONZERO 0xfffc _2 = _1 * 4; _3 = ia_60(D) + _2; _4 = *_3; _5 = (long unsigned int) i_54; # RANGE [irange] long unsigned int [0, +INF] NONZERO 0xfffc _6 = _5 * 4; ... ... The VRP twins, as well as other ranger clients, continue displaying things as they were before. Aldy
Re: [PATCH] HIGH part of symbol ref is invalid for constant pool
"Kewen.Lin" writes:
> Hi Jeff,
>
> Thanks for the patch, one question is inlined below.
>
> on 2022/7/4 14:58, Jiufu Guo wrote:
>> The high part of the symbol address is invalid for the constant pool. In
>> function rs6000_cannot_force_const_mem, we already return true for
>> "HIGH with UNSPEC" rtx. During debug GCC, I found that
>> rs6000_cannot_force_const_mem is called for some other HIGH code rtx
>> expressions which also indicate the high part of a symbol_ref.
>> For example:
>> (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc])
>> (high:DI (symbol_ref:DI ("var_1")..)))
>>
>> In the below case, this kind of rtx could occur in the middle of
>> optimizations
>> pass but was not dumped to a file. So, no test case is attached to this
>> patch.
>>
>
> Could you help to expand this more on how it affects some tree-optimization
> pass?
> I guess some tree-opt will expand gimple expression to rtx, evaluate the cost
> or similar and make some decision basing on it. If that is the case, you
> probably
> can construct one test case to show that: without this patch, the evaluated
> cost
> or similar looks off, the optimization decision is sub-optimal; with this
> patch,
> the optimization result is expected.
Hi Kewen,
Thanks a lot for your comments!
>From my investigations, I find some cases for which
rs6000_cannot_force_const_mem is called on "HIGH code" rtx which is not
UNSPEC sub-code. The code "decSetCoeff" is an example.
In the middle of "combine" pass, function "recog_for_combine" invokes
"force_const_mem", and the invoking could fail.
src = force_const_mem (mode, src);
if (src)
{
SUBST (SET_SRC (pat), src);
changed = true;
}
Here, the rtx "(high:DI (unspec:DI [(symbol_ref" is the argument for
the example code "decSetCoeff".
I also tried to see if other passes(GIMPLE) could hit this kind cases,
but did not find.
BR,
Jiufu(Jeff)
>
> BR,
> Kewen
>
>
>> extern const unsigned int __decPOWERS[10];
>> void
>> decSetCoeff (int *residue, const unsigned int *up)
>> {
>> unsigned int half = (unsigned int) __decPOWERS1[3] >> 1;
>>
>> if (*up >= half)
>> *residue = 7;
>>
>> return;
>> }
>>
>> This patch updates rs6000_cannot_force_const_mem to return true for
>> rtx with HIGH code.
>>
>>
>> Bootstrapped and regtested on ppc64le and ppc64.
>> Is it ok for trunk?
>>
>> BR,
>> Jiufu Guo
>>
>>
>> gcc/ChangeLog:
>>
>> * config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem):
>> Return true for HIGH code rtx.
>>
>> ---
>> gcc/config/rs6000/rs6000.cc | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>> index 3ff16b8ae04..c2b10669627 100644
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -9707,8 +9707,11 @@ rs6000_init_stack_protect_guard (void)
>> static bool
>> rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
>> {
>> - if (GET_CODE (x) == HIGH
>> - && GET_CODE (XEXP (x, 0)) == UNSPEC)
>> + /* High part of a symbol ref/address can not be put into constant pool.
>> e.g.
>> + (high:DI (symbol_ref:DI ("var")..)) or
>> + (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..)
>> + (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12.
>> */
>> + if (GET_CODE (x) == HIGH)
>> return true;
>>
>>/* A TLS symbol in the TOC cannot contain a sum. */
[x86 PATCH] PR target/106273: Add earlyclobber to *andn3_doubleword_bmi
This patch resolves PR target/106273 which is a wrong code regression
caused by the recent reorganization to split doubleword operations after
reload on x86. For the failing test case, the constraints on the
andnti3_doubleword_bmi pattern allow reload to allocate the output and
operand in overlapping but non-identical registers, i.e.
(insn 45 44 66 2 (parallel [
(set (reg/v:TI 5 di [orig:96 i ] [96])
(and:TI (not:TI (reg:TI 39 r11 [orig:83 _2 ] [83]))
(reg/v:TI 4 si [orig:100 i ] [100])))
(clobber (reg:CC 17 flags))
]) "pr106273.c":13:5 562 {*andnti3_doubleword_bmi}
where the output is in registers 5 and 6, and the second operand is
registers 4 and 5, which then leads to the incorrect split:
(insn 113 44 114 2 (parallel [
(set (reg:DI 5 di [orig:96 i ] [96])
(and:DI (not:DI (reg:DI 39 r11 [orig:83 _2 ] [83]))
(reg:DI 4 si [orig:100 i ] [100])))
(clobber (reg:CC 17 flags))
]) "pr106273.c":13:5 566 {*andndi_1}
(insn 114 113 66 2 (parallel [
(set (reg:DI 6 bp [ i+8 ])
(and:DI (not:DI (reg:DI 40 r12 [ _2+8 ]))
(reg:DI 5 di [ i+8 ])))
(clobber (reg:CC 17 flags))
]) "pr106273.c":13:5 566 {*andndi_1}
[Notice that reg:DI 5 is set in the first instruction, but assumed
to have its original value in the second]. My first thought was
that this could be fixed by swapping the order of the split instructions
(which works in this case), but in the general case, it's impossible
to handle (set (reg:TI x) (op (reg:TI x+1) (reg:TI x-1)). Hence for
correctness this pattern needs an earlyclobber "=&r", but we can also
allow cases where the output is the same as one of the operands (using
constraint "0"). The other binary logic operations (AND, IOR, XOR)
are unaffected as they constrain the output to match the first
operand, but BMI's andn is a three-operand instruction which can
lead to the overlapping cases described above.
This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
and make -k check, both with and without --target_board=unix{-m32} with
no new failures. Ok for mainline?
2022-07-15 Roger Sayle
gcc/ChangeLog
PR target/106273
* config/i386/i386.md (*andn3_doubleword_bmi): Update the
constraints to reflect the output is earlyclobber, unless it is
the same register (pair) as one of the operands.
gcc/testsuite/ChangeLog
PR target/106273
* gcc.target/i386/pr106273.c: New test case.
Thanks again, and sorry for the inconvenience.
Roger
--
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 3b02d0c..585b2d5 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -10423,10 +10423,10 @@
})
(define_insn_and_split "*andn3_doubleword_bmi"
- [(set (match_operand: 0 "register_operand" "=r")
+ [(set (match_operand: 0 "register_operand" "=&r,r,r")
(and:
- (not: (match_operand: 1 "register_operand" "r"))
- (match_operand: 2 "nonimmediate_operand" "ro")))
+ (not: (match_operand: 1 "register_operand" "r,0,r"))
+ (match_operand: 2 "nonimmediate_operand" "ro,ro,0")))
(clobber (reg:CC FLAGS_REG))]
"TARGET_BMI"
"#"
diff --git a/gcc/testsuite/gcc.target/i386/pr106273.c
b/gcc/testsuite/gcc.target/i386/pr106273.c
new file mode 100644
index 000..8c2fbbb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr106273.c
@@ -0,0 +1,27 @@
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-Og -march=cascadelake" } */
+typedef unsigned char u8;
+typedef unsigned short u16;
+typedef unsigned long long u64;
+
+u8 g;
+
+void
+foo (__int128 i, u8 *r)
+{
+ u16 a = __builtin_sub_overflow_p (0, i * g, 0);
+ i ^= g & i;
+ u64 s = (i >> 64) + i;
+ *r = ((union { u16 a; u8 b[2]; }) a).b[1] + s;
+}
+
+int
+main (void)
+{
+ u8 x;
+ foo (5, &x);
+ if (x != 5)
+__builtin_abort ();
+ return 0;
+}
+/* { dg-final { scan-assembler-not "andn\[ \\t\]+%rdi, %r11, %rdi" } } */
Re: [PATCH] Add condition coverage profiling
On 15.07.22 13:47, Jørgen Kvalsvik via Gcc-patches wrote: 2. New vocabulary for the output - decisions for, well, the decisions. It also writes at most one line per condition: decisions covered 1/4 condition 0 not covered (true false) condition 1 not covered (true) Do we really have multiple decisions? I think we have only one decision composed of conditions and zero or more boolean operators. We have variants of condition outcomes. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/
Re: [PATCH] Add condition coverage profiling
On 15/07/2022 15:31, Sebastian Huber wrote: > On 15.07.22 13:47, Jørgen Kvalsvik via Gcc-patches wrote: >> 2. New vocabulary for the output - decisions for, well, the decisions. It >> also >> writes at most one line per condition: >> >> decisions covered 1/4 >> condition 0 not covered (true false) >> condition 1 not covered (true) > > Do we really have multiple decisions? I think we have only one decision > composed > of conditions and zero or more boolean operators. We have variants of > condition > outcomes. > Maybe, I'm not sure - you could argue that since a fixture of boolean "dominates" the outcome (either by short circuiting subsequent terms or masking preceding terms) then that fixture is a decision which leads to one of two outcomes. The other parameters may change but they wouldn't change the decision. You have 2^N inputs but only N+1 decisions. Personally the "variants" phrasing doesn't feel right to me. That being said I'm open to making this whatever the maintainers feel is appropriate.
Re: ICE after folding svld1rq to vec_perm_expr duing forwprop
On Thu, 14 Jul 2022 at 17:22, Richard Sandiford
wrote:
>
> Richard Biener writes:
> > On Thu, Jul 14, 2022 at 9:55 AM Prathamesh Kulkarni
> > wrote:
> >>
> >> On Wed, 13 Jul 2022 at 12:22, Richard Biener
> >> wrote:
> >> >
> >> > On Tue, Jul 12, 2022 at 9:12 PM Prathamesh Kulkarni via Gcc-patches
> >> > wrote:
> >> > >
> >> > > Hi Richard,
> >> > > For the following test:
> >> > >
> >> > > svint32_t f2(int a, int b, int c, int d)
> >> > > {
> >> > > int32x4_t v = (int32x4_t) {a, b, c, d};
> >> > > return svld1rq_s32 (svptrue_b8 (), &v[0]);
> >> > > }
> >> > >
> >> > > The compiler emits following ICE with -O3 -mcpu=generic+sve:
> >> > > foo.c: In function ‘f2’:
> >> > > foo.c:4:11: error: non-trivial conversion in ‘view_convert_expr’
> >> > > 4 | svint32_t f2(int a, int b, int c, int d)
> >> > > | ^~
> >> > > svint32_t
> >> > > __Int32x4_t
> >> > > _7 = VIEW_CONVERT_EXPR<__Int32x4_t>(_8);
> >> > > during GIMPLE pass: forwprop
> >> > > dump file: foo.c.109t.forwprop2
> >> > > foo.c:4:11: internal compiler error: verify_gimple failed
> >> > > 0xfda04a verify_gimple_in_cfg(function*, bool)
> >> > > ../../gcc/gcc/tree-cfg.cc:5568
> >> > > 0xe9371f execute_function_todo
> >> > > ../../gcc/gcc/passes.cc:2091
> >> > > 0xe93ccb execute_todo
> >> > > ../../gcc/gcc/passes.cc:2145
> >> > >
> >> > > This happens because, after folding svld1rq_s32 to vec_perm_expr, we
> >> > > have:
> >> > > int32x4_t v;
> >> > > __Int32x4_t _1;
> >> > > svint32_t _9;
> >> > > vector(4) int _11;
> >> > >
> >> > >:
> >> > > _1 = {a_3(D), b_4(D), c_5(D), d_6(D)};
> >> > > v_12 = _1;
> >> > > _11 = v_12;
> >> > > _9 = VEC_PERM_EXPR <_11, _11, { 0, 1, 2, 3, ... }>;
> >> > > return _9;
> >> > >
> >> > > During forwprop, simplify_permutation simplifies vec_perm_expr to
> >> > > view_convert_expr,
> >> > > and the end result becomes:
> >> > > svint32_t _7;
> >> > > __Int32x4_t _8;
> >> > >
> >> > > ;; basic block 2, loop depth 0
> >> > > ;;pred: ENTRY
> >> > > _8 = {a_2(D), b_3(D), c_4(D), d_5(D)};
> >> > > _7 = VIEW_CONVERT_EXPR<__Int32x4_t>(_8);
> >> > > return _7;
> >> > > ;;succ: EXIT
> >> > >
> >> > > which causes the error duing verify_gimple since VIEW_CONVERT_EXPR
> >> > > has incompatible types (svint32_t, int32x4_t).
> >> > >
> >> > > The attached patch disables simplification of VEC_PERM_EXPR
> >> > > in simplify_permutation, if lhs and rhs have non compatible types,
> >> > > which resolves ICE, but am not sure if it's the correct approach ?
> >> >
> >> > It for sure papers over the issue. I think the error happens earlier,
> >> > the V_C_E should have been built with the type of the VEC_PERM_EXPR
> >> > which is the type of the LHS. But then you probably run into the
> >> > different sizes ICE (VLA vs constant size). I think for this case you
> >> > want a BIT_FIELD_REF instead of a VIEW_CONVERT_EXPR,
> >> > selecting the "low" part of the VLA vector.
> >> Hi Richard,
> >> Sorry I don't quite follow. In this case, we use VEC_PERM_EXPR to
> >> represent dup operation
> >> from fixed width to VLA vector. I am not sure how folding it to
> >> BIT_FIELD_REF will work.
> >> Could you please elaborate ?
> >>
> >> Also, the issue doesn't seem restricted to this case.
> >> The following test case also ICE's during forwprop:
> >> svint32_t foo()
> >> {
> >> int32x4_t v = (int32x4_t) {1, 2, 3, 4};
> >> svint32_t v2 = svld1rq_s32 (svptrue_b8 (), &v[0]);
> >> return v2;
> >> }
> >>
> >> foo2.c: In function ‘foo’:
> >> foo2.c:9:1: error: non-trivial conversion in ‘vector_cst’
> >> 9 | }
> >> | ^
> >> svint32_t
> >> int32x4_t
> >> v2_4 = { 1, 2, 3, 4 };
> >>
> >> because simplify_permutation folds
> >> VEC_PERM_EXPR< {1, 2, 3, 4}, {1, 2, 3, 4}, {0, 1, 2, 3, ...} >
> >> into:
> >> vector_cst {1, 2, 3, 4}
> >>
> >> and it complains during verify_gimple_assign_single because we don't
> >> support assignment of vector_cst to VLA vector.
> >>
> >> I guess the issue really is that currently, only VEC_PERM_EXPR
> >> supports lhs and rhs
> >> to have vector types with differing lengths, and simplifying it to
> >> other tree codes, like above,
> >> will result in type errors ?
> >
> > That might be the case - Richard should know.
>
> I don't see anything particularly special about VEC_PERM_EXPR here,
> or about the VLA vs. VLS thing. We would have the same issue trying
> to build a 128-bit vector from 2 64-bit vectors. And there are other
> tree codes whose input types are/can be different from their output
> types.
>
> So it just seems like a normal type correctness issue: a VEC_PERM_EXPR
> of type T needs to be replaced by something of type T. Whether T has a
> constant size or a variable size doesn't matter.
>
> > If so your type check
> > is still too late, you should instead recognize that we are permuting
> > a VLA vector and then refuse to go any of the non-VEC_PERM generating
> > paths - that probably means just allow
[committed] MAINTAINERS: Add myself to Write After Approval
ChangeLog: * MAINTAINERS: Add myself to Write After Approval. diff --git a/MAINTAINERS b/MAINTAINERS index 7d9aab76dd9676c806bd08abc7542553fcf81928..7a7ad42ced3027f1f7970916b355fd5fc7b0088c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -352,6 +352,7 @@ Kevin Buettner Andrew Burgess Adam Butcher Andrew Cagney +Andrew Carlotti Daniel Carrera Stephane Carrez Gabriel Charette
Re: kernel sparse annotations vs. compiler attributes and debug_annotate_{type,decl} WAS: Re: [PATCH 0/9] Add debug_annotate attributes
> On 7/14/22 8:09 AM, Jose E. Marchesi wrote: >> Hi Yonghong. >> >>> On 7/7/22 1:24 PM, Jose E. Marchesi wrote: Hi Yonghong. > On 6/21/22 9:12 AM, Jose E. Marchesi wrote: >> >>> On 6/17/22 10:18 AM, Jose E. Marchesi wrote: Hi Yonghong. > On 6/15/22 1:57 PM, David Faust wrote: >> >> On 6/14/22 22:53, Yonghong Song wrote: >>> >>> >>> On 6/7/22 2:43 PM, David Faust wrote: Hello, This patch series adds support for: - Two new C-language-level attributes that allow to associate (to "annotate" or to "tag") particular declarations and types with arbitrary strings. As explained below, this is intended to be used to, for example, characterize certain pointer types. - The conveyance of that information in the DWARF output in the form of a new DIE: DW_TAG_GNU_annotation. - The conveyance of that information in the BTF output in the form of two new kinds of BTF objects: BTF_KIND_DECL_TAG and BTF_KIND_TYPE_TAG. All of these facilities are being added to the eBPF ecosystem, and support for them exists in some form in LLVM. Purpose === 1) Addition of C-family language constructs (attributes) to specify free-text tags on certain language elements, such as struct fields. The purpose of these annotations is to provide additional information about types, variables, and function parameters of interest to the kernel. A driving use case is to tag pointer types within the linux kernel and eBPF programs with additional semantic information, such as '__user' or '__rcu'. For example, consider the linux kernel function do_execve with the following declaration: static int do_execve(struct filename *filename, const char __user *const __user *__argv, const char __user *const __user *__envp); Here, __user could be defined with these annotations to record semantic information about the pointer parameters (e.g., they are user-provided) in DWARF and BTF information. Other kernel facilites such as the eBPF verifier can read the tags and make use of the information. 2) Conveying the tags in the generated DWARF debug info. The main motivation for emitting the tags in DWARF is that the Linux kernel generates its BTF information via pahole, using DWARF as a source: ++ BTF BTF +--+ | pahole |---> vmlinux.btf --->| verifier | ++ +--+ ^^ || DWARF |BTF | || vmlinux +-+ module1.ko | BPF program | module2.ko +-+ ... This is because: a) Unlike GCC, LLVM will only generate BTF for BPF programs. b) GCC can generate BTF for whatever target with -gbtf, but there is no support for linking/deduplicating BTF in the linker. In the scenario above, the verifier needs access to the pointer tags of both the kernel types/declarations (conveyed in the DWARF and translated to BTF by pahole) and those of the BPF program (available directly in BTF). Another motivation for having the tag information in DWARF, unrelated to BPF and
Re: [PATCH v2 1/2] aarch64: Don't return invalid GIMPLE assign statements
On Wed, Jul 13, 2022 at 02:32:16PM +0200, Richard Biener wrote:
> On Wed, Jul 13, 2022 at 12:50 PM Andrew Carlotti
> wrote:
> > I specifically wanted to avoid not folding the call, because always
> > folding means that the builtin doesn't need to be implemented anywhere
> > else (which isn't relevant here, but may become relevant when folding
> > newly defined builtins in the future).
> >
> > I considered dropping the statement, but I wasn't sure at the time that
> > I could do it safely. I could send a patch to instead replace new_stmt
> > with a GIMPLE_NOP.
>
> If you can be sure there's no side-effect on the RHS then I think
> I'd prefer that over allocating an SSA name for something that's
> going to be DCEd anyway.
>
> Richard.
I discussed this off-list with Richard Sandiford, and we agreed that it
would be better to leave this code as it is.
The only time this form is likely to arise is if the statement has
side-effects (e.g. reading from volatile memory or triggering
floating-point exceptions), in which case we can't just replace it with
a nop. On the other hand, in the event that someone has written an
entirely redundant statement, then it will quickly get eliminated
anyway.
Adding code to distinguish between the two cases here, or to handle
the hard case, is unnecessary and wouldn't be worthwhile.
> > > >> gcc/ChangeLog:
> > > >>
> > > >> * config/aarch64/aarch64-builtins.cc
> > > >> (aarch64_general_gimple_fold_builtin): Add fixup for invalid GIMPLE.
> > > >>
> > > >> gcc/testsuite/ChangeLog:
> > > >>
> > > >> * gcc.target/aarch64/advsimd-intrinsics/ignored_return_1.c: New test.
> > > >>
> > > >> ---
> > > >>
> > > >> diff --git a/gcc/config/aarch64/aarch64-builtins.cc
> > > >> b/gcc/config/aarch64/aarch64-builtins.cc
> > > >> index
> > > >> e0a741ac663188713e21f457affa57217d074783..5753988a9964967c27a03aca5fddb9025fd8ed6e
> > > >> 100644
> > > >> --- a/gcc/config/aarch64/aarch64-builtins.cc
> > > >> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> > > >> @@ -3022,6 +3022,16 @@ aarch64_general_gimple_fold_builtin (unsigned
> > > >> int fcode, gcall *stmt,
> > > >> default:
> > > >>break;
> > > >> }
> > > >> +
> > > >> + /* GIMPLE assign statements (unlike calls) require a non-null lhs.
> > > >> If we
> > > >> + created an assign statement with a null lhs, then fix this by
> > > >> assigning
> > > >> + to a new (and subsequently unused) variable. */
> > > >> + if (new_stmt && is_gimple_assign (new_stmt) && !gimple_assign_lhs
> > > >> (new_stmt))
> > > >> +{
> > > >> + tree new_lhs = make_ssa_name (gimple_call_return_type (stmt));
> > > >> + gimple_assign_set_lhs (new_stmt, new_lhs);
> > > >> +}
> > > >> +
> > > >>return new_stmt;
> > > >> }
> > > >>
> > > >> diff --git
> > > >> a/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/ignored_return_1.c
> > > >>
> > > >> b/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/ignored_return_1.c
> > > >> new file mode 100644
> > > >> index
> > > >> ..345307456b175307f5cb22de5e59cfc6254f2737
> > > >> --- /dev/null
> > > >> +++
> > > >> b/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/ignored_return_1.c
> > > >> @@ -0,0 +1,9 @@
> > > >> +/* { dg-do compile { target { aarch64*-*-* } } } */
> > > >> +
> > > >> +#include
> > > >> +
> > > >> +int8_t *bar();
> > > >> +
> > > >> +void foo() {
> > > >> + __builtin_aarch64_ld1v16qi(bar());
> > > >> +}
[PATCH] aarch64: Replace manual swapping idiom with std::swap in aarch64.cc
Replace manual swapping idiom with std::swap in aarch64.cc
gcc/config/aarch64/aarch64.cc has a few manual swapping idioms of the form:
x = in0, in0 = in1, in1 = x;
The preferred way is using the standard:
std::swap (in0, in1);
We should just fix these to use std::swap.
This will also allow us to eliminate the x temporary rtx.
gcc/ChangeLog:
* config/aarch64/aarch64.cc (aarch64_evpc_trn): Use std:swap.
(aarch64_evpc_uzp): Likewise.
(aarch64_evpc_zip): Likewise.
---
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index
d049f9a9819628a73bfd57114c3b89d848da7d9c..b75a032c2f2c55d71bcb6b4b6ef1bd7f42a97235
100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -23498,7 +23498,7 @@ aarch64_evpc_trn (struct expand_vec_perm_d *d)
{
HOST_WIDE_INT odd;
poly_uint64 nelt = d->perm.length ();
- rtx out, in0, in1, x;
+ rtx out, in0, in1;
machine_mode vmode = d->vmode;
if (GET_MODE_UNIT_SIZE (vmode) > 8)
@@ -23522,7 +23522,7 @@ aarch64_evpc_trn (struct expand_vec_perm_d *d)
at the head of aarch64-sve.md for details. */
if (BYTES_BIG_ENDIAN && d->vec_flags == VEC_ADVSIMD)
{
- x = in0, in0 = in1, in1 = x;
+ std::swap (in0, in1);
odd = !odd;
}
out = d->target;
@@ -23592,7 +23592,7 @@ static bool
aarch64_evpc_uzp (struct expand_vec_perm_d *d)
{
HOST_WIDE_INT odd;
- rtx out, in0, in1, x;
+ rtx out, in0, in1;
machine_mode vmode = d->vmode;
if (GET_MODE_UNIT_SIZE (vmode) > 8)
@@ -23615,7 +23615,7 @@ aarch64_evpc_uzp (struct expand_vec_perm_d *d)
at the head of aarch64-sve.md for details. */
if (BYTES_BIG_ENDIAN && d->vec_flags == VEC_ADVSIMD)
{
- x = in0, in0 = in1, in1 = x;
+ std::swap (in0, in1);
odd = !odd;
}
out = d->target;
@@ -23631,7 +23631,7 @@ aarch64_evpc_zip (struct expand_vec_perm_d *d)
{
unsigned int high;
poly_uint64 nelt = d->perm.length ();
- rtx out, in0, in1, x;
+ rtx out, in0, in1;
machine_mode vmode = d->vmode;
if (GET_MODE_UNIT_SIZE (vmode) > 8)
@@ -23656,7 +23656,7 @@ aarch64_evpc_zip (struct expand_vec_perm_d *d)
at the head of aarch64-sve.md for details. */
if (BYTES_BIG_ENDIAN && d->vec_flags == VEC_ADVSIMD)
{
- x = in0, in0 = in1, in1 = x;
+ std::swap (in0, in1);
high = !high;
}
out = d->target;
Go patch committed: Don't crash on f(g()) if g returns a zero-sized value
This patch to the GCC interface of the Go frontend fixes a crash in
f(g()) if g returns a zero-sized value. In that case the GCC
interface modifies g to return void, since GCC's middle-end does not
have solid support for zero-sized values. This patch detects the
f(g()) case and replaces the call to g() with (g(), zero). The test
case for this is https://go.dev/cl/417481. This fixes
https://go.dev/issue/23868. Bootstrapped and ran Go testsuite on
x86_64-pc-linux-gnu. Committed to mainline.
Ian
* go-gcc.cc (Gcc_backend::call_expression): Handle a void
argument, as for f(g()) where g returns a zero-sized type.
1eb1495f7d0fa9b49f6f3c34bbbf4dd1e850607c
diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
index f3de7a8c183..7b4b2adb058 100644
--- a/gcc/go/go-gcc.cc
+++ b/gcc/go/go-gcc.cc
@@ -2112,6 +2112,19 @@ Gcc_backend::call_expression(Bfunction*, // containing
fcn for call
args[i] = fn_args.at(i)->get_tree();
if (args[i] == error_mark_node)
return this->error_expression();
+ if (TREE_TYPE(args[i]) == void_type_node)
+ {
+ // This can happen for a case like f(g()) where g returns a
+ // zero-sized type, because in that case we've changed g to
+ // return void.
+ tree t = TYPE_ARG_TYPES(TREE_TYPE(TREE_TYPE(fn)));
+ for (size_t j = 0; j < i; ++j)
+ t = TREE_CHAIN(t);
+ tree arg_type = TREE_TYPE(TREE_VALUE(t));
+ args[i] = fold_build2_loc(EXPR_LOCATION(args[i]), COMPOUND_EXPR,
+ arg_type, args[i],
+ build_zero_cst(arg_type));
+ }
}
tree fndecl = fn;
[PATCH] c++: ICE with erroneous template redeclaration [PR106311]
Here we ICE trying to get DECL_SOURCE_LOCATION of the parm that happens
to be error_mark_node in this ill-formed test. I kept running into this
while reducing code, so it'd be good to have it fixed.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
PR c++/106311
gcc/cp/ChangeLog:
* pt.cc (redeclare_class_template): Check DECL_P before accessing
DECL_SOURCE_LOCATION.
gcc/testsuite/ChangeLog:
* g++.dg/template/redecl5.C: New test.
---
gcc/cp/pt.cc| 3 ++-
gcc/testsuite/g++.dg/template/redecl5.C | 5 +
2 files changed, 7 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/template/redecl5.C
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 718dfa5bfa8..0a294e91a79 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -6377,7 +6377,8 @@ redeclare_class_template (tree type, tree parms, tree
cons)
{
auto_diagnostic_group d;
error ("template parameter %q+#D", tmpl_parm);
- inform (DECL_SOURCE_LOCATION (parm), "redeclared here as %q#D", parm);
+ inform (DECL_P (parm) ? DECL_SOURCE_LOCATION (parm) : input_location,
+ "redeclared here as %q#D", parm);
return false;
}
diff --git a/gcc/testsuite/g++.dg/template/redecl5.C
b/gcc/testsuite/g++.dg/template/redecl5.C
new file mode 100644
index 000..fb2d698e6bc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/redecl5.C
@@ -0,0 +1,5 @@
+// PR c++/106311
+// { dg-do compile }
+
+template struct array; // { dg-error "template parameter" }
+template struct array { }; // { dg-error "declared" }
base-commit: 23dd41c480fa9f06c33c1e6090bbae53869f85af
--
2.36.1
Re: [PATCH] c++: Add __reference_con{struc,ver}ts_from_temporary [PR104477]
On Thu, Jul 14, 2022 at 11:48:51PM -0400, Jason Merrill wrote:
> On 7/14/22 13:43, Marek Polacek wrote:
> > On Tue, Jul 12, 2022 at 04:15:00PM -0400, Jason Merrill wrote:
> > > On 7/12/22 16:10, Jason Merrill wrote:
> > > > On 7/8/22 13:41, Marek Polacek wrote:
> > > > > This patch implements C++23 P2255R2, which adds two new type traits to
> > > > > detect reference binding to a temporary. They can be used to detect
> > > > > code
> > > > > like
> > > > >
> > > > > std::tuple t("meow");
> > > > >
> > > > > which is incorrect because it always creates a dangling reference,
> > > > > because
> > > > > the std::string temporary is created inside the selected constructor
> > > > > of
> > > > > std::tuple, and not outside it.
> > > > >
> > > > > There are two new compiler builtins,
> > > > > __reference_constructs_from_temporary
> > > > > and __reference_converts_from_temporary. The former is used to
> > > > > simulate
> > > > > direct- and the latter copy-initialization context. But I had a
> > > > > hard time
> > > > > finding a test where there's actually a difference. Under DR 2267,
> > > > > both
> > > > > of these are invalid:
> > > > >
> > > > > struct A { } a;
> > > > > struct B { explicit B(const A&); };
> > > > > const B &b1{a};
> > > > > const B &b2(a);
> > > > >
> > > > > so I had to peruse [over.match.ref], and eventually realized that the
> > > > > difference can be seen here:
> > > > >
> > > > > struct G {
> > > > > operator int(); // #1
> > > > > explicit operator int&&(); // #2
> > > > > };
> > > > >
> > > > > int&& r1(G{}); // use #2 (no temporary)
> > > > > int&& r2 = G{}; // use #1 (a temporary is created to be bound to
> > > > > int&&)
> > > > >
> > > > > The implementation itself was rather straightforward because we
> > > > > already
> > > > > have conv_binds_ref_to_prvalue. The main function here is
> > > > > reference_from_temporary. The renaming to
> > > > > ref_conv_binds_to_temporary_p
> > > > > is because previously the function didn't distinguish between an
> > > > > invalid
> > > > > conversion and one that binds to a prvalue.
> > > > >
> > > > > The patch also adds the relevant class and variable templates to
> > > > > .
> > > > >
> > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > > >
> > > > > PR c++/104477
> > > > >
> > > > > gcc/c-family/ChangeLog:
> > > > >
> > > > > * c-common.cc (c_common_reswords): Add
> > > > > __reference_constructs_from_temporary and
> > > > > __reference_converts_from_temporary.
> > > > > * c-common.h (enum rid): Add RID_REF_CONSTRUCTS_FROM_TEMPORARY
> > > > > and
> > > > > RID_REF_CONVERTS_FROM_TEMPORARY.
> > > > >
> > > > > gcc/cp/ChangeLog:
> > > > >
> > > > > * call.cc (ref_conv_binds_directly_p): Rename to ...
> > > > > (ref_conv_binds_to_temporary_p): ... this. Add a new bool
> > > > > parameter. Return true only if the conversion is valid and
> > > > > conv_binds_ref_to_prvalue returns true.
> > > > > * constraint.cc (diagnose_trait_expr): Handle
> > > > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > > > > * cp-tree.h (enum cp_trait_kind): Add
> > > > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY
> > > > > and CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > > > > (ref_conv_binds_directly_p): Rename to ...
> > > > > (ref_conv_binds_to_temporary_p): ... this.
> > > > > (reference_from_temporary): Declare.
> > > > > * cxx-pretty-print.cc (pp_cxx_trait_expression): Handle
> > > > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > > > > * method.cc (reference_from_temporary): New.
> > > > > * parser.cc (cp_parser_primary_expression): Handle
> > > > > RID_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > RID_REF_CONVERTS_FROM_TEMPORARY.
> > > > > (cp_parser_trait_expr): Likewise.
> > > > > (warn_for_range_copy): Adjust to call
> > > > > ref_conv_binds_to_temporary_p.
> > > > > * semantics.cc (trait_expr_value): Handle
> > > > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > > > > (finish_trait_expr): Likewise.
> > > > >
> > > > > libstdc++-v3/ChangeLog:
> > > > >
> > > > > * include/std/type_traits (reference_constructs_from_temporary,
> > > > > reference_converts_from_temporary): New class templates.
> > > > > (reference_constructs_from_temporary_v,
> > > > > reference_converts_from_temporary_v): New variable templates.
> > > > > (__cpp_lib_reference_from_temporary): Define for C++23.
> > > > > * include/std/version (__cpp_lib_reference_from_temporary):
> > > > > Define for
> > > > > C++23.
> > > > > * testsuite/20_util/variable_templates_for_traits.cc: Test
> > > > > reference_constructs_from_temporary_v and
> > > > > reference_converts_from_temporary_v.
> > > > > * tes
[PATCH] Adding three new function attributes for static analysis of file descriptors
---
gcc/analyzer/sm-fd.cc| 257 ---
gcc/c-family/c-attribs.cc| 115
gcc/doc/extend.texi | 19 ++
gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 ++
gcc/testsuite/gcc.dg/analyzer/fd-6.c | 14 ++
5 files changed, 431 insertions(+), 27 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c
diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
index 8e4300b06e2..20018dfd12b 100644
--- a/gcc/analyzer/sm-fd.cc
+++ b/gcc/analyzer/sm-fd.cc
@@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
#include "analyzer/analyzer-selftests.h"
#include "tristate.h"
#include "selftest.h"
+#include "stringpool.h"
+#include "attribs.h"
#include "analyzer/call-string.h"
#include "analyzer/program-point.h"
#include "analyzer/store.h"
#include "analyzer/region-model.h"
+#include "bitmap.h"
#if ENABLE_ANALYZER
@@ -59,6 +62,13 @@ enum access_mode
WRITE_ONLY
};
+enum fd_access_direction
+{
+ DIR_READ_WRITE,
+ DIR_READ,
+ DIR_WRITE
+};
+
class fd_state_machine : public state_machine
{
public:
@@ -104,6 +114,8 @@ public:
bool is_readonly_fd_p (state_t s) const;
bool is_writeonly_fd_p (state_t s) const;
enum access_mode get_access_mode_from_flag (int flag) const;
+ void inform_filedescriptor_attribute (tree callee_fndecl, int arg,
+fd_access_direction fd_dir) const;
/* State for a constant file descriptor (>= 0) */
state_t m_constant_fd;
@@ -146,7 +158,7 @@ private:
void check_for_open_fd (sm_context *sm_ctxt, const supernode *node,
const gimple *stmt, const gcall *call,
const tree callee_fndecl,
- enum access_direction access_fn) const;
+ enum fd_access_direction access_fn) const;
void make_valid_transitions_on_condition (sm_context *sm_ctxt,
const supernode *node,
@@ -156,6 +168,12 @@ private:
const supernode *node,
const gimple *stmt,
const svalue *lhs) const;
+ bitmap get_fd_attrs (const char *attr_name, tree callee_fndecl) const;
+ void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl, bitmap argmap,
+ fd_access_direction fd_attr_access_dir) const;
+
};
/* Base diagnostic class relative to fd_state_machine. */
@@ -294,10 +312,10 @@ class fd_access_mode_mismatch : public fd_diagnostic
{
public:
fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
- enum access_direction fd_dir,
- const tree callee_fndecl)
+ enum fd_access_direction fd_dir,
+ const tree callee_fndecl, bool attr, int arg_idx)
: fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
-m_callee_fndecl (callee_fndecl)
+m_callee_fndecl (callee_fndecl), m_attr (attr), m_arg_idx (arg_idx)
{
}
@@ -317,19 +335,27 @@ public:
bool
emit (rich_location *rich_loc) final override
{
+bool warned;
switch (m_fd_dir)
{
case DIR_READ:
-return warning_at (rich_loc, get_controlling_option (),
+warned = warning_at (rich_loc, get_controlling_option (),
"%qE on % file descriptor %qE",
m_callee_fndecl, m_arg);
+break;
case DIR_WRITE:
-return warning_at (rich_loc, get_controlling_option (),
+warned = warning_at (rich_loc, get_controlling_option (),
"%qE on % file descriptor %qE",
m_callee_fndecl, m_arg);
+break;
default:
gcc_unreachable ();
}
+ if (warned && m_attr)
+ {
+m_sm.inform_filedescriptor_attribute (m_callee_fndecl, m_arg_idx,
m_fd_dir);
+ }
+ return warned;
}
bool
@@ -359,8 +385,10 @@ public:
}
private:
- enum access_direction m_fd_dir;
+ enum fd_access_direction m_fd_dir;
const tree m_callee_fndecl;
+ bool m_attr;
+ int m_arg_idx;
};
class double_close : public fd_diagnostic
@@ -422,8 +450,9 @@ class fd_use_after_close : public fd_diagnostic
{
public:
fd_use_after_close (const fd_state_machine &sm, tree arg,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ const tree callee_fndecl, bool attr, int arg_idx)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl), m_attr
(attr),
+m_arg_idx (arg_idx)
{
}
@@ -439,12 +468,27 @@ public:
return OPT_W
[committed] analyzer: documentation nits relating to new fd warnings
Lightly tested; pushed to trunk as r13-1712-gb1d07b50d43e95.
gcc/ChangeLog:
* doc/invoke.texi (Static Analyzer Options): Add the new fd
warnings to the initial gccoptlist, and to the list of those
disabled by -fanalyzer-checker=taint.
Signed-off-by: David Malcolm
---
gcc/doc/invoke.texi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d5ff1018372..84d6f0f9860 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -446,6 +446,11 @@ Objective-C and Objective-C++ Dialects}.
-Wno-analyzer-double-fclose @gol
-Wno-analyzer-double-free @gol
-Wno-analyzer-exposure-through-output-file @gol
+-Wno-analyzer-fd-access-mode-mismatch @gol
+-Wno-analyzer-fd-double-close @gol
+-Wno-analyzer-fd-leak @gol
+-Wno-analyzer-fd-use-after-close @gol
+-Wno-analyzer-fd-use-without-check @gol
-Wno-analyzer-file-leak @gol
-Wno-analyzer-free-of-non-heap @gol
-Wno-analyzer-malloc-leak @gol
@@ -10231,6 +10236,11 @@ following warnings from @option{-fanalyzer}:
-Wanalyzer-double-fclose @gol
-Wanalyzer-double-free @gol
-Wanalyzer-exposure-through-output-file @gol
+-Wanalyzer-fd-access-mode-mismatch @gol
+-Wanalyzer-fd-double-close @gol
+-Wanalyzer-fd-leak @gol
+-Wanalyzer-fd-use-after-close @gol
+-Wanalyzer-fd-use-without-check @gol
-Wanalyzer-file-leak @gol
-Wanalyzer-free-of-non-heap @gol
-Wanalyzer-malloc-leak @gol
--
2.26.3
[committed] analyzer: fix taint false positive on optimized range checks [PR106284]
PR analyzer/106284 reports a false positive from
-Wanalyzer-tainted-array-index seen on the Linux kernel
with a version of my patches from:
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584372.html
in drivers/usb/class/usblp.c in function ‘usblp_set_protocol’ handling
usblp_ioctl on IOCNR_SET_PROTOCOL, which has:
| 1337 | if (protocol < USBLP_FIRST_PROTOCOL || protocol >
USBLP_LAST_PROTOCOL)
| |~
| ||
| |(15) following ‘false’ branch...
|..
| 1341 | if (usblp->intf->num_altsetting > 1) {
| |
| || |
| || (16) ...to here
| |(17) following ‘true’ branch...
| 1342 | alts = usblp->protocol[protocol].alt_setting;
| |
| | |
| | (18) ...to here
| | (19) use of attacker-controlled value ‘arg’ in
array lookup without bounds checking
where "arg" is "protocol" (albeit from the caller frame, the ioctl
callback), and is clearly checked at (15).
The root cause is that at -O1 and above fold-const's build_range-check
can optimize range checks
(c>=low) && (c<=high)
into
(c-low>=0) && (c-low<=high-low)
and thus into a single check:
(unsigned)(c - low) <= (unsigned)(high-low).
I initially attempted to fix this by detecting such conditions in
region_model::on_condition, and calling on_condition for both of the
implied conditions. This turned out not to work since the current
sm_context framework doesn't support applying two conditions
simultaneously: it led to a transition from the old state to has_lb,
then a transition from the old state *again* to has_ub, thus leaving
the new state as has_ub, rather than the stop state.
Instead, this patch fixes things by special-casing it within
taint_state_machine::on_condition.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r13-1713-g0a8edfbd37d399.
gcc/analyzer/ChangeLog:
PR analyzer/106284
* sm-taint.cc (taint_state_machine::on_condition): Handle range
checks optimized by build_range_check.
gcc/testsuite/ChangeLog:
PR analyzer/106284
* gcc.dg/analyzer/torture/taint-read-index-2.c: New test.
Signed-off-by: David Malcolm
---
gcc/analyzer/sm-taint.cc | 42 ++
.../analyzer/torture/taint-read-index-2.c | 56 +++
2 files changed, 98 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/torture/taint-read-index-2.c
diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index 4075cf6d868..2de9284624e 100644
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -848,6 +848,48 @@ taint_state_machine::on_condition (sm_context *sm_ctxt,
case LE_EXPR:
case LT_EXPR:
{
+ /* Detect where build_range_check has optimized
+ (c>=low) && (c<=high)
+ into
+ (c-low>=0) && (c-low<=high-low)
+ and thus into:
+ (unsigned)(c - low) <= (unsigned)(high-low). */
+ if (const binop_svalue *binop_sval
+ = lhs->dyn_cast_binop_svalue ())
+ {
+ const svalue *inner_lhs = binop_sval->get_arg0 ();
+ enum tree_code inner_op = binop_sval->get_op ();
+ const svalue *inner_rhs = binop_sval->get_arg1 ();
+ if (const svalue *before_cast = inner_lhs->maybe_undo_cast ())
+ inner_lhs = before_cast;
+ if (tree outer_rhs_cst = rhs->maybe_get_constant ())
+ if (tree inner_rhs_cst = inner_rhs->maybe_get_constant ())
+ if (inner_op == PLUS_EXPR
+ && TREE_CODE (inner_rhs_cst) == INTEGER_CST
+ && TREE_CODE (outer_rhs_cst) == INTEGER_CST
+ && TYPE_UNSIGNED (TREE_TYPE (inner_rhs_cst))
+ && TYPE_UNSIGNED (TREE_TYPE (outer_rhs_cst)))
+ {
+ /* We have
+ (unsigned)(INNER_LHS + CST_A) get_state (stmt, inner_lhs);
+ if (old_state == m_tainted
+ || old_state == m_has_lb
+ || old_state == m_has_ub)
+ sm_ctxt->set_next_state (stmt, inner_lhs, m_stop);
+ return;
+ }
+ }
+
sm_ctxt->on_transition (node, stmt, lhs, m_tainted,
m_has_ub);
sm_ctxt->on_transition (node, stmt, lhs, m_has_lb,
diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/taint-read-index-2.c
b/gcc/testsuite/gcc.dg/analyzer/torture/taint-read-index-2.c
new file mode 100644
index 000..6a4ebdbba16
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/torture/taint-read-index-2.c
@@ -0,0 +1,56 @@
+// TODO: remove need for the taint option:
+/* { dg-additional-options "-fanalyzer-checke
Re: [x86 PATCH] PR target/106273: Add earlyclobber to *andn3_doubleword_bmi
On Fri, Jul 15, 2022 at 3:28 PM Roger Sayle wrote:
>
>
>
> This patch resolves PR target/106273 which is a wrong code regression
>
> caused by the recent reorganization to split doubleword operations after
>
> reload on x86. For the failing test case, the constraints on the
>
> andnti3_doubleword_bmi pattern allow reload to allocate the output and
>
> operand in overlapping but non-identical registers, i.e.
>
>
>
> (insn 45 44 66 2 (parallel [
>
> (set (reg/v:TI 5 di [orig:96 i ] [96])
>
> (and:TI (not:TI (reg:TI 39 r11 [orig:83 _2 ] [83]))
>
> (reg/v:TI 4 si [orig:100 i ] [100])))
>
> (clobber (reg:CC 17 flags))
>
> ]) "pr106273.c":13:5 562 {*andnti3_doubleword_bmi}
>
>
>
> where the output is in registers 5 and 6, and the second operand is
>
> registers 4 and 5, which then leads to the incorrect split:
>
>
>
> (insn 113 44 114 2 (parallel [
>
> (set (reg:DI 5 di [orig:96 i ] [96])
>
> (and:DI (not:DI (reg:DI 39 r11 [orig:83 _2 ] [83]))
>
> (reg:DI 4 si [orig:100 i ] [100])))
>
> (clobber (reg:CC 17 flags))
>
> ]) "pr106273.c":13:5 566 {*andndi_1}
>
>
>
> (insn 114 113 66 2 (parallel [
>
> (set (reg:DI 6 bp [ i+8 ])
>
> (and:DI (not:DI (reg:DI 40 r12 [ _2+8 ]))
>
> (reg:DI 5 di [ i+8 ])))
>
> (clobber (reg:CC 17 flags))
>
> ]) "pr106273.c":13:5 566 {*andndi_1}
>
>
>
> [Notice that reg:DI 5 is set in the first instruction, but assumed
>
> to have its original value in the second]. My first thought was
>
> that this could be fixed by swapping the order of the split instructions
>
> (which works in this case), but in the general case, it's impossible
>
> to handle (set (reg:TI x) (op (reg:TI x+1) (reg:TI x-1)). Hence for
>
> correctness this pattern needs an earlyclobber "=&r", but we can also
>
> allow cases where the output is the same as one of the operands (using
>
> constraint "0"). The other binary logic operations (AND, IOR, XOR)
>
> are unaffected as they constrain the output to match the first
>
> operand, but BMI's andn is a three-operand instruction which can
>
> lead to the overlapping cases described above.
>
>
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
>
> and make -k check, both with and without --target_board=unix{-m32} with
>
> no new failures. Ok for mainline?
>
>
>
> 2022-07-15 Roger Sayle
>
>
>
> gcc/ChangeLog
>
> PR target/106273
>
> * config/i386/i386.md (*andn3_doubleword_bmi): Update the
>
> constraints to reflect the output is earlyclobber, unless it is
>
> the same register (pair) as one of the operands.
>
>
>
> gcc/testsuite/ChangeLog
>
> PR target/106273
>
> * gcc.target/i386/pr106273.c: New test case.
OK with a small testcase adjustment.
Thanks,
Uros.
+int
+main (void)
If possible, please name this function "bar", the "main" function
assumes a runtime test.
+{
+ u8 x;
+ foo (5, &x);
+ if (x != 5)
+__builtin_abort ();
+ return 0;
+}
>
>
>
>
>
> Thanks again, and sorry for the inconvenience.
>
> Roger
>
> --
>
>
Re: kernel sparse annotations vs. compiler attributes and debug_annotate_{type, decl} WAS: Re: [PATCH 0/9] Add debug_annotate attributes
On 7/15/22 7:17 AM, Jose E. Marchesi wrote:
On 7/14/22 8:09 AM, Jose E. Marchesi wrote:
Hi Yonghong.
On 7/7/22 1:24 PM, Jose E. Marchesi wrote:
Hi Yonghong.
On 6/21/22 9:12 AM, Jose E. Marchesi wrote:
On 6/17/22 10:18 AM, Jose E. Marchesi wrote:
Hi Yonghong.
On 6/15/22 1:57 PM, David Faust wrote:
On 6/14/22 22:53, Yonghong Song wrote:
On 6/7/22 2:43 PM, David Faust wrote:
Hello,
This patch series adds support for:
- Two new C-language-level attributes that allow to associate (to "annotate" or
to "tag") particular declarations and types with arbitrary strings. As
explained below, this is intended to be used to, for example,
characterize
certain pointer types.
- The conveyance of that information in the DWARF output in the form of a new
DIE: DW_TAG_GNU_annotation.
- The conveyance of that information in the BTF output in the form of two new
kinds of BTF objects: BTF_KIND_DECL_TAG and BTF_KIND_TYPE_TAG.
All of these facilities are being added to the eBPF ecosystem, and support for
them exists in some form in LLVM.
Purpose
===
1) Addition of C-family language constructs (attributes) to specify free-text
tags on certain language elements, such as struct fields.
The purpose of these annotations is to provide additional
information about
types, variables, and function parameters of interest to the kernel.
A
driving use case is to tag pointer types within the linux kernel and
eBPF
programs with additional semantic information, such as '__user' or
'__rcu'.
For example, consider the linux kernel function do_execve with the
following declaration:
static int do_execve(struct filename *filename,
const char __user *const __user *__argv,
const char __user *const __user *__envp);
Here, __user could be defined with these annotations to record
semantic
information about the pointer parameters (e.g., they are
user-provided) in
DWARF and BTF information. Other kernel facilites such as the eBPF
verifier
can read the tags and make use of the information.
2) Conveying the tags in the generated DWARF debug info.
The main motivation for emitting the tags in DWARF is that the Linux
kernel
generates its BTF information via pahole, using DWARF as a source:
++ BTF BTF +--+
| pahole |---> vmlinux.btf --->| verifier |
++ +--+
^^
||
DWARF |BTF |
||
vmlinux +-+
module1.ko | BPF program |
module2.ko +-+
...
This is because:
a) Unlike GCC, LLVM will only generate BTF for BPF programs.
b) GCC can generate BTF for whatever target with -gbtf, but there
is no
support for linking/deduplicating BTF in the linker.
In the scenario above, the verifier needs access to the pointer tags
of
both the kernel types/declarations (conveyed in the DWARF and
translated
to BTF by pahole) and those of the BPF program (available directly
in BTF).
Another motivation for having the tag information in DWARF,
unrelated to
BPF and BTF, is that the drgn project (another DWARF consumer) also
wants
to benefit from these tags in order to differentiate between
different
kinds of pointers in the kernel.
3) Conveying the tags in the generated BTF debug info.
This is easy: the main purpose of having this info in BTF is for the
compiled eBPF programs. The kernel verifier can then access the tags
of pointers used by the eBPF programs.
For more information about these tags and the motivation behind them, please
refer to the following linux kernel discussions:
https://lore.kernel.org/bpf/20210914223004.244411-1-...@fb.com/
https://lore.kernel.org/bpf/20211012164838.3345699-1-...@fb.com/
https://lore.kernel.org/bpf/2022012604.1504583-1-...@fb.com/
Implementation Overview
===
To enable these annotations, two new C language attributes are added:
__attribute__((debug_annotate_decl("foo"))) and
__attribute__((debug_annotate_type("bar"))). Both attributes accept a single
arbitrary string constant argument, which will be recorded in the generated
DWARF and/or BTF debug information. They have no effect on code generation.
Note that we are not using the same
Re: [PATCH] Adding three new function attributes for static analysis of file descriptors
On Fri, 2022-07-15 at 21:08 +0530, Immad Mir wrote:
Thanks for the patch.
Various review comments:
The patch is missing a ChangeLog.
> ---
> gcc/analyzer/sm-fd.cc| 257 ---
> gcc/c-family/c-attribs.cc| 115
> gcc/doc/extend.texi | 19 ++
> gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 ++
> gcc/testsuite/gcc.dg/analyzer/fd-6.c | 14 ++
> 5 files changed, 431 insertions(+), 27 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c
>
> diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
> index 8e4300b06e2..20018dfd12b 100644
> --- a/gcc/analyzer/sm-fd.cc
> +++ b/gcc/analyzer/sm-fd.cc
[...snip...]
> +enum fd_access_direction
> +{
> + DIR_READ_WRITE,
> + DIR_READ,
> + DIR_WRITE
> +};
Don't we already have DIR_READ and DIR_WRITE in enum access_direction
in analyzer.h? I wonder why this isn't an error due to the naming
collision?
The new enum refers to a *set* of valid access directions, so maybe
rename the new enum to "enum access_directions" (note the plural), and
rename the elements from DIR_ to DIRS_. Please carefully check all
usage of DIR_* in sm-fd.cc.
Maybe instead we should convert the values in enum access_direction
from indexes into bitfield masks.
[...snip...]
> @@ -317,19 +335,27 @@ public:
>bool
>emit (rich_location *rich_loc) final override
>{
> +bool warned;
> switch (m_fd_dir)
>{
>case DIR_READ:
> -return warning_at (rich_loc, get_controlling_option (),
> +warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on % file descriptor %qE",
> m_callee_fndecl, m_arg);
> +break;
>case DIR_WRITE:
> -return warning_at (rich_loc, get_controlling_option (),
> +warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on % file descriptor %qE",
> m_callee_fndecl, m_arg);
> +break;
...i.e. which DIR_READ/DIR_WRITE values are these cases using?
>default:
> gcc_unreachable ();
>}
> + if (warned && m_attr)
> + {
> +m_sm.inform_filedescriptor_attribute (m_callee_fndecl, m_arg_idx,
> m_fd_dir);
> + }
Redundant braces here ^^^
> + return warned;
>}
>
>bool
> @@ -359,8 +385,10 @@ public:
>}
>
> private:
> - enum access_direction m_fd_dir;
> + enum fd_access_direction m_fd_dir;
>const tree m_callee_fndecl;
> + bool m_attr;
> + int m_arg_idx;
> };
Most (all?) of the concrete subclasses seem to be adding the two fields
m_attr and m_arg_idx, so can you add them to class fd_diagnostic, or
introduce a common subclass, rather than repeating them in each
subclass?
I suspect the code would be simpler if inform_filedescriptor_attribute
were a method of fd_diagnostic, rather than of the state_machine: you
could move the (&& m_attr) part of the conditional into there, rather
than having every emit vfunc do the same test, and having to pass the
same fields to it each time (except perhaps the access dir?).
[...snip...]
> @@ -466,25 +510,29 @@ public:
>describe_final_event (const evdesc::final_event &ev) final override
>{
> if (m_first_close_event.known_p ())
> - return ev.formatted_print (
> - "%qE on closed file descriptor %qE; %qs was at %@",
> m_callee_fndecl,
> - m_arg, "close", &m_first_close_event);
> -else
> - return ev.formatted_print ("%qE on closed file descriptor %qE",
> - m_callee_fndecl, m_arg);
> +return ev.formatted_print (
> +"%qE on closed file descriptor %qE; %qs was at %@",
> m_callee_fndecl,
> +m_arg, "close", &m_first_close_event);
> + else
> +return ev.formatted_print ("%qE on closed file descriptor %qE",
> + m_callee_fndecl, m_arg);
What changed here? Is this just whitespace changes?
[...snip...]
> fd_state_machine::fd_state_machine (logger *logger)
> : state_machine ("file-descriptor", logger),
>m_constant_fd (add_state ("fd-constant")),
> @@ -647,11 +733,126 @@ fd_state_machine::on_stmt (sm_context *sm_ctxt, const
> supernode *node,
> on_read (sm_ctxt, node, stmt, call, callee_fndecl);
> return true;
>} // "read"
> +
> +
> + {
> +// Handle __attribute__((fd_arg))
> +bitmap argmap = get_fd_attrs ("fd_arg", callee_fndecl);
> +if (argmap)
> + check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl,
> argmap, DIR_READ_WRITE);
> +
> +// Handle __attribute__((fd_arg_read))
> +bitmap read_argmap = get_fd_attrs ("fd_arg_read", callee_fndecl);
> +if(read_argmap)
> + check_for_fd_attrs (sm
Re: [PATCH] c++: Add __reference_con{struc, ver}ts_from_temporary [PR104477]
Well, is_xible is not is_xible_p because it doesn't need to be both is_*
and *_p. But xes_from_temporary is less obviously a question, so
xes_from_temporary_p would imho be a better name.
On Fri, Jul 15, 2022, 18:33 Marek Polacek via Libstdc++ <
libstd...@gcc.gnu.org> wrote:
> On Thu, Jul 14, 2022 at 11:48:51PM -0400, Jason Merrill wrote:
> > On 7/14/22 13:43, Marek Polacek wrote:
> > > On Tue, Jul 12, 2022 at 04:15:00PM -0400, Jason Merrill wrote:
> > > > On 7/12/22 16:10, Jason Merrill wrote:
> > > > > On 7/8/22 13:41, Marek Polacek wrote:
> > > > > > This patch implements C++23 P2255R2, which adds two new type
> traits to
> > > > > > detect reference binding to a temporary. They can be used to
> detect code
> > > > > > like
> > > > > >
> > > > > > std::tuple t("meow");
> > > > > >
> > > > > > which is incorrect because it always creates a dangling
> reference,
> > > > > > because
> > > > > > the std::string temporary is created inside the selected
> constructor of
> > > > > > std::tuple, and not outside it.
> > > > > >
> > > > > > There are two new compiler builtins,
> > > > > > __reference_constructs_from_temporary
> > > > > > and __reference_converts_from_temporary. The former is used to
> simulate
> > > > > > direct- and the latter copy-initialization context. But I had a
> > > > > > hard time
> > > > > > finding a test where there's actually a difference. Under DR
> 2267, both
> > > > > > of these are invalid:
> > > > > >
> > > > > > struct A { } a;
> > > > > > struct B { explicit B(const A&); };
> > > > > > const B &b1{a};
> > > > > > const B &b2(a);
> > > > > >
> > > > > > so I had to peruse [over.match.ref], and eventually realized
> that the
> > > > > > difference can be seen here:
> > > > > >
> > > > > > struct G {
> > > > > > operator int(); // #1
> > > > > > explicit operator int&&(); // #2
> > > > > > };
> > > > > >
> > > > > > int&& r1(G{}); // use #2 (no temporary)
> > > > > > int&& r2 = G{}; // use #1 (a temporary is created to be bound to
> int&&)
> > > > > >
> > > > > > The implementation itself was rather straightforward because we
> already
> > > > > > have conv_binds_ref_to_prvalue. The main function here is
> > > > > > reference_from_temporary. The renaming to
> ref_conv_binds_to_temporary_p
> > > > > > is because previously the function didn't distinguish between an
> invalid
> > > > > > conversion and one that binds to a prvalue.
> > > > > >
> > > > > > The patch also adds the relevant class and variable templates to
> > > > > > .
> > > > > >
> > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > > > >
> > > > > > PR c++/104477
> > > > > >
> > > > > > gcc/c-family/ChangeLog:
> > > > > >
> > > > > > * c-common.cc (c_common_reswords): Add
> > > > > > __reference_constructs_from_temporary and
> > > > > > __reference_converts_from_temporary.
> > > > > > * c-common.h (enum rid): Add
> RID_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > > RID_REF_CONVERTS_FROM_TEMPORARY.
> > > > > >
> > > > > > gcc/cp/ChangeLog:
> > > > > >
> > > > > > * call.cc (ref_conv_binds_directly_p): Rename to ...
> > > > > > (ref_conv_binds_to_temporary_p): ... this. Add a new bool
> > > > > > parameter. Return true only if the conversion is valid and
> > > > > > conv_binds_ref_to_prvalue returns true.
> > > > > > * constraint.cc (diagnose_trait_expr): Handle
> > > > > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > > > > > * cp-tree.h (enum cp_trait_kind): Add
> > > > > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY
> > > > > > and CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > > > > > (ref_conv_binds_directly_p): Rename to ...
> > > > > > (ref_conv_binds_to_temporary_p): ... this.
> > > > > > (reference_from_temporary): Declare.
> > > > > > * cxx-pretty-print.cc (pp_cxx_trait_expression): Handle
> > > > > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > > > > > * method.cc (reference_from_temporary): New.
> > > > > > * parser.cc (cp_parser_primary_expression): Handle
> > > > > > RID_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > > RID_REF_CONVERTS_FROM_TEMPORARY.
> > > > > > (cp_parser_trait_expr): Likewise.
> > > > > > (warn_for_range_copy): Adjust to call
> ref_conv_binds_to_temporary_p.
> > > > > > * semantics.cc (trait_expr_value): Handle
> > > > > > CPTK_REF_CONSTRUCTS_FROM_TEMPORARY and
> > > > > > CPTK_REF_CONVERTS_FROM_TEMPORARY.
> > > > > > (finish_trait_expr): Likewise.
> > > > > >
> > > > > > libstdc++-v3/ChangeLog:
> > > > > >
> > > > > > * include/std/type_traits
> (reference_constructs_from_temporary,
> > > > > > reference_converts_from_temporary): New class templates.
> > > > > > (reference_constructs_from_temporary_v,
> > > > > > reference_converts_from_temporary_v): New variable
> templates.
> > > > > > (_
[PATCH, committed] Fortran: do not generate conflicting results under -ff2c [PR104313]
Dear all,
the attached patch by Steve fixes a regression under -ff2c for functions
where the result is not set. There would otherwise be conflicting
declarations of the returned result, which gimple doesn't like.
I've committed this as obvious after discussion with Steve for him,
see PR, as
commit r13-1715-g517fb1a78102df43f052c6934c27dd51d786aff7
This fixes a 10/11/12/13 regression, will be backported in the next days.
Thanks,
Harald
From 517fb1a78102df43f052c6934c27dd51d786aff7 Mon Sep 17 00:00:00 2001
From: Steve Kargl
Date: Fri, 15 Jul 2022 22:07:15 +0200
Subject: [PATCH] Fortran: do not generate conflicting results under -ff2c
[PR104313]
gcc/fortran/ChangeLog:
PR fortran/104313
* trans-decl.cc (gfc_generate_return): Do not generate conflicting
fake results for functions with no result variable under -ff2c.
gcc/testsuite/ChangeLog:
PR fortran/104313
* gfortran.dg/pr104313.f: New test.
---
gcc/fortran/trans-decl.cc| 2 +-
gcc/testsuite/gfortran.dg/pr104313.f | 11 +++
2 files changed, 12 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gfortran.dg/pr104313.f
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index 6493cc2f6b1..908a4c6d42e 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -6474,7 +6474,7 @@ gfc_generate_return (void)
NULL_TREE, and a 'return' is generated without a variable.
The following generates a 'return __result_XXX' where XXX is
the function name. */
- if (sym == sym->result && sym->attr.function)
+ if (sym == sym->result && sym->attr.function && !flag_f2c)
{
result = gfc_get_fake_result_decl (sym, 0);
result = fold_build2_loc (input_location, MODIFY_EXPR,
diff --git a/gcc/testsuite/gfortran.dg/pr104313.f b/gcc/testsuite/gfortran.dg/pr104313.f
new file mode 100644
index 000..89c8947cb0a
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr104313.f
@@ -0,0 +1,11 @@
+! { dg-do compile }
+! { dg-additional-options "-ff2c -fdump-tree-original" }
+!
+! PR fortran/104313 - ICE verify_gimple failed with -ff2c
+! Contributed by G.Steinmetz
+
+ function f(a)
+ return
+ end
+
+! { dg-final { scan-tree-dump-times "return" 1 "original" } }
--
2.35.3
Re: [PATCH] c++: Add __reference_con{struc,ver}ts_from_temporary [PR104477]
On Fri, Jul 15, 2022 at 10:59:41PM +0300, Ville Voutilainen wrote: > Well, is_xible is not is_xible_p because it doesn't need to be both is_* > and *_p. But xes_from_temporary is less obviously a question, so > xes_from_temporary_p would imho be a better name. Yeah, I guess so. But I've already pushed the patch. Marek
Re: [PATCH, rs6000] Additional cleanup of rs6000_builtin_mask
On Thu, 2022-07-14 at 11:28 +0800, Kewen.Lin wrote:
> Hi Will,
>
> Thanks for the cleanup! Some comments are inlined.
Hi,
Thanks for the review. A few comments and responses below. TLDR I'll
incorporate the suggestions in V2 that will show up ... after. :-)
>
> on 2022/7/14 05:39, will schmidt wrote:
> > [PATCH, rs6000] Additional cleanup of rs6000_builtin_mask
> >
> > Hi,
> > Post the rs6000 builtins rewrite, some of the leftover builtin
> > code is redundant and can be removed.
> > This replaces the remaining usage of bu_mask in
> > rs6000_target_modify_macros() with checks against the rs6000_cpu
> > directly.
> > Thusly the bu_mask variable can be removed. After that variable
> > is eliminated there are no other uses of
> > rs6000_builtin_mask_calculate(),
> > so that function can also be safely removed.
> >
>
> The TargetVariable rs6000_builtin_mask in rs6000.opt is useless, it
> seems
> it can be removed together?
Yes, if I also remove usage of x_rs6000_builtin_mask. There are a few
remaining reference to x_r_b_m, but those appear safe to remove after
this cleanup as well. I'll confirm and likely include the removal in
V2.
>
> > I have tested this on current systems (P8,P9,P10) without
> > regressions.
> >
> > OK for trunk?
> >
> >
> > Thanks,
> > -Will
> >
> >
> >
> > - /* Set the builtin mask of the various options used that could
> > affect which
> > - builtins were used. In the past we used target_flags, but
> > we've run out
> > - of bits, and some options are no longer in target_flags. */
> > - rs6000_builtin_mask = rs6000_builtin_mask_calculate ();
> > - if (TARGET_DEBUG_BUILTIN || TARGET_DEBUG_TARGET)
> > -rs6000_print_builtin_options (stderr, 0, "builtin mask",
> > - rs6000_builtin_mask);
> > -
>
> I wonder if it's a good idea to still dump some information for
> built-in
> functions debugging even with new bif framework, it can be handled in
> a
> separated patch if yes. The new bif framework adopts stanzas for bif
> guarding, if we want to do similar things, we can refer to the code
> like:
> TARGET_POPCNTB means all bifs with ENB_P5 are available
> TARGET_CMPB means all bifs with ENB_P6 are available
> ...
>
> , dump information like "current enabled stanzas: ENB_xx, ENB_xxx,
> ..."
> (even without ENB_ prefix).
Possibly. There does exist some debug already, and I still have some
work in progress related to some of the OPTION and TARGET handling.
I'll keep this in mind as I continue poking in this space. :-)
> >/* Initialize all of the registers. */
> >rs6000_init_hard_regno_mode_ok (global_init_p);
> >
> >/* Save the initial options in case the user does function
> > specific options */
> >if (global_init_p)
> > @@ -24495,17 +24442,15 @@ rs6000_pragma_target_parse (tree args,
> > tree pop_target)
> >
> >if ((diff_flags != 0) || (diff_bumask != 0))
> > {
> > /* Delete old macros. */
> > rs6000_target_modify_macros_ptr (false,
> > - prev_flags & diff_flags,
> > - prev_bumask & diff_bumask);
> > + prev_flags & diff_flags);
> >
> > /* Define new macros. */
> > rs6000_target_modify_macros_ptr (true,
> > - cur_flags & diff_flags,
> > - cur_bumask & diff_bumask);
> > + cur_flags & diff_flags);
> > }
> > }
> >
> >return true;
> > }
> > @@ -24732,19 +24677,10 @@ rs6000_print_isa_options (FILE *file, int
> > indent, const char *string,
> >rs6000_print_options_internal (file, indent, string, flags, "-
> > m",
> > &rs6000_opt_masks[0],
> > ARRAY_SIZE (rs6000_opt_masks));
> > }
> >
> > -static void
> > -rs6000_print_builtin_options (FILE *file, int indent, const char
> > *string,
> > - HOST_WIDE_INT flags)
> > -{
> > - rs6000_print_options_internal (file, indent, string, flags, "",
> > -&rs6000_builtin_mask_names[0],
> > -ARRAY_SIZE
> > (rs6000_builtin_mask_names));
> > -}
>
> rs6000_builtin_mask_names becomes useless too, can be removed too?
It can. I'll include removal in V2.
Thanks
-Will
>
> BR,
> Kewen
Re: [PATCH] libphobos: Fix instability in the parallelized testsuite
Excerpts from Lewis Hyatt via Gcc-patches's message of Juli 14, 2022 11:53 pm:
> Hello-
>
> I get a different number of test results from libphobos.unittest/unittest.exp,
> depending on server load. I believe it's because this testsuite doesn't check
> runtest_file_p:
>
> $ make -j 1 RUNTESTFLAGS='unittest.exp' check-target-libphobos | grep '^#'
> # of expected passes 10
>
> $ make -j 2 RUNTESTFLAGS='unittest.exp' check-target-libphobos | grep '^#'
> # of expected passes 10
> # of expected passes 10
>
> $ make -j 4 RUNTESTFLAGS='unittest.exp' check-target-libphobos | grep '^#'
> # of expected passes 10
> # of expected passes 10
> # of expected passes 10
> # of expected passes 10
>
> When running in parallel along with other tests, even at a fixed argument
> for -j, the number of tests that actually execute will depend on how many of
> the
> parallel sub-makes happened to start prior to the first one finishing, hence
> it changes from run to run.
>
> The attached patch fixes it for me, if it looks OK? Thanks, this would remove
> some noise from before/after test comparisons.
>
> -Lewis
> libphobos: Fix instability in the parallelized testsuite
>
> libphobos.unittest/unittest.exp calls bare dg-test rather than dg-runtest, and
> so it should call runtest_file_p to determine whether to run each test or
> not. Without that call, the tests run too many times in parallel mode (they
> will
> run as many times, as the argument to make -j).
Hi Lewis,
Thanks! Good spot. I think it should be calling dg-runtest however,
same as what libphobos.cycles/cycles.exp is doing. Could also fix the
test name so each one is unique, just to hit two birds in one -
something like the following would suffice (haven't had time to check).
Kind Regards,
Iain.
---
--- a/libphobos/testsuite/libphobos.unittest/unittest.exp
+++ b/libphobos/testsuite/libphobos.unittest/unittest.exp
@@ -42,8 +42,10 @@ foreach unit_test $unit_test_list {
set expected_fail [lindex $unit_test 1]
foreach test $tests {
-set shouldfail $expected_fail
-dg-test $test "" $test_flags
+ set libphobos_test_name "[dg-trim-dirname $srcdir $test] $test_flags"
+ set shouldfail $expected_fail
+ dg-runtest $test "" $test_flags
+ set libphobos_test_name ""
}
set shouldfail 0
[PATCH 1/2] xtensa: constantsynth: Make try to find shorter instruction
This patch allows the constant synthesis to choose shorter instruction
if possible.
/* example */
int test(void) {
return 128 << 8;
}
;; before
test:
movia2, 0x100
addmi a2, a2, 0x7f00
ret.n
;; after
test:
movi.n a2, 1
sllia2, a2, 15
ret.n
When the Code Density Option is configured, the latter is one byte smaller
than the former.
gcc/ChangeLog:
* config/xtensa/xtensa.cc (xtensa_emit_constantsynth): Remove.
(xtensa_constantsynth_2insn): Change to try all three synthetic
methods and to use the one that fits the immediate value of
the seed into a Narrow Move Immediate instruction "MOVI.N"
when the Code Density Option is configured.
---
gcc/config/xtensa/xtensa.cc | 58 ++---
1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/gcc/config/xtensa/xtensa.cc b/gcc/config/xtensa/xtensa.cc
index 13f2b2b832c..94337452ba8 100644
--- a/gcc/config/xtensa/xtensa.cc
+++ b/gcc/config/xtensa/xtensa.cc
@@ -1035,35 +1035,35 @@ xtensa_split_operand_pair (rtx operands[4],
machine_mode mode)
load-immediate / arithmetic ones, instead of a L32R instruction
(plus a constant in litpool). */
-static void
-xtensa_emit_constantsynth (rtx dst, enum rtx_code code,
- HOST_WIDE_INT imm0, HOST_WIDE_INT imm1,
- rtx (*gen_op)(rtx, HOST_WIDE_INT),
- HOST_WIDE_INT imm2)
-{
- gcc_assert (REG_P (dst));
- emit_move_insn (dst, GEN_INT (imm0));
- emit_move_insn (dst, gen_rtx_fmt_ee (code, SImode,
- dst, GEN_INT (imm1)));
- if (gen_op)
-emit_move_insn (dst, gen_op (dst, imm2));
-}
-
static int
xtensa_constantsynth_2insn (rtx dst, HOST_WIDE_INT srcval,
rtx (*gen_op)(rtx, HOST_WIDE_INT),
HOST_WIDE_INT op_imm)
{
- int shift = exact_log2 (srcval + 1);
+ HOST_WIDE_INT imm = INT_MAX;
+ rtx x = NULL_RTX;
+ int shift;
+ gcc_assert (REG_P (dst));
+
+ shift = exact_log2 (srcval + 1);
if (IN_RANGE (shift, 1, 31))
{
- xtensa_emit_constantsynth (dst, LSHIFTRT, -1, 32 - shift,
-gen_op, op_imm);
- return 1;
+ imm = -1;
+ x = gen_lshrsi3 (dst, dst, GEN_INT (32 - shift));
}
- if (IN_RANGE (srcval, (-2048 - 32768), (2047 + 32512)))
+
+ shift = ctz_hwi (srcval);
+ if ((!x || (TARGET_DENSITY && ! IN_RANGE (imm, -32, 95)))
+ && xtensa_simm12b (srcval >> shift))
+{
+ imm = srcval >> shift;
+ x = gen_ashlsi3 (dst, dst, GEN_INT (shift));
+}
+
+ if ((!x || (TARGET_DENSITY && ! IN_RANGE (imm, -32, 95)))
+ && IN_RANGE (srcval, (-2048 - 32768), (2047 + 32512)))
{
HOST_WIDE_INT imm0, imm1;
@@ -1076,19 +1076,19 @@ xtensa_constantsynth_2insn (rtx dst, HOST_WIDE_INT
srcval,
imm0 = srcval - imm1;
if (TARGET_DENSITY && imm1 < 32512 && IN_RANGE (imm0, 224, 255))
imm0 -= 256, imm1 += 256;
- xtensa_emit_constantsynth (dst, PLUS, imm0, imm1, gen_op, op_imm);
- return 1;
+ imm = imm0;
+ x = gen_addsi3 (dst, dst, GEN_INT (imm1));
}
- shift = ctz_hwi (srcval);
- if (xtensa_simm12b (srcval >> shift))
-{
- xtensa_emit_constantsynth (dst, ASHIFT, srcval >> shift, shift,
-gen_op, op_imm);
- return 1;
-}
+ if (!x)
+return 0;
- return 0;
+ emit_move_insn (dst, GEN_INT (imm));
+ emit_insn (x);
+ if (gen_op)
+emit_move_insn (dst, gen_op (dst, op_imm));
+
+ return 1;
}
static rtx
--
2.20.1
[PATCH 2/2] xtensa: Optimize "bitwise AND with imm1" followed by "branch if (not) equal to imm2"
This patch enhances the effectiveness of the previously posted one:
"xtensa: Optimize bitwise AND operation with some specific forms of constants".
/* example */
extern void foo(int);
void test(int a) {
if ((a & (-1U << 8)) == (128 << 8)) /* 0 or one of "b4const" */
foo(a);
}
;; before
.global test
test:
movia3, -0x100
movi.n a4, 1
and a3, a2, a3
sllia4, a4, 15
bne a3, a4, .L3
j.l foo, a9
.L1:
ret.n
;; after
.global test
test:
srlia3, a2, 8
bneia3, 128, .L1
j.l foo, a9
.L1:
ret.n
gcc/ChangeLog:
* config/xtensa/xtensa.md
(*masktrue_const_pow2_minus_one, *masktrue_const_negative_pow2,
*masktrue_const_shifted_mask): If the immediate for bitwise AND is
represented as '-(1 << N)', decrease the lower bound of N from 12
to 1. And the other immediate for conditional branch is now no
longer limited to zero, but also one of some positive integers.
Finally, remove the checks of some conditions, because the comparison
expressions that don't satisfy such checks are determined as
compile-time constants and thus will be optimized away before
RTL expansion.
---
gcc/config/xtensa/xtensa.md | 73 ++---
1 file changed, 44 insertions(+), 29 deletions(-)
diff --git a/gcc/config/xtensa/xtensa.md b/gcc/config/xtensa/xtensa.md
index 6a58d3e2776..c02f1a56641 100644
--- a/gcc/config/xtensa/xtensa.md
+++ b/gcc/config/xtensa/xtensa.md
@@ -1716,63 +1716,78 @@
(define_insn_and_split "*masktrue_const_pow2_minus_one"
[(set (pc)
- (if_then_else (match_operator 3 "boolean_operator"
+ (if_then_else (match_operator 4 "boolean_operator"
[(and:SI (match_operand:SI 0 "register_operand" "r")
(match_operand:SI 1 "const_int_operand" "i"))
-(const_int 0)])
- (label_ref (match_operand 2 "" ""))
+(match_operand:SI 2 "const_int_operand" "i")])
+ (label_ref (match_operand 3 "" ""))
(pc)))]
- "IN_RANGE (exact_log2 (INTVAL (operands[1]) + 1), 17, 31)"
+ "IN_RANGE (exact_log2 (INTVAL (operands[1]) + 1), 17, 31)
+ /* && (~INTVAL (operands[1]) & INTVAL (operands[2])) == 0 // can be
omitted */
+ && xtensa_b4const_or_zero (INTVAL (operands[2]) << (32 - floor_log2 (INTVAL
(operands[1]) + 1)))"
"#"
"&& can_create_pseudo_p ()"
- [(set (match_dup 4)
+ [(set (match_dup 5)
(ashift:SI (match_dup 0)
(match_dup 1)))
(set (pc)
- (if_then_else (match_op_dup 3
- [(match_dup 4)
-(const_int 0)])
- (label_ref (match_dup 2))
+ (if_then_else (match_op_dup 4
+ [(match_dup 5)
+(match_dup 2)])
+ (label_ref (match_dup 3))
(pc)))]
{
- operands[1] = GEN_INT (32 - floor_log2 (INTVAL (operands[1]) + 1));
- operands[4] = gen_reg_rtx (SImode);
+ int shift = 32 - floor_log2 (INTVAL (operands[1]) + 1);
+ operands[1] = GEN_INT (shift);
+ operands[2] = GEN_INT (INTVAL (operands[2]) << shift);
+ operands[5] = gen_reg_rtx (SImode);
}
[(set_attr "type""jump")
(set_attr "mode""none")
(set (attr "length")
- (if_then_else (match_test "TARGET_DENSITY
- && INTVAL (operands[1]) == 0x7FFF")
- (const_int 5)
- (const_int 6)))])
+ (if_then_else (match_test "(TARGET_DENSITY && INTVAL (operands[1]) ==
0x7FFF)
+ && INTVAL (operands[2]) == 0")
+ (const_int 4)
+ (if_then_else (match_test "TARGET_DENSITY
+&& (INTVAL (operands[1]) ==
0x7FFF
+|| INTVAL (operands[2]) ==
0)")
+ (const_int 5)
+ (const_int 6])
(define_insn_and_split "*masktrue_const_negative_pow2"
[(set (pc)
- (if_then_else (match_operator 3 "boolean_operator"
+ (if_then_else (match_operator 4 "boolean_operator"
[(and:SI (match_operand:SI 0 "register_operand" "r")
(match_operand:SI 1 "const_int_operand" "i"))
-(const_int 0)])
- (label_ref (match_operand 2 "" ""))
+(match_operand:SI 2 "const_int_operand" "i")])
+ (label_ref (match_operand 3 "" ""))
(pc)))]
- "IN_RANGE (exact_log2 (-INTVAL (operands[1])), 12, 30)"
+ "IN_RANGE (exact_log2 (-INTVAL (operands[1])), 1, 30)
+ /* && (~INTVAL (operands[1]) &
