Skip to content

Commit d9a8ec7

Browse files
committed
[RISC-V] Fix unreported code quality regression with single bit manipulations
I was reviewing some code recently and spotted an oddity. In a few places we were emitting andi dst,src,-1 and in others [x]ori dst,src,0. Those are obviously nops and we should get rid of them. Most of these are coming from a split part of a couple define_insn_and_split patterns added back in late 2022, so this is an unreported 13, 14 & 15 code quality regression (verified on godbolt, https://godbolt.org/z/EPszox5Kd). Essentially the split part is matching over-aggressively and splitting what should be a trivial bitmanip insn such as bset, bclr or binv into a nop logical with a bit twiddle. Since the split portions trigger post-reload nothing comes along to remove the nop logical operations. The fix is trivial. Just refine the condition. I considered refining the operand predicates too. Both are valid approaches. I noticed the formatting was goofy, so fixed that while I was in there. I'm aware of one other similar case, but I haven't concluded if it's a regression or not. Tested in my tester. Waiting for pre-commit CI to do its thing. Jeff gcc/ * config/riscv/bitmanip.md (*<or_optab>i<mode>_extrabit): Reject cases where we only need to twiddle one bit. Fix formatting. (*andi<mode>extrabit): Likewise. gcc/testsuite/ * gcc.target/riscv/redundant-andi.c: New test. * gcc.target/riscv/redundant-ori.c: Likewise
1 parent 456f5ef commit d9a8ec7

File tree

3 files changed

+69
-10
lines changed

3 files changed

+69
-10
lines changed

gcc/config/riscv/bitmanip.md

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1017,17 +1017,17 @@
10171017
[(set (match_operand:X 0 "register_operand" "=r")
10181018
(any_or:X (match_operand:X 1 "register_operand" "r")
10191019
(match_operand:X 2 "uimm_extra_bit_or_twobits" "i")))]
1020-
"TARGET_ZBS"
1020+
"TARGET_ZBS && !single_bit_mask_operand (operands[2], VOIDmode)"
10211021
"#"
10221022
"&& reload_completed"
10231023
[(set (match_dup 0) (<or_optab>:X (match_dup 1) (match_dup 3)))
10241024
(set (match_dup 0) (<or_optab>:X (match_dup 0) (match_dup 4)))]
10251025
{
1026-
unsigned HOST_WIDE_INT bits = UINTVAL (operands[2]);
1027-
unsigned HOST_WIDE_INT topbit = HOST_WIDE_INT_1U << floor_log2 (bits);
1026+
unsigned HOST_WIDE_INT bits = UINTVAL (operands[2]);
1027+
unsigned HOST_WIDE_INT topbit = HOST_WIDE_INT_1U << floor_log2 (bits);
10281028

1029-
operands[3] = GEN_INT (bits &~ topbit);
1030-
operands[4] = GEN_INT (topbit);
1029+
operands[3] = GEN_INT (bits &~ topbit);
1030+
operands[4] = GEN_INT (topbit);
10311031
}
10321032
[(set_attr "type" "bitmanip")])
10331033

@@ -1036,17 +1036,17 @@
10361036
[(set (match_operand:X 0 "register_operand" "=r")
10371037
(and:X (match_operand:X 1 "register_operand" "r")
10381038
(match_operand:X 2 "not_uimm_extra_bit_or_nottwobits" "i")))]
1039-
"TARGET_ZBS"
1039+
"TARGET_ZBS && !not_single_bit_mask_operand (operands[2], VOIDmode)"
10401040
"#"
10411041
"&& reload_completed"
10421042
[(set (match_dup 0) (and:X (match_dup 1) (match_dup 3)))
10431043
(set (match_dup 0) (and:X (match_dup 0) (match_dup 4)))]
10441044
{
1045-
unsigned HOST_WIDE_INT bits = UINTVAL (operands[2]);
1046-
unsigned HOST_WIDE_INT topbit = HOST_WIDE_INT_1U << floor_log2 (~bits);
1045+
unsigned HOST_WIDE_INT bits = UINTVAL (operands[2]);
1046+
unsigned HOST_WIDE_INT topbit = HOST_WIDE_INT_1U << floor_log2 (~bits);
10471047

1048-
operands[3] = GEN_INT (bits | topbit);
1049-
operands[4] = GEN_INT (~topbit);
1048+
operands[3] = GEN_INT (bits | topbit);
1049+
operands[4] = GEN_INT (~topbit);
10501050
}
10511051
[(set_attr "type" "bitmanip")])
10521052

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
/* { dg-do compile } */
2+
/* { dg-options "-march=rv64gcb -mabi=lp64" } */
3+
/* { dg-skip-if "" { *-*-* } { "-O0" } } */
4+
5+
6+
typedef struct sv SV;
7+
8+
typedef struct magic MAGIC;
9+
typedef short I16;
10+
typedef unsigned short U16;
11+
typedef int I32;
12+
typedef unsigned int U32;
13+
struct sv
14+
{
15+
U32 sv_refcnt;
16+
U32 sv_flags;
17+
};
18+
struct magic
19+
{
20+
U16 mg_private;
21+
};
22+
extern SV **PL_psig_ptr;
23+
int
24+
Perl_magic_setsig (SV *sv, MAGIC *mg, const char *s)
25+
{
26+
I32 i;
27+
i = (I16) mg->mg_private;
28+
if (sv)
29+
{
30+
PL_psig_ptr[i] = (++((sv)->sv_refcnt), ((SV *) ((void *) (sv))));
31+
((sv)->sv_flags &= ~0x00080000);
32+
}
33+
else
34+
{
35+
PL_psig_ptr[i] = ((void *) 0);
36+
}
37+
return 0;
38+
}
39+
40+
/* { dg-final { scan-assembler-not "andi\t" } } */
41+
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/* { dg-do compile } */
2+
/* { dg-options "-march=rv64gcb -mabi=lp64" } */
3+
/* { dg-skip-if "" { *-*-* } { "-O0" } } */
4+
5+
6+
struct sv XS_constant__make_const_svz_2_0;
7+
struct sv {
8+
int sv_flags;
9+
} XS_constant__make_const() {
10+
struct sv *sv = &XS_constant__make_const_svz_2_0;
11+
sv->sv_flags |= 65536;
12+
if (XS_constant__make_const_svz_2_0.sv_flags & 5)
13+
for (;;)
14+
;
15+
}
16+
17+
/* { dg-final { scan-assembler-not "ori\t" } } */
18+

0 commit comments

Comments
 (0)