* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
@ 2023-03-07 9:01 ` rguenth at gcc dot gnu.org
2023-03-27 8:29 ` marxin at gcc dot gnu.org
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-03-07 9:01 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|--- |13.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
2023-03-07 9:01 ` [Bug target/109040] " rguenth at gcc dot gnu.org
@ 2023-03-27 8:29 ` marxin at gcc dot gnu.org
2023-03-27 8:45 ` [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a marxin at gcc dot gnu.org
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: marxin at gcc dot gnu.org @ 2023-03-27 8:29 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
I'm going to bisect that..
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
2023-03-07 9:01 ` [Bug target/109040] " rguenth at gcc dot gnu.org
2023-03-27 8:29 ` marxin at gcc dot gnu.org
@ 2023-03-27 8:45 ` marxin at gcc dot gnu.org
2023-04-04 12:54 ` jakub at gcc dot gnu.org
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: marxin at gcc dot gnu.org @ 2023-03-27 8:45 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
Martin Liška <marxin at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |NEW
CC| |rzinsly at ventanamicro dot com
Summary|[13 Regression] wrong code |[13 Regression] wrong code
|with v16hi compare & mask |with v16hi compare & mask
|on riscv64 at -O2 |on riscv64 at -O2 since
| |r13-4907-g2e886eef7f2b5a
Last reconfirmed| |2023-03-27
Ever confirmed|0 |1
--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r13-4907-g2e886eef7f2b5a.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
` (2 preceding siblings ...)
2023-03-27 8:45 ` [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a marxin at gcc dot gnu.org
@ 2023-04-04 12:54 ` jakub at gcc dot gnu.org
2023-04-04 13:41 ` jakub at gcc dot gnu.org
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2023-04-04 12:54 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jakub at gcc dot gnu.org
--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54808
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54808&action=edit
gcc13-riscv-mvconst.patch
I think it is weird to have multiple mov like patterns.
I've tried to get rid of this by adding a new predicate for
*movsi_internal/*movdi_64bit, which after expansion and until end of split1
accepts the splittable constants and turn the current *mvconst_internal into a
pure splitter.
This doesn't fix the PR though, but I think it would be a good idea anyway.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
` (3 preceding siblings ...)
2023-04-04 12:54 ` jakub at gcc dot gnu.org
@ 2023-04-04 13:41 ` jakub at gcc dot gnu.org
2023-04-04 13:46 ` jakub at gcc dot gnu.org
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2023-04-04 13:41 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |ebotcazou at gcc dot gnu.org
--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That said, I've tried to reproduce this using -O1 -fno-tree-forwprop
unsigned int a;
unsigned short b;
__attribute__((noipa))
unsigned int
foo (void)
{
unsigned int c = a & 0x8084c;
unsigned short d = c;
return d + b;
}
but while the RTL is quite similar in that case, this one works.
The problem with the #c0 testcase is during combine.
We have before combine:
(insn 32 23 34 2 (set (reg:SI 167 [ m ])
(mem/c:SI (reg/v/f:SI 147) [1 m+0 S4 A128])) "pr109040.c":9:30 180
{*movsi_internal}
(expr_list:REG_DEAD (reg/v/f:SI 147)
(nil)))
(insn 34 32 35 2 (set (reg:SI 168)
(const_int 526412 [0x8084c])) "pr109040.c":9:30 176 {*mvconst_internal}
(nil))
(insn 35 34 47 2 (set (reg:SI 166)
(and:SI (reg:SI 167 [ m ])
(reg:SI 168))) "pr109040.c":9:30 95 {andsi3}
(expr_list:REG_DEAD (reg:SI 168)
(expr_list:REG_DEAD (reg:SI 167 [ m ])
(expr_list:REG_EQUAL (and:SI (reg:SI 167 [ m ])
(const_int 526412 [0x8084c]))
(nil)))))
(insn 47 35 39 2 (set (reg:HI 175)
(subreg:HI (reg:SI 166) 0)) "pr109040.c":9:11 181 {*movhi_internal}
(expr_list:REG_DEAD (reg:SI 166)
(nil)))
(insn 39 47 40 2 (set (reg:SI 171)
(zero_extend:SI (reg:HI 175))) "pr109040.c":9:11 111
{*zero_extendhisi2}
(expr_list:REG_DEAD (reg:HI 175)
(nil)))
(insn 40 39 43 2 (set (reg:SI 172)
(leu:SI (reg:SI 171)
(const_int 5 [0x5]))) "pr109040.c":9:11 291 {*sleu_sisi}
(expr_list:REG_DEAD (reg:SI 171)
(nil)))
Now, the zero extension from HImode to SImode of m & 0x8084c would be best
combined as
m & 0x84c, but 0x84c doesn't fit into signed 12-bit immediate for ANDI
instruction.
On the above shorter testcase the major difference before combine is that the
zero_extend
is combined with the subreg, so
(insn 10 9 11 2 (set (reg:SI 148 [ c ])
(zero_extend:SI (subreg:HI (reg:SI 144 [ c ]) 0))) "pr109040-2.c":10:12
111 {*zero_extendhisi2}
(expr_list:REG_DEAD (reg:SI 144 [ c ])
(nil)))
in there. On the short testcase, the first successful combine is trying to
combine the *mvconst_internal, and and zero_extend:
Failed to match this instruction:
(set (reg:SI 148 [ c ])
(and:SI (reg:SI 145 [ a ])
(const_int 2124 [0x84c])))
Successfully matched this instruction:
(set (reg:SI 144 [ c ])
(const_int 2124 [0x84c]))
Successfully matched this instruction:
(set (reg:SI 148 [ c ])
(and:SI (reg:SI 145 [ a ])
(reg:SI 144 [ c ])))
and everything is fine.
On the #c0 testcase, the first successful combine from the above ones is trying
to combine the and and insn 47 (subreg) into:
Successfully matched this instruction:
(set (subreg:SI (reg:HI 175) 0)
(and:SI (reg:SI 167 [ m ])
(reg:SI 168)))
Now, not really sure if that's valid given that riscv is
WORD_REGISTER_OPERATIONS 1 target.
But maybe even the insn 47 before combine is wrong for such a target.
As pseudo 168 is 0x8084c, the upper half contains one randomish bit.
And later this new insn is combined with the leu into
Successfully matched this instruction:
(set (reg:SI 172)
(leu:SI (subreg:SI (reg:HI 175) 0)
(const_int 5 [0x5])))
which is definitely wrong, because the zero extension disappeared.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
` (4 preceding siblings ...)
2023-04-04 13:41 ` jakub at gcc dot gnu.org
@ 2023-04-04 13:46 ` jakub at gcc dot gnu.org
2023-04-04 15:02 ` jakub at gcc dot gnu.org
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2023-04-04 13:46 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The insn with HImode subreg comes from RTL DSE, before DSE we have:
(insn 36 35 39 2 (set (mem/c:SI (plus:SI (reg/f:SI 65 frame)
(const_int -64 [0xffffffffffffffc0])) [2 S4 A128])
(reg:SI 166)) "pr109040.c":9:11 178 {*movsi_internal}
(expr_list:REG_DEAD (reg:SI 166)
(nil)))
(insn 39 36 40 2 (set (reg:SI 171)
(zero_extend:SI (mem/c:HI (plus:SI (reg/f:SI 65 frame)
(const_int -64 [0xffffffffffffffc0])) [0 S2 A128])))
"pr109040.c":9:11 111 {*zero_extendhisi2}
(nil))
and DSE makes:
(insn 47 35 39 2 (set (reg:HI 175)
(subreg:HI (reg:SI 166) 0)) "pr109040.c":9:11 179 {*movhi_internal}
(expr_list:REG_DEAD (reg:SI 166)
(nil)))
(insn 39 47 40 2 (set (reg:SI 171)
(zero_extend:SI (reg:HI 175))) "pr109040.c":9:11 111
{*zero_extendhisi2}
(expr_list:REG_DEAD (reg:HI 175)
(nil)))
out of it. Does it for WORD_REGISTER_OPERATIONS need to create a single
instruction?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
` (5 preceding siblings ...)
2023-04-04 13:46 ` jakub at gcc dot gnu.org
@ 2023-04-04 15:02 ` jakub at gcc dot gnu.org
2023-04-04 20:08 ` ebotcazou at gcc dot gnu.org
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2023-04-04 15:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54809
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54809&action=edit
gcc13-pr109040.patch
The following patch seems to fix it at least when eyeballing the testcase in a
cross-compiler, it achieves exactly what we want, & 0x84c to perform the zero
extension. But I have no way to actually test this on riscv.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
` (6 preceding siblings ...)
2023-04-04 15:02 ` jakub at gcc dot gnu.org
@ 2023-04-04 20:08 ` ebotcazou at gcc dot gnu.org
2023-04-05 14:17 ` law at gcc dot gnu.org
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: ebotcazou at gcc dot gnu.org @ 2023-04-04 20:08 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
WORD_REGISTER_OPERATIONS means that subword register operations in RTL are
actually performed on the whole word register by the machine, e.g.. for
(insn 47 35 39 2 (set (reg:HI 175)
(subreg:HI (reg:SI 166) 0)) "pr109040.c":9:11 179 {*movhi_internal}
(expr_list:REG_DEAD (reg:SI 166)
(nil)))
you can infer the MSB of (reg:SI 175) instead of them being undefined. But I
don't think that it prevents any particular instruction form.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
` (7 preceding siblings ...)
2023-04-04 20:08 ` ebotcazou at gcc dot gnu.org
@ 2023-04-05 14:17 ` law at gcc dot gnu.org
2023-04-14 7:32 ` cvs-commit at gcc dot gnu.org
2023-04-19 9:13 ` cvs-commit at gcc dot gnu.org
10 siblings, 0 replies; 12+ messages in thread
From: law at gcc dot gnu.org @ 2023-04-05 14:17 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
Jeffrey A. Law <law at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |DUPLICATE
--- Comment #8 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Duplicate.
*** This bug has been marked as a duplicate of bug 108947 ***
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
` (8 preceding siblings ...)
2023-04-05 14:17 ` law at gcc dot gnu.org
@ 2023-04-14 7:32 ` cvs-commit at gcc dot gnu.org
2023-04-19 9:13 ` cvs-commit at gcc dot gnu.org
10 siblings, 0 replies; 12+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-04-14 7:32 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:
https://gcc.gnu.org/g:9d1a6119590ef828f9782a7083d03e535bc2f2cf
commit r13-7178-g9d1a6119590ef828f9782a7083d03e535bc2f2cf
Author: Jakub Jelinek <jakub@redhat.com>
Date: Fri Apr 14 09:20:49 2023 +0200
combine: Fix AND handling for WORD_REGISTER_OPERATIONS targets [PR109040]
The following testcase is miscompiled on riscv since the addition
of *mvconst_internal define_insn_and_split.
We have:
(insn 36 35 39 2 (set (mem/c:SI (plus:SI (reg/f:SI 65 frame)
(const_int -64 [0xffffffffffffffc0])) [2 S4 A128])
(reg:SI 166)) "pr109040.c":9:11 178 {*movsi_internal}
(expr_list:REG_DEAD (reg:SI 166)
(nil)))
(insn 39 36 40 2 (set (reg:SI 171)
(zero_extend:SI (mem/c:HI (plus:SI (reg/f:SI 65 frame)
(const_int -64 [0xffffffffffffffc0])) [0 S2 A128])))
"pr109040.c":9:11 111 {*zero_extendhisi2}
(nil))
and RTL DSE's replace_read since r0-86337-g18b526e806ab6455 handles
even different modes like in the above case, and so it optimizes it into:
(insn 47 35 39 2 (set (reg:HI 175)
(subreg:HI (reg:SI 166) 0)) "pr109040.c":9:11 179 {*movhi_internal}
(expr_list:REG_DEAD (reg:SI 166)
(nil)))
(insn 39 47 40 2 (set (reg:SI 171)
(zero_extend:SI (reg:HI 175))) "pr109040.c":9:11 111
{*zero_extendhisi2}
(expr_list:REG_DEAD (reg:HI 175)
(nil)))
Pseudo 166 is result of AND with 0x8084c constant (forced into a register).
Combine attempts to combine the AND with the insn 47 above created by DSE,
and turns it because of WORD_REGISTER_OPERATIONS and its assumption that
all
the subword operations are actually done on word mode into:
(set (subreg:SI (reg:HI 175) 0)
(and:SI (reg:SI 167 [ m ])
(reg:SI 168)))
and later on the ZERO_EXTEND is thrown away.
We then see
(and:SI (subreg:SI (reg:HI 175) 0) (const_int 0x84c))
and optimize that into
(subreg:SI (and:HI (reg:HI 175) (const_int 0x84c)) 0)
which is still fine, in WORD_REGISTER_OPERATIONS the AND in HImode
will set all upper bits up to BITS_PER_WORD to zeros.
But later on simplify_binary_operation_1 or simplify_and_const_int_1
sees that because nonzero_bits ((reg:HI 175), HImode) == 0x84c, we can
optimize the AND into (reg:HI 175). That isn't correct, because while
the low 16 bits of that REG are known to have all bits but 0x84c cleared,
we don't know that all the upper 16 bits are all clear as well.
So, for WORD_REGISTER_OPERATIONS for integral modes smaller than word mode,
we need to check all bits from word_mode in nonzero_bits for the
optimizations.
2023-04-14 Jeff Law <jlaw@ventanamicro.com>
Jakub Jelinek <jakub@redhat.com>
PR target/108947
PR target/109040
* combine.cc (simplify_and_const_int_1): Compute nonzero_bits in
word_mode rather than mode if WORD_REGISTER_OPERATIONS and mode is
smaller than word_mode.
* simplify-rtx.cc (simplify_context::simplify_binary_operation_1)
<case AND>: Likewise.
* gcc.dg/pr108947.c: New test.
* gcc.c-torture/execute/pr109040.c: New test.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Bug target/109040] [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 since r13-4907-g2e886eef7f2b5a
2023-03-06 11:14 [Bug target/109040] New: [13 Regression] wrong code with v16hi compare & mask on riscv64 at -O2 zsojka at seznam dot cz
` (9 preceding siblings ...)
2023-04-14 7:32 ` cvs-commit at gcc dot gnu.org
@ 2023-04-19 9:13 ` cvs-commit at gcc dot gnu.org
10 siblings, 0 replies; 12+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-04-19 9:13 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109040
--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:
https://gcc.gnu.org/g:76f44fbfea1f11e53d4b7e83f0debd029c94a1b3
commit r14-64-g76f44fbfea1f11e53d4b7e83f0debd029c94a1b3
Author: Jakub Jelinek <jakub@redhat.com>
Date: Wed Apr 19 11:13:11 2023 +0200
dse: Use SUBREG_REG for copy_to_mode_reg in DSE replace_read for
WORD_REGISTER_OPERATIONS targets [PR109040]
While we've agreed this is not the right fix for the PR109040 bug,
the patch clearly improves generated code (at least on the testcase from
the
PR), so I'd like to propose this as optimization heuristics improvement
for GCC 14.
2023-04-19 Jakub Jelinek <jakub@redhat.com>
PR target/109040
* dse.cc (replace_read): If read_reg is a SUBREG of a word mode
REG, for WORD_REGISTER_OPERATIONS copy SUBREG_REG of it into
a new REG rather than the SUBREG.
^ permalink raw reply [flat|nested] 12+ messages in thread