Re: [PATCH v1] Match: Extract integer_types_ternary_match helper to avoid code dup [NFC]
On Sat, May 18, 2024, 9:17 PM wrote:
> From: Pan Li
>
> There are sorts of match pattern for SAT related cases, there will be
> some duplicated code to check the dest, op_0, op_1 are same tree types.
> Aka ternary tree type matches. Thus, extract one helper function to
> do this and avoid match code duplication.
>
> The below test suites are passed for this patch:
> * The rv64gcv fully regression test.
> * The x86 bootstrap test.
> * The x86 regression test.
>
> gcc/ChangeLog:
>
> * generic-match-head.cc (integer_types_ternary_match): New helper
> function to check tenary tree type matches or not.
> * gimple-match-head.cc (integer_types_ternary_match): Ditto but
> for match.
> * match.pd: Leverage above helper function to avoid code dup.
>
> Signed-off-by: Pan Li
> ---
> gcc/generic-match-head.cc | 17 +
> gcc/gimple-match-head.cc | 17 +
> gcc/match.pd | 25 +
> 3 files changed, 39 insertions(+), 20 deletions(-)
>
> diff --git a/gcc/generic-match-head.cc b/gcc/generic-match-head.cc
> index 0d3f648fe8d..cdd48c7a5cc 100644
> --- a/gcc/generic-match-head.cc
> +++ b/gcc/generic-match-head.cc
> @@ -59,6 +59,23 @@ types_match (tree t1, tree t2)
>return TYPE_MAIN_VARIANT (t1) == TYPE_MAIN_VARIANT (t2);
> }
>
> +/* Routine to determine if the types T1, T2 and T3 are effectively
> + the same integer type for GENERIC. If T1, T2 or T3 is not a type,
> + the test applies to their TREE_TYPE. */
> +
> +static inline bool
> +integer_types_ternary_match (tree t1, tree t2, tree t3)
> +{
> + t1 = TYPE_P (t1) ? t1 : TREE_TYPE (t1);
> + t2 = TYPE_P (t2) ? t2 : TREE_TYPE (t2);
> + t3 = TYPE_P (t3) ? t3 : TREE_TYPE (t3);
> +
> + if (!INTEGRAL_TYPE_P (t1) || !INTEGRAL_TYPE_P (t2) || !INTEGRAL_TYPE_P
> (t3))
> +return false;
> +
> + return types_match (t1, t2) && types_match (t1, t3);
> +}
> +
> /* Return if T has a single use. For GENERIC, we assume this is
> always true. */
>
> diff --git a/gcc/gimple-match-head.cc b/gcc/gimple-match-head.cc
> index 5f8a1a1ad8e..91f2e56b8ef 100644
> --- a/gcc/gimple-match-head.cc
> +++ b/gcc/gimple-match-head.cc
> @@ -79,6 +79,23 @@ types_match (tree t1, tree t2)
>return types_compatible_p (t1, t2);
> }
>
> +/* Routine to determine if the types T1, T2 and T3 are effectively
> + the same integer type for GIMPLE. If T1, T2 or T3 is not a type,
> + the test applies to their TREE_TYPE. */
> +
> +static inline bool
> +integer_types_ternary_match (tree t1, tree t2, tree t3)
> +{
> + t1 = TYPE_P (t1) ? t1 : TREE_TYPE (t1);
> + t2 = TYPE_P (t2) ? t2 : TREE_TYPE (t2);
> + t3 = TYPE_P (t3) ? t3 : TREE_TYPE (t3);
> +
> + if (!INTEGRAL_TYPE_P (t1) || !INTEGRAL_TYPE_P (t2) || !INTEGRAL_TYPE_P
> (t3))
> +return false;
> +
> + return types_match (t1, t2) && types_match (t1, t3);
> +}
> +
> /* Return if T has a single use. For GIMPLE, we also allow any
> non-SSA_NAME (ie constants) and zero uses to cope with uses
> that aren't linked up yet. */
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 0f9c34fa897..b291e34bbe4 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -3046,38 +3046,23 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> /* Unsigned Saturation Add */
> (match (usadd_left_part_1 @0 @1)
> (plus:c @0 @1)
> - (if (INTEGRAL_TYPE_P (type)
> - && TYPE_UNSIGNED (TREE_TYPE (@0))
> - && types_match (type, TREE_TYPE (@0))
> - && types_match (type, TREE_TYPE (@1)
> + (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0,
> @1
>
Even though unsigned might be the cheaper check, you might need to swap the
order back to where it was so you check integral first.
Otherwise this is nice cleanup. (Note I can't approve it though).
Thanks,
Andrew
> (match (usadd_left_part_2 @0 @1)
> (realpart (IFN_ADD_OVERFLOW:c @0 @1))
> - (if (INTEGRAL_TYPE_P (type)
> - && TYPE_UNSIGNED (TREE_TYPE (@0))
> - && types_match (type, TREE_TYPE (@0))
> - && types_match (type, TREE_TYPE (@1)
> + (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0,
> @1
>
> (match (usadd_right_part_1 @0 @1)
> (negate (convert (lt (plus:c @0 @1) @0)))
> - (if (INTEGRAL_TYPE_P (type)
> - && TYPE_UNSIGNED (TREE_TYPE (@0))
> - && types_match (type, TREE_TYPE (@0))
> - && types_match (type, TREE_TYPE (@1)
> + (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0,
> @1
>
> (match (usadd_right_part_1 @0 @1)
> (negate (convert (gt @0 (plus:c @0 @1
> - (if (INTEGRAL_TYPE_P (type)
> - && TYPE_UNSIGNED (TREE_TYPE (@0))
> - && types_match (type, TREE_TYPE (@0))
> - && types_match (type, TREE_TYPE (@1)
> + (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0,
> @1
>
> (match (usadd_right_part_2 @0 @1)
> (negate (convert (ne (imagpart (IFN_ADD_OVERFLOW:c @0 @1))
> integer_zerop)))
> - (if (INTEGRAL_TYPE_P (type)
> - &&
[PATCH v1] Match: Extract integer_types_ternary_match helper to avoid code dup [NFC]
From: Pan Li
There are sorts of match pattern for SAT related cases, there will be
some duplicated code to check the dest, op_0, op_1 are same tree types.
Aka ternary tree type matches. Thus, extract one helper function to
do this and avoid match code duplication.
The below test suites are passed for this patch:
* The rv64gcv fully regression test.
* The x86 bootstrap test.
* The x86 regression test.
gcc/ChangeLog:
* generic-match-head.cc (integer_types_ternary_match): New helper
function to check tenary tree type matches or not.
* gimple-match-head.cc (integer_types_ternary_match): Ditto but
for match.
* match.pd: Leverage above helper function to avoid code dup.
Signed-off-by: Pan Li
---
gcc/generic-match-head.cc | 17 +
gcc/gimple-match-head.cc | 17 +
gcc/match.pd | 25 +
3 files changed, 39 insertions(+), 20 deletions(-)
diff --git a/gcc/generic-match-head.cc b/gcc/generic-match-head.cc
index 0d3f648fe8d..cdd48c7a5cc 100644
--- a/gcc/generic-match-head.cc
+++ b/gcc/generic-match-head.cc
@@ -59,6 +59,23 @@ types_match (tree t1, tree t2)
return TYPE_MAIN_VARIANT (t1) == TYPE_MAIN_VARIANT (t2);
}
+/* Routine to determine if the types T1, T2 and T3 are effectively
+ the same integer type for GENERIC. If T1, T2 or T3 is not a type,
+ the test applies to their TREE_TYPE. */
+
+static inline bool
+integer_types_ternary_match (tree t1, tree t2, tree t3)
+{
+ t1 = TYPE_P (t1) ? t1 : TREE_TYPE (t1);
+ t2 = TYPE_P (t2) ? t2 : TREE_TYPE (t2);
+ t3 = TYPE_P (t3) ? t3 : TREE_TYPE (t3);
+
+ if (!INTEGRAL_TYPE_P (t1) || !INTEGRAL_TYPE_P (t2) || !INTEGRAL_TYPE_P (t3))
+return false;
+
+ return types_match (t1, t2) && types_match (t1, t3);
+}
+
/* Return if T has a single use. For GENERIC, we assume this is
always true. */
diff --git a/gcc/gimple-match-head.cc b/gcc/gimple-match-head.cc
index 5f8a1a1ad8e..91f2e56b8ef 100644
--- a/gcc/gimple-match-head.cc
+++ b/gcc/gimple-match-head.cc
@@ -79,6 +79,23 @@ types_match (tree t1, tree t2)
return types_compatible_p (t1, t2);
}
+/* Routine to determine if the types T1, T2 and T3 are effectively
+ the same integer type for GIMPLE. If T1, T2 or T3 is not a type,
+ the test applies to their TREE_TYPE. */
+
+static inline bool
+integer_types_ternary_match (tree t1, tree t2, tree t3)
+{
+ t1 = TYPE_P (t1) ? t1 : TREE_TYPE (t1);
+ t2 = TYPE_P (t2) ? t2 : TREE_TYPE (t2);
+ t3 = TYPE_P (t3) ? t3 : TREE_TYPE (t3);
+
+ if (!INTEGRAL_TYPE_P (t1) || !INTEGRAL_TYPE_P (t2) || !INTEGRAL_TYPE_P (t3))
+return false;
+
+ return types_match (t1, t2) && types_match (t1, t3);
+}
+
/* Return if T has a single use. For GIMPLE, we also allow any
non-SSA_NAME (ie constants) and zero uses to cope with uses
that aren't linked up yet. */
diff --git a/gcc/match.pd b/gcc/match.pd
index 0f9c34fa897..b291e34bbe4 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3046,38 +3046,23 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
/* Unsigned Saturation Add */
(match (usadd_left_part_1 @0 @1)
(plus:c @0 @1)
- (if (INTEGRAL_TYPE_P (type)
- && TYPE_UNSIGNED (TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@1)
+ (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0, @1
(match (usadd_left_part_2 @0 @1)
(realpart (IFN_ADD_OVERFLOW:c @0 @1))
- (if (INTEGRAL_TYPE_P (type)
- && TYPE_UNSIGNED (TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@1)
+ (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0, @1
(match (usadd_right_part_1 @0 @1)
(negate (convert (lt (plus:c @0 @1) @0)))
- (if (INTEGRAL_TYPE_P (type)
- && TYPE_UNSIGNED (TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@1)
+ (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0, @1
(match (usadd_right_part_1 @0 @1)
(negate (convert (gt @0 (plus:c @0 @1
- (if (INTEGRAL_TYPE_P (type)
- && TYPE_UNSIGNED (TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@1)
+ (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0, @1
(match (usadd_right_part_2 @0 @1)
(negate (convert (ne (imagpart (IFN_ADD_OVERFLOW:c @0 @1)) integer_zerop)))
- (if (INTEGRAL_TYPE_P (type)
- && TYPE_UNSIGNED (TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@0))
- && types_match (type, TREE_TYPE (@1)
+ (if (TYPE_UNSIGNED (type) && integer_types_ternary_match (type, @0, @1
/* We cannot merge or overload usadd_left_part_1 and usadd_left_part_2
because the sub part of left_part_2 cannot work with right_part_1.
--
2.34.1
[to-be-committed][RISC-V][PR target/115142] Do not create invalidate shift-add insn
Repost, this time with the RISC-V tag so it's picked up by the CI system.
This fixes a minor bug that showed up in the CI system, presumably with
fuzz testing.
Under the right circumstances, we could end trying to emit a shift-add
style sequence where the to-be-shifted operand was not a register. This
naturally leads to an unrecognized insn.
The circumstances which triggered this weren't something that should
appear in the wild (-ftree-ter, without optimization enabled). So I
wasn't planning to backport. Obviously if it shows up in another
context we can revisit that decision.
PR target/115142
gcc/
* config/riscv/riscv.cc (mem_shadd_or_shadd_rtx_p): Make sure
shifted argument is a register.
gcc/testsuite
* gcc.target/riscv/pr115142.c: New test.
I've run this through my rv32gcv and rv64gc tester. Waiting on the CI
system before committing.
jeff
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 7a34b4be873..d0c22058b8c 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2465,6 +2465,7 @@ mem_shadd_or_shadd_rtx_p (rtx x)
{
return ((GET_CODE (x) == ASHIFT
|| GET_CODE (x) == MULT)
+ && register_operand (XEXP (x, 0), GET_MODE (x))
&& CONST_INT_P (XEXP (x, 1))
&& ((GET_CODE (x) == ASHIFT && IN_RANGE (INTVAL (XEXP (x, 1)), 1, 3))
|| (GET_CODE (x) == MULT
diff --git a/gcc/testsuite/gcc.target/riscv/pr115142.c
b/gcc/testsuite/gcc.target/riscv/pr115142.c
new file mode 100644
index 000..40ba49dfa20
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr115142.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O0 -ftree-ter" } */
+
+long a;
+char b;
+void e() {
+ char f[8][1];
+ b = f[a][a];
+}
+
RE: [PATCH] Add widening expansion of MULT_HIGHPART_EXPR for integral modes
Hi Botcazou,
Just notice that this patch may result in some ICE when build libc++ for the
riscv port, details as below.
Please note not all configuration can reproduce this issue, feel free to ping
me if you cannot reproduce this issue. CC more riscv port people for awareness.
during GIMPLE pass: slp
In file included from
/home/box/panli/gnu-toolchain/gcc/libstdc++-v3/src/c++17/floating_to_chars.cc:124:
/home/box/panli/gnu-toolchain/gcc/libstdc++-v3/src/c++17/ryu/generic_128.c: In
function 'int
{anonymous}::ryu::generic128::generic_to_chars(floating_decimal_128, char*)':
/home/box/panli/gnu-toolchain/gcc/libstdc++-v3/src/c++17/ryu/generic_128.c:251:5:
internal compiler error: in require, at machmode.h:313
251 | int generic_to_chars(const struct floating_decimal_128 v, char* const
result) {
| ^~~~
0x30d526b diagnostic_impl(rich_location*, diagnostic_metadata const*, int, char
const*, __va_list_tag (*) [1], diagnostic_t)
???:0
0x30d637e internal_error(char const*, ...)
???:0
0xdd9c9d fancy_abort(char const*, int, char const*)
???:0
0xbb4f9f can_mult_highpart_p(machine_mode, bool) [clone .cold]
???:0
0x17384b3 default_preferred_div_as_shifts_over_mult(tree_node const*)
???:0
0x2b34967 vect_recog_divmod_pattern(vec_info*, _stmt_vec_info*, tree_node**)
???:0
0x2b2f106 vect_pattern_recog_1(vec_info*, vect_recog_func*, _stmt_vec_info*)
???:0
0x2b2f3b1 vect_pattern_recog(vec_info*)
???:0
0x1a5e403 vect_slp_region(vec,
vec, vec*, unsigned
int, loop*)
???:0
0x1a60505 vect_slp_bbs(vec const&, loop*)
???:0
0x1a608f3 vect_slp_function(function*)
???:0
0x1a6c2a1 (anonymous namespace)::pass_slp_vectorize::execute(function*)
???:0
Please submit a full bug report, with preprocessed source (by using
-freport-bug).
Please include the complete backtrace with any bug report.
Pan
-Original Message-
From: Eric Botcazou
Sent: Tuesday, April 30, 2024 12:11 AM
To: gcc-patches@gcc.gnu.org
Subject: [PATCH] Add widening expansion of MULT_HIGHPART_EXPR for integral modes
Hi,
for integral modes, the expansion of MULT_HIGHPART_EXPR requires the presence
of an {s,u}mul_highpart optab whereas, for vector modes, widening expansion is
supported. This adds a widening expansion for integral modes too, which is in
fact already implemented in expmed_mult_highpart_optab. We'll use that in a
subsequent change to the Ada front-end to generate fast modulo reduction for
modular types with nonbinary modulus (a little controversial Ada 95 feature).
Tested on x86-64/Linux, OK for the mainline?
2024-04-29 Eric Botcazou
* expmed.h (expmed_mult_highpart_optab): Declare.
* expmed.cc (expmed_mult_highpart_optab): Remove static keyword.
Do not assume that OP1 is a constant integer. Fix pasto.
(expmed_mult_highpart): Pass OP1 narrowed to MODE in all the calls
to expmed_mult_highpart_optab.
* optabs-query.cc (can_mult_highpart_p): Use 2 for integer widening
and shift subsequent values accordingly.
* optabs.cc (expand_mult_highpart): Call expmed_mult_highpart_optab
when can_mult_highpart_p returns 2 and adjust to above change.
--
Eric Botcazou
[PATCH] PHIOPT: Don't transform minmax if middle bb contains a phi [PR115143]
The problem here is even if last_and_only_stmt returns a statement,
the bb might still contain a phi node which defines a ssa name
which is used in that statement so we need to add a check to make sure
that the phi nodes are empty for the middle bbs in both the
`CMP?MINMAX:MINMAX` case and the `CMP?MINMAX:B` cases.
OK for trunk and backport to all open branches since r14-3827-g30e6ee074588ba
was backported?
Bootstrapped and tested on x86_64_linux-gnu with no regressions.
PR tree-optimization/115143
gcc/ChangeLog:
* tree-ssa-phiopt.cc (minmax_replacement): Check for empty
phi nodes for middle bbs for the case where middle bb is not empty.
gcc/testsuite/ChangeLog:
* gcc.c-torture/compile/pr115143-1.c: New test.
* gcc.c-torture/compile/pr115143-2.c: New test.
* gcc.c-torture/compile/pr115143-3.c: New test.
Signed-off-by: Andrew Pinski
---
.../gcc.c-torture/compile/pr115143-1.c| 21 +
.../gcc.c-torture/compile/pr115143-2.c| 30 +++
.../gcc.c-torture/compile/pr115143-3.c| 29 ++
gcc/tree-ssa-phiopt.cc| 12
4 files changed, 92 insertions(+)
create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr115143-1.c
create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr115143-2.c
create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr115143-3.c
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr115143-1.c
b/gcc/testsuite/gcc.c-torture/compile/pr115143-1.c
new file mode 100644
index 000..5cb119ea432
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr115143-1.c
@@ -0,0 +1,21 @@
+/* PR tree-optimization/115143 */
+/* This used to ICE.
+ minmax part of phiopt would transform,
+ would transform `a!=0?min(a, b) : 0` into `min(a,b)`
+ which was correct except b was defined by a phi in the inner
+ bb which was not handled. */
+short a, d;
+char b;
+long c;
+unsigned long e, f;
+void g(unsigned long h) {
+ if (c ? e : b)
+if (e)
+ if (d) {
+a = f ? ({
+ unsigned long i = d ? f : 0, j = e ? h : 0;
+ i < j ? i : j;
+}) : 0;
+ }
+}
+
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr115143-2.c
b/gcc/testsuite/gcc.c-torture/compile/pr115143-2.c
new file mode 100644
index 000..05c3bbe9738
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr115143-2.c
@@ -0,0 +1,30 @@
+/* { dg-options "-fgimple" } */
+/* PR tree-optimization/115143 */
+/* This used to ICE.
+ minmax part of phiopt would transform,
+ would transform `a!=0?min(a, b) : 0` into `min(a,b)`
+ which was correct except b was defined by a phi in the inner
+ bb which was not handled. */
+unsigned __GIMPLE (ssa,startwith("phiopt"))
+foo (unsigned a, unsigned b)
+{
+ unsigned j;
+ unsigned _23;
+ unsigned _12;
+
+ __BB(2):
+ if (a_6(D) != 0u)
+goto __BB3;
+ else
+goto __BB4;
+
+ __BB(3):
+ j_10 = __PHI (__BB2: b_11(D));
+ _23 = __MIN (a_6(D), j_10);
+ goto __BB4;
+
+ __BB(4):
+ _12 = __PHI (__BB3: _23, __BB2: 0u);
+ return _12;
+
+}
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr115143-3.c
b/gcc/testsuite/gcc.c-torture/compile/pr115143-3.c
new file mode 100644
index 000..53c5fb5588e
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr115143-3.c
@@ -0,0 +1,29 @@
+/* { dg-options "-fgimple" } */
+/* PR tree-optimization/115143 */
+/* This used to ICE.
+ minmax part of phiopt would transform,
+ would transform `a!=0?min(a, b) : 0` into `min(a,b)`
+ which was correct except b was defined by a phi in the inner
+ bb which was not handled. */
+unsigned __GIMPLE (ssa,startwith("phiopt"))
+foo (unsigned a, unsigned b)
+{
+ unsigned j;
+ unsigned _23;
+ unsigned _12;
+
+ __BB(2):
+ if (a_6(D) > 0u)
+goto __BB3;
+ else
+goto __BB4;
+
+ __BB(3):
+ j_10 = __PHI (__BB2: b_7(D));
+ _23 = __MIN (a_6(D), j_10);
+ goto __BB4;
+
+ __BB(4):
+ _12 = __PHI (__BB3: _23, __BB2: 0u);
+ return _12;
+}
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index f166c3132cb..918cf50b589 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -1925,6 +1925,10 @@ minmax_replacement (basic_block cond_bb, basic_block
middle_bb, basic_block alt_
|| gimple_code (assign) != GIMPLE_ASSIGN)
return false;
+ /* There cannot be any phi nodes in the middle bb. */
+ if (!gimple_seq_empty_p (phi_nodes (middle_bb)))
+ return false;
+
lhs = gimple_assign_lhs (assign);
ass_code = gimple_assign_rhs_code (assign);
if (ass_code != MAX_EXPR && ass_code != MIN_EXPR)
@@ -1938,6 +1942,10 @@ minmax_replacement (basic_block cond_bb, basic_block
middle_bb, basic_block alt_
|| gimple_code (assign) != GIMPLE_ASSIGN)
return false;
+ /* There cannot be any phi nodes in the alt middle bb. */
+ if (!gimple_seq_empty_p (phi_nodes (alt_middle_bb)))
+ return false;
+
alt_lhs = gimple_assign_lhs
[to-be-committed][PR target/115142] Do not create invalidate shift-add insn
This fixes a minor bug that showed up in the CI system, presumably with
fuzz testing.
Under the right circumstances, we could end trying to emit a shift-add
style sequence where the to-be-shifted operand was not a register. This
naturally leads to an unrecognized insn.
The circumstances which triggered this weren't something that should
appear in the wild (-ftree-ter, without optimization enabled). So I
wasn't planning to backport. Obviously if it shows up in another
context we can revisit that decision.
PR target/115142
gcc/
* config/riscv/riscv.cc (mem_shadd_or_shadd_rtx_p): Make sure
shifted argument is a register.
gcc/testsuite
* gcc.target/riscv/pr115142.c: New test.
I've run this through my rv32gcv and rv64gc tester. Waiting on the CI
system before committing.
jeffdiff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 7a34b4be873..d0c22058b8c 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2465,6 +2465,7 @@ mem_shadd_or_shadd_rtx_p (rtx x)
{
return ((GET_CODE (x) == ASHIFT
|| GET_CODE (x) == MULT)
+ && register_operand (XEXP (x, 0), GET_MODE (x))
&& CONST_INT_P (XEXP (x, 1))
&& ((GET_CODE (x) == ASHIFT && IN_RANGE (INTVAL (XEXP (x, 1)), 1, 3))
|| (GET_CODE (x) == MULT
diff --git a/gcc/testsuite/gcc.target/riscv/pr115142.c
b/gcc/testsuite/gcc.target/riscv/pr115142.c
new file mode 100644
index 000..40ba49dfa20
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr115142.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O0 -ftree-ter" } */
+
+long a;
+char b;
+void e() {
+ char f[8][1];
+ b = f[a][a];
+}
+
[C PATCH] Fix for redeclared enumerator initialized with different type [PR115109]
Bootstrapped and regression tested on x86_64
c23: Fix for redeclared enumerator initialized with different type
[PR115109]
c23 specifies that the type of a redeclared enumerator is the one of the
previous declaration. Convert initializers with different type accordingly
and add -Woverflow warning.
2024-05-18 Martin Uecker
PR c/115109
gcc/c/
* c-decl.cc (build_enumerator): When redeclaring an
enumerator convert value to previous type.
gcc/testsuite/
* gcc.dg/pr115109.c: New test.
* gcc.dg/c23-tag-enum-6.c: New test.
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index b691b91b3db..08a51c7ad50 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -10209,6 +10209,7 @@ build_enumerator (location_t decl_loc, location_t loc,
struct c_enum_contents *the_enum, tree name, tree value)
{
tree decl;
+ tree old_decl;
/* Validate and default VALUE. */
@@ -10268,6 +10269,25 @@ build_enumerator (location_t decl_loc, location_t loc,
definition. */
value = convert (the_enum->enum_type, value);
}
+ else if (flag_isoc23
+ && (old_decl = lookup_name_in_scope (name, current_scope))
+ && old_decl != error_mark_node
+ && TREE_TYPE (old_decl)
+ && TREE_TYPE (TREE_TYPE (old_decl))
+ && TREE_CODE (old_decl) == CONST_DECL)
+{
+ tree previous_type = TREE_TYPE (TREE_TYPE (old_decl));
+
+ if (!int_fits_type_p (value, previous_type))
+ {
+ warning_at (loc, OPT_Woverflow,
+ "value of redeclared enumerator outside the range of "
+ "the previous type %qT", previous_type);
+ locate_old_decl (old_decl);
+ }
+
+ value = convert (previous_type, value);
+}
else
{
/* Even though the underlying type of an enum is unspecified, the
diff --git a/gcc/testsuite/gcc.dg/c23-tag-enum-6.c
b/gcc/testsuite/gcc.dg/c23-tag-enum-6.c
new file mode 100644
index 000..ff9ec89775e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/c23-tag-enum-6.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c23" } */
+
+#include
+
+enum E : int { a = 1, b = 2 };
+enum E : int { b = _Generic(a, enum E: 2), a = 1 };
+
+enum H { x = 1 };
+enum H { x = 2UL + UINT_MAX }; /* { dg-warning "outside the range" } */
+
+enum K : int { z = 1 };
+enum K : int { z = 2UL + UINT_MAX }; /* { dg-error "outside the range" } */
+
diff --git a/gcc/testsuite/gcc.dg/pr115109.c b/gcc/testsuite/gcc.dg/pr115109.c
new file mode 100644
index 000..0c327ce1697
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr115109.c
@@ -0,0 +1,6 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c23" } */
+
+enum E { a = 1L, b = 2 };
+enum E { a = 1L, b = _Generic(a, enum E: 2) };
+
Re: [PATCH] Optab: add isfinite_optab for __builtin_isfinite
On Thu, Apr 11, 2024 at 8:07 PM HAO CHEN GUI wrote: > > Hi, > This patch adds an optab for __builtin_isfinite. The finite check can be > implemented on rs6000 by a single instruction. It needs an optab to be > expanded to the certain sequence of instructions. > > The subsequent patches will implement the expand on rs6000. > > Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no > regressions. Is this OK for next stage-1? This is missing adding documentation for the new optab. It should be documented in md.texi under `Standard Pattern Names For Generation` section. Thanks, Andrew > > Thanks > Gui Haochen > > ChangeLog > optab: Add isfinite_optab for isfinite builtin > > gcc/ > * builtins.cc (interclass_mathfn_icode): Set optab to isfinite_optab > for isfinite builtin. > * optabs.def (isfinite_optab): New. > > patch.diff > diff --git a/gcc/builtins.cc b/gcc/builtins.cc > index d2786f207b8..5262aa01660 100644 > --- a/gcc/builtins.cc > +++ b/gcc/builtins.cc > @@ -2459,8 +2459,9 @@ interclass_mathfn_icode (tree arg, tree fndecl) >errno_set = true; builtin_optab = ilogb_optab; break; > CASE_FLT_FN (BUILT_IN_ISINF): >builtin_optab = isinf_optab; break; > -case BUILT_IN_ISNORMAL: > case BUILT_IN_ISFINITE: > + builtin_optab = isfinite_optab; break; > +case BUILT_IN_ISNORMAL: > CASE_FLT_FN (BUILT_IN_FINITE): > case BUILT_IN_FINITED32: > case BUILT_IN_FINITED64: > diff --git a/gcc/optabs.def b/gcc/optabs.def > index ad14f9328b9..dcd77315c2a 100644 > --- a/gcc/optabs.def > +++ b/gcc/optabs.def > @@ -352,6 +352,7 @@ OPTAB_D (fmod_optab, "fmod$a3") > OPTAB_D (hypot_optab, "hypot$a3") > OPTAB_D (ilogb_optab, "ilogb$a2") > OPTAB_D (isinf_optab, "isinf$a2") > +OPTAB_D (isfinite_optab, "isfinite$a2") > OPTAB_D (issignaling_optab, "issignaling$a2") > OPTAB_D (ldexp_optab, "ldexp$a3") > OPTAB_D (log10_optab, "log10$a2")
Re: [PATCH] Optab: add isnormal_optab for __builtin_isnormal
On Fri, Apr 12, 2024 at 1:10 AM HAO CHEN GUI wrote: > > Hi, > This patch adds an optab for __builtin_isnormal. The normal check can be > implemented on rs6000 by a single instruction. It needs an optab to be > expanded to the certain sequence of instructions. > > The subsequent patches will implement the expand on rs6000. > > Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no > regressions. Is this OK for next stage-1? This is missing adding documentation for the new optab. It should be documented in md.texi under `Standard Pattern Names For Generation` section. Thanks, Andrew > > Thanks > Gui Haochen > ChangeLog > optab: Add isnormal_optab for isnormal builtin > > gcc/ > * builtins.cc (interclass_mathfn_icode): Set optab to isnormal_optab > for isnormal builtin. > * optabs.def (isnormal_optab): New. > > patch.diff > diff --git a/gcc/builtins.cc b/gcc/builtins.cc > index 3174f52ebe8..defb39de95f 100644 > --- a/gcc/builtins.cc > +++ b/gcc/builtins.cc > @@ -2462,6 +2462,7 @@ interclass_mathfn_icode (tree arg, tree fndecl) > case BUILT_IN_ISFINITE: >builtin_optab = isfinite_optab; break; > case BUILT_IN_ISNORMAL: > + builtin_optab = isnormal_optab; break; > CASE_FLT_FN (BUILT_IN_FINITE): > case BUILT_IN_FINITED32: > case BUILT_IN_FINITED64: > diff --git a/gcc/optabs.def b/gcc/optabs.def > index dcd77315c2a..3c401fc0b4c 100644 > --- a/gcc/optabs.def > +++ b/gcc/optabs.def > @@ -353,6 +353,7 @@ OPTAB_D (hypot_optab, "hypot$a3") > OPTAB_D (ilogb_optab, "ilogb$a2") > OPTAB_D (isinf_optab, "isinf$a2") > OPTAB_D (isfinite_optab, "isfinite$a2") > +OPTAB_D (isnormal_optab, "isnormal$a2") > OPTAB_D (issignaling_optab, "issignaling$a2") > OPTAB_D (ldexp_optab, "ldexp$a3") > OPTAB_D (log10_optab, "log10$a2")
Re: [C PATCH] Fix for some variably modified types not being recognized [PR114831]
(correct email)
> We did not propagate C_TYPE_VARIABLY_MODIFIED to pointers in all
> cases. I added this directly in two places, but maybe we should
> check all cases of build_pointer_type or integrate this into
> c_build_pointer_type and use this everywhere (but I do not fully
> understand the pointer mode logic there).
>
>
> Bootstrapped and regession tested on x86_64.
>
>
> c: Fix for some variably modified types not being recognized [PR114831]
>
> We did not evaluate expressions with variably modified types correctly
> in typeof and did not produce warnings when jumping over declarations
> using typeof. After addressof or array-to-pointer decay we construct
> new pointer types that have to be marked variably modified if the pointer
> target is variably modified.
>
> 2024-05-18 Martin Uecker
>
> PR c/114831
> gcc/c/
> * c-typeck.cc (array_to_pointer_conversion, build_unary_op):
> Propagate flag to pointer target.
>
> gcc/testsuite/
> * gcc.dg/pr114831-1.c: New test.
> * gcc.dg/pr114831-2.c: New test.
> * gcc.dg/gnu23-varmod-1.c: New test.
> * gcc.dg/gnu23-varmod-2.c: New test.
>
> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
> index 7ecca9f58c6..2d092357e0f 100644
> --- a/gcc/c/c-typeck.cc
> +++ b/gcc/c/c-typeck.cc
> @@ -1891,8 +1891,12 @@ array_to_pointer_conversion (location_t loc, tree exp)
>
>copy_warning (exp, orig_exp);
>
> + bool varmod = C_TYPE_VARIABLY_MODIFIED (restype);
> +
>ptrtype = build_pointer_type (restype);
>
> + C_TYPE_VARIABLY_MODIFIED (ptrtype) = varmod;
> +
>if (INDIRECT_REF_P (exp))
> return convert (ptrtype, TREE_OPERAND (exp, 0));
>
> @@ -4630,6 +4634,7 @@ build_unary_op (location_t location, enum tree_code
> code, tree xarg,
>tree eptype = NULL_TREE;
>const char *invalid_op_diag;
>bool int_operands;
> + bool varmod;
>
>int_operands = EXPR_INT_CONST_OPERANDS (xarg);
>if (int_operands)
> @@ -5113,8 +5118,12 @@ build_unary_op (location_t location, enum tree_code
> code, tree xarg,
>gcc_assert (TREE_CODE (arg) != COMPONENT_REF
> || !DECL_C_BIT_FIELD (TREE_OPERAND (arg, 1)));
>
> + varmod = C_TYPE_VARIABLY_MODIFIED (argtype);
> +
>argtype = build_pointer_type (argtype);
>
> + C_TYPE_VARIABLY_MODIFIED (argtype) = varmod;
> +
>/* ??? Cope with user tricks that amount to offsetof. Delete this
>when we have proper support for integer constant expressions. */
>val = get_base_address (arg);
> diff --git a/gcc/testsuite/gcc.dg/gnu23-varmod-1.c
> b/gcc/testsuite/gcc.dg/gnu23-varmod-1.c
> new file mode 100644
> index 000..add10d13573
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/gnu23-varmod-1.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile }
> + * { dg-options "-std=gnu23" } */
> +
> +int foo(int n)
> +{
> + int (*a(void))[n] { return 0; };
> + goto err; /* { dg-error "jump into scope" "variably modified" } */
> + typeof((n++,a)) b2;
> +err:
> + return n;
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/gnu23-varmod-2.c
> b/gcc/testsuite/gcc.dg/gnu23-varmod-2.c
> new file mode 100644
> index 000..c36af1d1647
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/gnu23-varmod-2.c
> @@ -0,0 +1,16 @@
> +/* { dg-do run }
> + * { dg-options "-std=gnu23" } */
> +
> +int foo(int n)
> +{
> + int (*a(void))[n] { return 0; };
> + typeof((n++,a)) b2;
> + return n;
> +}
> +
> +int main()
> +{
> + if (2 != foo(1))
> + __builtin_abort();
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/pr114831-1.c
> b/gcc/testsuite/gcc.dg/pr114831-1.c
> new file mode 100644
> index 000..ed30a494b3c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr114831-1.c
> @@ -0,0 +1,27 @@
> +/* { dg-do compile }
> + * { dg-options "-std=c23" } */
> +
> +void f(int n)
> +{
> + int a[n];
> + goto foo; /* { dg-error "jump into scope" "variably modified" } */
> + typeof(a) b1;
> +foo:
> +}
> +
> +void g(int n)
> +{
> + int a2[1][n];
> + goto foo; /* { dg-error "jump into scope" "variably modified" } */
> + typeof((n++,a2)) b2;
> +foo:
> +}
> +
> +void h(int n)
> +{
> + int a[n];
> + typeof(a) b1;
> + goto foo; /* { dg-error "jump into scope" "variably modified" } */
> + typeof() b;
> +foo:
> +}
> diff --git a/gcc/testsuite/gcc.dg/pr114831-2.c
> b/gcc/testsuite/gcc.dg/pr114831-2.c
> new file mode 100644
> index 000..ecfd87988c2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr114831-2.c
> @@ -0,0 +1,16 @@
> +/* { dg-do run }
> + * { dg-options "-std=c23" } */
> +
> +int foo(int n)
> +{
> + int a[1][n];
> + typeof((n++,a)) b2;
> + return n;
> +}
> +
> +int main()
> +{
> + if (2 != foo(1))
> + __builtin_abort();
> +}
> +
>
[C PATCH] Fix for some variably modified types not being recognized [PR114831]
We did not propagate C_TYPE_VARIABLY_MODIFIED to pointers in all
cases. I added this directly in two places, but maybe we should
check all cases of build_pointer_type or integrate this into
c_build_pointer_type and use this everywhere (but I do not fully
understand the pointer mode logic there).
Bootstrapped and regession tested on x86_64.
c: Fix for some variably modified types not being recognized [PR114831]
We did not evaluate expressions with variably modified types correctly
in typeof and did not produce warnings when jumping over declarations
using typeof. After addressof or array-to-pointer decay we construct
new pointer types that have to be marked variably modified if the pointer
target is variably modified.
2024-05-18 Martin Uecker
PR c/114831
gcc/c/
* c-typeck.cc (array_to_pointer_conversion, build_unary_op):
Propagate flag to pointer target.
gcc/testsuite/
* gcc.dg/pr114831-1.c: New test.
* gcc.dg/pr114831-2.c: New test.
* gcc.dg/gnu23-varmod-1.c: New test.
* gcc.dg/gnu23-varmod-2.c: New test.
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 7ecca9f58c6..2d092357e0f 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -1891,8 +1891,12 @@ array_to_pointer_conversion (location_t loc, tree exp)
copy_warning (exp, orig_exp);
+ bool varmod = C_TYPE_VARIABLY_MODIFIED (restype);
+
ptrtype = build_pointer_type (restype);
+ C_TYPE_VARIABLY_MODIFIED (ptrtype) = varmod;
+
if (INDIRECT_REF_P (exp))
return convert (ptrtype, TREE_OPERAND (exp, 0));
@@ -4630,6 +4634,7 @@ build_unary_op (location_t location, enum tree_code code,
tree xarg,
tree eptype = NULL_TREE;
const char *invalid_op_diag;
bool int_operands;
+ bool varmod;
int_operands = EXPR_INT_CONST_OPERANDS (xarg);
if (int_operands)
@@ -5113,8 +5118,12 @@ build_unary_op (location_t location, enum tree_code
code, tree xarg,
gcc_assert (TREE_CODE (arg) != COMPONENT_REF
|| !DECL_C_BIT_FIELD (TREE_OPERAND (arg, 1)));
+ varmod = C_TYPE_VARIABLY_MODIFIED (argtype);
+
argtype = build_pointer_type (argtype);
+ C_TYPE_VARIABLY_MODIFIED (argtype) = varmod;
+
/* ??? Cope with user tricks that amount to offsetof. Delete this
when we have proper support for integer constant expressions. */
val = get_base_address (arg);
diff --git a/gcc/testsuite/gcc.dg/gnu23-varmod-1.c
b/gcc/testsuite/gcc.dg/gnu23-varmod-1.c
new file mode 100644
index 000..add10d13573
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gnu23-varmod-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile }
+ * { dg-options "-std=gnu23" } */
+
+int foo(int n)
+{
+ int (*a(void))[n] { return 0; };
+ goto err; /* { dg-error "jump into scope" "variably modified" } */
+ typeof((n++,a)) b2;
+err:
+ return n;
+}
+
diff --git a/gcc/testsuite/gcc.dg/gnu23-varmod-2.c
b/gcc/testsuite/gcc.dg/gnu23-varmod-2.c
new file mode 100644
index 000..c36af1d1647
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gnu23-varmod-2.c
@@ -0,0 +1,16 @@
+/* { dg-do run }
+ * { dg-options "-std=gnu23" } */
+
+int foo(int n)
+{
+ int (*a(void))[n] { return 0; };
+ typeof((n++,a)) b2;
+ return n;
+}
+
+int main()
+{
+ if (2 != foo(1))
+ __builtin_abort();
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr114831-1.c
b/gcc/testsuite/gcc.dg/pr114831-1.c
new file mode 100644
index 000..ed30a494b3c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr114831-1.c
@@ -0,0 +1,27 @@
+/* { dg-do compile }
+ * { dg-options "-std=c23" } */
+
+void f(int n)
+{
+ int a[n];
+ goto foo; /* { dg-error "jump into scope" "variably modified" } */
+ typeof(a) b1;
+foo:
+}
+
+void g(int n)
+{
+ int a2[1][n];
+ goto foo; /* { dg-error "jump into scope" "variably modified" } */
+ typeof((n++,a2)) b2;
+foo:
+}
+
+void h(int n)
+{
+ int a[n];
+ typeof(a) b1;
+ goto foo; /* { dg-error "jump into scope" "variably modified" } */
+ typeof() b;
+foo:
+}
diff --git a/gcc/testsuite/gcc.dg/pr114831-2.c
b/gcc/testsuite/gcc.dg/pr114831-2.c
new file mode 100644
index 000..ecfd87988c2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr114831-2.c
@@ -0,0 +1,16 @@
+/* { dg-do run }
+ * { dg-options "-std=c23" } */
+
+int foo(int n)
+{
+ int a[1][n];
+ typeof((n++,a)) b2;
+ return n;
+}
+
+int main()
+{
+ if (2 != foo(1))
+ __builtin_abort();
+}
+
[patch,avr,applied] PR115065: Tweak __clzhi2
Applied as obvious, Johann -- Author: Wolfgang Hospital Date: Sat May 18 15:02:51 2024 +0200 AVR: target/115065 - Tweak __clzhi2. The libgcc implementation of __clzhi2 can be tweaked by one cycle in some situations by re-arranging the instructions. It also reduces the WCET by 1 cycle. libgcc/ PR target/115065 * config/avr/lib1funcs.S (__clzhi2): Tweak. diff --git a/libgcc/config/avr/lib1funcs.S b/libgcc/config/avr/lib1funcs.S index 04a4eb01ab4..d48b04747da 100644 --- a/libgcc/config/avr/lib1funcs.S +++ b/libgcc/config/avr/lib1funcs.S @@ -2921,11 +2921,9 @@ DEFUN __clzhi2 clr r26 tst r25 brne 1f -subi r26, -8 or r25, r24 -brne 1f -ldi r24, 16 -ret +breq 0f +subi r26, -8 1: cpi r25, 16 brsh 3f subi r26, -3 @@ -2936,6 +2934,8 @@ DEFUN __clzhi2 mov r24, r26 clr r25 ret +0: ldi r24, 16 +ret ENDF __clzhi2 #endif /* defined (L_clzhi2) */
[pushed] wwwdocs: egcs-1.1: Use 64-bit instead of 64 bit
Another instance I found. With that wwwdocs should be consistent. Pushed. Gerald --- htdocs/egcs-1.1/index.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/egcs-1.1/index.html b/htdocs/egcs-1.1/index.html index 5db4e342..a62ed3df 100644 --- a/htdocs/egcs-1.1/index.html +++ b/htdocs/egcs-1.1/index.html @@ -37,7 +37,7 @@ in older versions of EGCS: Vastly improved C++ compiler and integrated C++ runtime libraries. Fixes for the /tmp symlink race security problems. - New targets including mips16, arm-thumb and 64 bit PowerPC. + New targets including mips16, arm-thumb and 64-bit PowerPC. Improvements to GNU Fortran (g77) compiler and runtime library made since g77 version 0.5.23. -- 2.45.0
Re: [Patch, aarch64] v6: Preparatory patch to place target independent and,dependent changed code in one file
Hello Alex:
On 16/05/24 10:21 pm, Alex Coplan wrote:
> Hi Ajit,
>
> Thanks a lot for working through the review feedback.
>
> The patch LGTM with the two minor suggested changes below. I can't
> approve the patch, though, so you'll need an OK from Richard S.
>
> Also, I'm not sure if it makes sense to apply the patch in isolation, it
> might make more sense to only apply it in series with follow-up patches to:
> - Finish renaming any bits of the generic code that need renaming (I
>guess we'll want to rename at least ldp_bb_info to something else,
>probably there are other bits too).
> - Move the generic parts out of gcc/config/aarch64 to a .cc file in the
>middle-end.
>
> I'll let Richard S make the final judgement on that. I don't really
> mind either way.
>
> On 15/05/2024 15:06, Ajit Agarwal wrote:
>> Hello Alex/Richard:
>>
>> All review comments are addressed.
>>
>> Common infrastructure of load store pair fusion is divided into target
>> independent and target dependent changed code.
>>
>> Target independent code is the Generic code with pure virtual function
>> to interface between target independent and dependent code.
>>
>> Target dependent code is the implementation of pure virtual function for
>> aarch64 target and the call to target independent code.
>>
>> Bootstrapped and regtested on aarch64-linux-gnu.
>>
>> Thanks & Regards
>> Ajit
>>
>> aarch64: Preparatory patch to place target independent and
>> dependent changed code in one file
>>
>> Common infrastructure of load store pair fusion is divided into target
>> independent and target dependent changed code.
>>
>> Target independent code is the Generic code with pure virtual function
>> to interface betwwen target independent and dependent code.
>>
>> Target dependent code is the implementation of pure virtual function for
>> aarch64 target and the call to target independent code.
>>
>> 2024-05-15 Ajit Kumar Agarwal
>>
>> gcc/ChangeLog:
>>
>> * config/aarch64/aarch64-ldp-fusion.cc: Place target
>> independent and dependent changed code.
>> ---
>> gcc/config/aarch64/aarch64-ldp-fusion.cc | 533 +++
>> 1 file changed, 357 insertions(+), 176 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> b/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> index 1d9caeab05d..429e532ea3b 100644
>> --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> @@ -138,6 +138,225 @@ struct alt_base
>>poly_int64 offset;
>> };
>>
>> +// Virtual base class for load/store walkers used in alias analysis.
>> +struct alias_walker
>> +{
>> + virtual bool conflict_p (int ) const = 0;
>> + virtual insn_info *insn () const = 0;
>> + virtual bool valid () const = 0;
>> + virtual void advance () = 0;
>> +};
>> +
>> +// When querying handle_writeback_opportunities, this enum is used to
>> +// qualify which opportunities we are asking about.
>> +enum class writeback {
>> + // Only those writeback opportunities that arise from existing
>> + // auto-increment accesses.
>> + EXISTING,
>
> Very minor nit: I think an extra blank line here would be nice for readability
> now that the enumerators have comments above.
>
>> + // All writeback opportunities including those that involve folding
>> + // base register updates into a non-writeback pair.
>> + ALL
>> +};
>> +
>
> Can we have a block comment here which describes the purpose of the
> class and how it fits together with the target? Something like the
> following would do:
>
> // This class can be overriden by targets to give a pass that fuses
> // adjacent loads and stores into load/store pair instructions.
> //
> // The target can override the various virtual functions to customize
> // the behaviour of the pass as appropriate for the target.
>
Addressed in v7 of the patch.
>> +struct pair_fusion {
>> + pair_fusion ()
>> + {
>> +calculate_dominance_info (CDI_DOMINATORS);
>> +df_analyze ();
>> +crtl->ssa = new rtl_ssa::function_info (cfun);
>> + };
>> +
>> + // Given:
>> + // - an rtx REG_OP, the non-memory operand in a load/store insn,
>> + // - a machine_mode MEM_MODE, the mode of the MEM in that insn, and
>> + // - a boolean LOAD_P (true iff the insn is a load), then:
>> + // return true if the access should be considered an FP/SIMD access.
>> + // Such accesses are segregated from GPR accesses, since we only want
>> + // to form pairs for accesses that use the same register file.
>> + virtual bool fpsimd_op_p (rtx, machine_mode, bool)
>> + {
>> +return false;
>> + }
>> +
>> + // Return true if we should consider forming pairs from memory
>> + // accesses with operand mode MODE at this stage in compilation.
>> + virtual bool pair_operand_mode_ok_p (machine_mode mode) = 0;
>> +
>> + // Return true iff REG_OP is a suitable register operand for a paired
>> + // memory access, where LOAD_P is true if we're asking about loads and
>> + // false for stores. MODE
Re: [Patch, aarch64] v6: Preparatory patch to place target independent and,dependent changed code in one file
Hello Richard:
On 17/05/24 11:07 pm, Richard Sandiford wrote:
> Ajit Agarwal writes:
>> Hello Alex/Richard:
>>
>> All review comments are addressed.
>>
>> Common infrastructure of load store pair fusion is divided into target
>> independent and target dependent changed code.
>>
>> Target independent code is the Generic code with pure virtual function
>> to interface between target independent and dependent code.
>>
>> Target dependent code is the implementation of pure virtual function for
>> aarch64 target and the call to target independent code.
>>
>> Bootstrapped and regtested on aarch64-linux-gnu.
>>
>> Thanks & Regards
>> Ajit
>
> Thanks for the patch and thanks to Alex for the reviews. The patch
> looks good to me apart from the minor nits below and the comments that
> Alex had. Please post the updated patch for a final ok though.
>
>> aarch64: Preparatory patch to place target independent and
>> dependent changed code in one file
>>
>> Common infrastructure of load store pair fusion is divided into target
>> independent and target dependent changed code.
>>
>> Target independent code is the Generic code with pure virtual function
>> to interface betwwen target independent and dependent code.
>>
>> Target dependent code is the implementation of pure virtual function for
>> aarch64 target and the call to target independent code.
>>
>> 2024-05-15 Ajit Kumar Agarwal
>>
>> gcc/ChangeLog:
>>
>> * config/aarch64/aarch64-ldp-fusion.cc: Place target
>> independent and dependent changed code.
>
> Not sure this is a complete sentence. Maybe:
>
> * config/aarch64/aarch64-ldp-fusion.cc: Factor out a
> target-independent interface and move it to the head of the file.
>
> That technically isn't detailed enough for a changelog entry,
> but IMO we should use it anyway. It's pointless to write the usual
> amount of detail when the code is going to move soon.
>
Addressed in v7 of the patch.
>> ---
>> gcc/config/aarch64/aarch64-ldp-fusion.cc | 533 +++
>> 1 file changed, 357 insertions(+), 176 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> b/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> index 1d9caeab05d..429e532ea3b 100644
>> --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
>> @@ -138,6 +138,225 @@ struct alt_base
>>poly_int64 offset;
>> };
>>
>> +// Virtual base class for load/store walkers used in alias analysis.
>> +struct alias_walker
>> +{
>> + virtual bool conflict_p (int ) const = 0;
>> + virtual insn_info *insn () const = 0;
>> + virtual bool valid () const = 0;
>> + virtual void advance () = 0;
>> +};
>> +
>> +// When querying handle_writeback_opportunities, this enum is used to
>> +// qualify which opportunities we are asking about.
>> +enum class writeback {
>> + // Only those writeback opportunities that arise from existing
>> + // auto-increment accesses.
>> + EXISTING,
>> + // All writeback opportunities including those that involve folding
>
> There should be a comma after "opportunities"
>
>> + // base register updates into a non-writeback pair.
>> + ALL
>> +};
>> +
>> +struct pair_fusion {
>> + pair_fusion ()
>> + {
>> +calculate_dominance_info (CDI_DOMINATORS);
>> +df_analyze ();
>> +crtl->ssa = new rtl_ssa::function_info (cfun);
>> + };
>
> Unnecessary trailing ";". I think it'd be better to define this and
> the destructor out-of-line though. For one thing, it'll reduce the number
> of header file dependencies, once the code is moved to its own header file.
>
Addressed in v7 of the patch.
>> +
>> + // Given:
>> + // - an rtx REG_OP, the non-memory operand in a load/store insn,
>> + // - a machine_mode MEM_MODE, the mode of the MEM in that insn, and
>> + // - a boolean LOAD_P (true iff the insn is a load), then:
>> + // return true if the access should be considered an FP/SIMD access.
>> + // Such accesses are segregated from GPR accesses, since we only want
>> + // to form pairs for accesses that use the same register file.
>> + virtual bool fpsimd_op_p (rtx, machine_mode, bool)
>> + {
>> +return false;
>> + }
>> +
>> + // Return true if we should consider forming pairs from memory
>> + // accesses with operand mode MODE at this stage in compilation.
>> + virtual bool pair_operand_mode_ok_p (machine_mode mode) = 0;
>> +
>> + // Return true iff REG_OP is a suitable register operand for a paired
>> + // memory access, where LOAD_P is true if we're asking about loads and
>> + // false for stores. MODE gives the mode of the operand.
>> + virtual bool pair_reg_operand_ok_p (bool load_p, rtx reg_op,
>> + machine_mode mode) = 0;
>> +
>> + // Return alias check limit.
>> + // This is needed to avoid unbounded quadratic behaviour when
>> + // performing alias analysis.
>> + virtual int pair_mem_alias_check_limit () = 0;
>
> I think the end result should be to make this a
[Patch, aarch64] v7: Preparatory patch to place target independent and dependent changed code in one file
Hello Alex/Richard:
All comments are addressed.
Common infrastructure of load store pair fusion is divided into target
independent and target dependent changed code.
Target independent code is the Generic code with pure virtual function
to interface between target independent and dependent code.
Target dependent code is the implementation of pure virtual function for
aarch64 target and the call to target independent code.
Bootstrapped and regtested on aarch64-linux-gnu.
Thanks & Regards
Ajit
aarch64: Preparatory patch to place target independent and
dependent changed code in one file
Common infrastructure of load store pair fusion is divided into target
independent and target dependent changed code.
Target independent code is the Generic code with pure virtual function
to interface betwwen target independent and dependent code.
Target dependent code is the implementation of pure virtual function for
aarch64 target and the call to target independent code.
2024-05-18 Ajit Kumar Agarwal
gcc/ChangeLog:
* config/aarch64/aarch64-ldp-fusion.cc: Factor out a
target-independent interface and move it to the head of the file
---
gcc/config/aarch64/aarch64-ldp-fusion.cc | 555 +++
1 file changed, 373 insertions(+), 182 deletions(-)
diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc
b/gcc/config/aarch64/aarch64-ldp-fusion.cc
index 1d9caeab05d..e4e55b84f8b 100644
--- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
+++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
@@ -138,6 +138,235 @@ struct alt_base
poly_int64 offset;
};
+// Virtual base class for load/store walkers used in alias analysis.
+struct alias_walker
+{
+ virtual bool conflict_p (int ) const = 0;
+ virtual insn_info *insn () const = 0;
+ virtual bool valid () const = 0;
+ virtual void advance () = 0;
+};
+
+// When querying should_handle_writeback, this enum is used to
+// qualify which opportunities we are asking about.
+enum class writeback {
+ // Only those writeback opportunities that arise from existing
+ // auto-increment accesses.
+ EXISTING,
+
+ // All writeback opportunities including those that involve folding
+ // base register updates into a non-writeback pair.
+ ALL
+};
+
+// This class can be overriden by targets to give a pass that fuses
+// adjacent loads and stores into load/store pair instructions.
+//
+// The target can override the various virtual functions to customize
+// the behaviour of the pass as appropriate for the target.
+struct pair_fusion {
+ pair_fusion ();
+
+ // Given:
+ // - an rtx REG_OP, the non-memory operand in a load/store insn,
+ // - a machine_mode MEM_MODE, the mode of the MEM in that insn, and
+ // - a boolean LOAD_P (true iff the insn is a load), then:
+ // return true if the access should be considered an FP/SIMD access.
+ // Such accesses are segregated from GPR accesses, since we only want
+ // to form pairs for accesses that use the same register file.
+ virtual bool fpsimd_op_p (rtx, machine_mode, bool)
+ {
+return false;
+ }
+
+ // Return true if we should consider forming pairs from memory
+ // accesses with operand mode MODE at this stage in compilation.
+ virtual bool pair_operand_mode_ok_p (machine_mode mode) = 0;
+
+ // Return true iff REG_OP is a suitable register operand for a paired
+ // memory access, where LOAD_P is true if we're asking about loads and
+ // false for stores. MODE gives the mode of the operand.
+ virtual bool pair_reg_operand_ok_p (bool load_p, rtx reg_op,
+ machine_mode mode) = 0;
+
+ // Return alias check limit.
+ // This is needed to avoid unbounded quadratic behaviour when
+ // performing alias analysis.
+ virtual int pair_mem_alias_check_limit () = 0;
+
+ // Return true if we should try to handle writeback opportunities.
+ // WHICH determines the kinds of writeback opportunities the caller
+ // is asking about.
+ virtual bool should_handle_writeback (enum writeback which) = 0;
+
+ // Given BASE_MEM, the mem from the lower candidate access for a pair,
+ // and LOAD_P (true if the access is a load), check if we should proceed
+ // to form the pair given the target's code generation policy on
+ // paired accesses.
+ virtual bool pair_mem_ok_with_policy (rtx base_mem, bool load_p) = 0;
+
+ // Generate the pattern for a paired access. PATS gives the patterns
+ // for the individual memory accesses (which by this point must share a
+ // common base register). If WRITEBACK is non-NULL, then this rtx
+ // describes the update to the base register that should be performed by
+ // the resulting insn. LOAD_P is true iff the accesses are loads.
+ virtual rtx gen_pair (rtx *pats, rtx writeback, bool load_p) = 0;
+
+ // Return true if INSN is a paired memory access. If so, set LOAD_P to
+ // true iff INSN is a load pair.
+ virtual bool pair_mem_insn_p (rtx_insn *insn, bool _p) = 0;
+
+ // Return true if we should track loads.
+ virtual bool
Re: [PATCH] libstdc++: increment *this instead of this
On Sat, May 18, 2024 at 3:25 PM Jakub Jelinek wrote: > On Sat, May 18, 2024 at 02:53:20PM +0800, Kefu Chai wrote: > > libstdc++-v3/ChangeLog: > > > > * include/bits/unicode.h (enable_borrowed_range): Call ++(*this) > > instead of ++this > > This should be already fixed, see https://gcc.gnu.org/PR115119 Thanks Jakub. Indeed. The mirror of the gcc source repo I am using has some latencies. > > Jakub > >
Re: [PATCH] libstdc++: increment *this instead of this
On Sat, May 18, 2024 at 02:53:20PM +0800, Kefu Chai wrote: > libstdc++-v3/ChangeLog: > > * include/bits/unicode.h (enable_borrowed_range): Call ++(*this) > instead of ++this This should be already fixed, see https://gcc.gnu.org/PR115119 Jakub
[PATCH] libstdc++: increment *this instead of this
From: Kefu Chai
in _Grapheme_cluster_view::_Iterator, we implement its post-increment
operator (a++) using its pre-increment opereator (++a). but we use
++this
to call the pre-increment opereator in the implementation of the
post-increment operator. one cannot assign to `this`. both GCC and Clang
error out when compiling this piece of code. like:
:7:9: error: expression is not assignable
7 | ++this;
| ^
and
:7:11: error: increment of read-only location '(Foo*)this'
7 | ++this;
| ^~~~
:7:11: error: lvalue required as increment operand
to address this issue, we use ++(*this) instead. please note, we don't
use the post-increment operator of _Grapheme_cluster_view::_Iterator
in libstdc++ yet. and _Grapheme_cluster_view::_Iterator is not a part
of the public interface exposed by the std::format. this class is used
to estimate the width of formatted text, so this piece of code is not
tested by existing test cases at this moment. the build failure surfaced
when building our C++20 project with the libstdc++ shippped by GCC-14
and with Clang-19 (881f20e9), which, according to its release notes:
Clang now performs semantic analysis for unary operators with dependent
operands that are known to be of non-class non-enumeration type prior
to instantiation.
I guess that's why this issue was not identified until now.
libstdc++-v3/ChangeLog:
* include/bits/unicode.h (enable_borrowed_range): Call ++(*this)
instead of ++this
Signed-off-by: Kefu Chai
---
libstdc++-v3/include/bits/unicode.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libstdc++-v3/include/bits/unicode.h
b/libstdc++-v3/include/bits/unicode.h
index 46238143fb6..12226927b71 100644
--- a/libstdc++-v3/include/bits/unicode.h
+++ b/libstdc++-v3/include/bits/unicode.h
@@ -802,7 +802,7 @@ inline namespace __v15_1_0
operator++(int)
{
auto __tmp = *this;
- ++this;
+ ++(*this);
return __tmp;
}
--
2.45.1
