On 5/17/23 10:02, Jivan Hakobyan via Gcc-patches wrote:
Subject:
[v2] RISC-V: Remove masking third operand of rotate instructions
From:
Jivan Hakobyan via Gcc-patches <gcc-patches@gcc.gnu.org>
Date:
5/17/23, 10:02
To:
gcc-patches@gcc.gnu.org
Rotate instructions do not need to mask the third operand.
For example, RV64 the following code:
unsigned long foo1(unsigned long rs1, unsigned long rs2)
{
long shamt = rs2 & (64 - 1);
return (rs1 << shamt) | (rs1 >> ((64 - shamt) & (64 - 1)));
}
Compiles to:
foo1:
andi a1,a1,63
rol a0,a0,a1
ret
This patch removes unnecessary masking.
Besides, I have merged masking insns for shifts that were written before.
gcc/ChangeLog:
* config/riscv/riscv.md (*<optab><mode>3_mask): New pattern,
combined from ...
(*<optab>si3_mask, *<optab>di3_mask): Here.
(*<optab><mode>3_mask_1): New pattern, combined from ...
(*<optab>si3_mask_1, *<optab>di3_mask_1): Here.
* config/riscv/bitmanip.md (*<bitmanip_optab><mode>3_mask): New
pattern.
(*<bitmanip_optab>si3_sext_mask): Likewise.
* config/riscv/iterators.md (shiftm1): Generalize to handle more
masking constants.
(bitmanip_rotate): New iterator.
(bitmanip_optab): Add rotates.
* config/riscv/predicates.md (const_si_mask_operand): Renamed
from const31_operand. Generalize to handle more mask constants.
(const_di_mask_operand): Similarly.
gcc/testsuite/ChangeLog:
* testsuite/gcc.target/riscv/shift-and-2.c: Fixed test
* testsuite/gcc.target/riscv/zbb-rol-ror-01.c: New test
* testsuite/gcc.target/riscv/zbb-rol-ror-02.c: New test
* testsuite/gcc.target/riscv/zbb-rol-ror-03.c: New test
* testsuite/gcc.target/riscv/zbb-rol-ror-04.c: New test
* testsuite/gcc.target/riscv/zbb-rol-ror-05.c: New test
* testsuite/gcc.target/riscv/zbb-rol-ror-06.c: New test
* testsuite/gcc.target/riscv/zbb-rol-ror-07.c: New test
Thanks for the updated patch. A few comments.
-- With the best regards Jivan Hakobyan
rotate_mask.patch
diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index a27fc3e34a1..0fd0cbdeb04 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -351,6 +351,42 @@
"rolw\t%0,%1,%2"
[(set_attr "type" "bitmanip")])
+(define_insn_and_split "*<bitmanip_optab><mode>3_mask"
+ [(set (match_operand:X 0 "register_operand" "= r")
+ (bitmanip_rotate:X
+ (match_operand:X 1 "register_operand" " r")
+ (match_operator 4 "subreg_lowpart_operator"
+ [(and:X
+ (match_operand:X 2 "register_operand" "r")
+ (match_operand 3 "<X:shiftm1>" "<X:shiftm1p>"))])))]
+ "TARGET_ZBB || TARGET_ZBKB"
+ "#"
+ "&& 1"
+ [(set (match_dup 0)
+ (bitmanip_rotate:X (match_dup 1)
+ (match_dup 2)))]
+ "operands[2] = gen_lowpart (QImode, operands[2]);"
+ [(set_attr "type" "bitmanip")
+ (set_attr "mode" "<X:MODE>")])
So I couldn't resist the temptation to look at the mode iterator
improvements a bit after our call this morning. As you note, the most
obvious changes will regress the testsuite. But it turns out there are
things we can do to further simplify/combine patterns.
So the trick for the above pattern is to use GPR2 rather than X for the
mode of the bitwise-and subexpression. That allows the mode of that
subexpression to vary independently of the mode of the output.
Similarly for the other pattern that you're adding to bitmanip.md.
We can use GPR/GPR2 iterators in the riscv.md changes as well. The
primary benefit in doing so is we can eliminate another pair of patterns.
With those change we have simpler code that still passes all the new tests.
I've regression tested this V3 variant with no issues. I'll commit it
to the trunk under your name since the bulk of the work is yours.
Thanks,
jeff
commit 6da6ed95c9ca247d405da3dfb737b743686fe5e6
Author: Jivan Hakobyan <jivanhakoby...@gmail.com>
Date: Wed May 17 13:00:28 2023 -0600
RISC-V: Remove masking third operand of rotate instructions
Rotate instructions do not need to mask the third operand.
For example, RV64 the following code:
unsigned long foo1(unsigned long rs1, unsigned long rs2)
{
long shamt = rs2 & (64 - 1);
return (rs1 << shamt) | (rs1 >> ((64 - shamt) & (64 - 1)));
}
Compiles to:
foo1:
andi a1,a1,63
rol a0,a0,a1
ret
This patch removes unnecessary masking.
Besides, I have merged masking insns for shifts that were written
before.
gcc/ChangeLog:
* config/riscv/riscv.md (*<optab><GPR:mode>3_mask): New pattern,
combined from ...
(*<optab>si3_mask, *<optab>di3_mask): Here.
(*<optab>si3_mask_1, *<optab>di3_mask_1): And here.
* config/riscv/bitmanip.md (*<bitmanip_optab><GPR:mode>3_mask): New
pattern.
(*<bitmanip_optab>si3_sext_mask): Likewise.
* config/riscv/iterators.md (shiftm1): Use const_si_mask_operand
and const_di_mask_operand.
(bitmanip_rotate): New iterator.
(bitmanip_optab): Add rotates.
* config/riscv/predicates.md (const_si_mask_operand): Renamed
from const31_operand. Generalize to handle more mask constants.
(const_di_mask_operand): Similarly.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/shift-and-2.c: Fixed test
* gcc.target/riscv/zbb-rol-ror-01.c: New test
* gcc.target/riscv/zbb-rol-ror-02.c: New test
* gcc.target/riscv/zbb-rol-ror-03.c: New test
* gcc.target/riscv/zbb-rol-ror-04.c: New test
* gcc.target/riscv/zbb-rol-ror-05.c: New test
* gcc.target/riscv/zbb-rol-ror-06.c: New test
* gcc.target/riscv/zbb-rol-ror-07.c: New test
diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index a27fc3e34a1..6f3d24a8a88 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -351,6 +351,42 @@ (define_insn "rotlsi3_sext"
"rolw\t%0,%1,%2"
[(set_attr "type" "bitmanip")])
+(define_insn_and_split "*<bitmanip_optab><GPR:mode>3_mask"
+ [(set (match_operand:GPR 0 "register_operand" "= r")
+ (bitmanip_rotate:GPR
+ (match_operand:GPR 1 "register_operand" " r")
+ (match_operator 4 "subreg_lowpart_operator"
+ [(and:GPR2
+ (match_operand:GPR2 2 "register_operand" "r")
+ (match_operand 3 "<GPR:shiftm1>" "<GPR:shiftm1p>"))])))]
+ "TARGET_ZBB || TARGET_ZBKB"
+ "#"
+ "&& 1"
+ [(set (match_dup 0)
+ (bitmanip_rotate:GPR (match_dup 1)
+ (match_dup 2)))]
+ "operands[2] = gen_lowpart (QImode, operands[2]);"
+ [(set_attr "type" "bitmanip")
+ (set_attr "mode" "<GPR:MODE>")])
+
+(define_insn_and_split "*<bitmanip_optab>si3_sext_mask"
+ [(set (match_operand:DI 0 "register_operand" "= r")
+ (sign_extend:DI (bitmanip_rotate:SI
+ (match_operand:SI 1 "register_operand" " r")
+ (match_operator 4 "subreg_lowpart_operator"
+ [(and:GPR
+ (match_operand:GPR 2 "register_operand" "r")
+ (match_operand 3 "const_si_mask_operand"))]))))]
+ "TARGET_64BIT && (TARGET_ZBB || TARGET_ZBKB)"
+ "#"
+ "&& 1"
+ [(set (match_dup 0)
+ (sign_extend:DI (bitmanip_rotate:SI (match_dup 1)
+ (match_dup 2))))]
+ "operands[2] = gen_lowpart (QImode, operands[2]);"
+ [(set_attr "type" "bitmanip")
+ (set_attr "mode" "DI")])
+
;; orc.b (or-combine) is added as an unspec for the benefit of the support
;; for optimized string functions (such as strcmp).
(define_insn "orcb<mode>2"
diff --git a/gcc/config/riscv/iterators.md b/gcc/config/riscv/iterators.md
index 1d56324df03..8afe98e4410 100644
--- a/gcc/config/riscv/iterators.md
+++ b/gcc/config/riscv/iterators.md
@@ -117,7 +117,7 @@ (define_mode_attr UNITMODE [(HF "HF") (SF "SF") (DF "DF")])
(define_mode_attr HALFMODE [(DF "SI") (DI "SI") (TF "DI")])
; bitmanip mode attribute
-(define_mode_attr shiftm1 [(SI "const31_operand") (DI "const63_operand")])
+(define_mode_attr shiftm1 [(SI "const_si_mask_operand") (DI
"const_di_mask_operand")])
(define_mode_attr shiftm1p [(SI "DsS") (DI "DsD")])
;; -------------------------------------------------------------------
@@ -174,6 +174,8 @@ (define_code_iterator bitmanip_minmax [smin umin smax umax])
(define_code_iterator clz_ctz_pcnt [clz ctz popcount])
+(define_code_iterator bitmanip_rotate [rotate rotatert])
+
;; -------------------------------------------------------------------
;; Code Attributes
;; -------------------------------------------------------------------
@@ -271,7 +273,9 @@ (define_code_attr bitmanip_optab [(smin "smin")
(umax "umax")
(clz "clz")
(ctz "ctz")
- (popcount "popcount")])
+ (popcount "popcount")
+ (rotate "rotl")
+ (rotatert "rotr")])
(define_code_attr bitmanip_insn [(smin "min")
(smax "max")
(umin "minu")
diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
index e5adf06fa25..ffcbb9a7589 100644
--- a/gcc/config/riscv/predicates.md
+++ b/gcc/config/riscv/predicates.md
@@ -235,13 +235,15 @@ (define_predicate "not_single_bit_mask_operand"
(and (match_code "const_int")
(match_test "SINGLE_BIT_MASK_OPERAND (~UINTVAL (op))")))
-(define_predicate "const31_operand"
+(define_predicate "const_si_mask_operand"
(and (match_code "const_int")
- (match_test "INTVAL (op) == 31")))
+ (match_test "(INTVAL (op) & (GET_MODE_BITSIZE (SImode) - 1))
+ == GET_MODE_BITSIZE (SImode) - 1")))
-(define_predicate "const63_operand"
+(define_predicate "const_di_mask_operand"
(and (match_code "const_int")
- (match_test "INTVAL (op) == 63")))
+ (match_test "(INTVAL (op) & (GET_MODE_BITSIZE (DImode) - 1))
+ == GET_MODE_BITSIZE (DImode) - 1")))
(define_predicate "imm5_operand"
(and (match_code "const_int")
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index a9179931217..e773bc748bf 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -2048,45 +2048,6 @@ (define_insn "<optab>si3"
[(set_attr "type" "shift")
(set_attr "mode" "SI")])
-(define_insn_and_split "*<optab>si3_mask"
- [(set (match_operand:SI 0 "register_operand" "= r")
- (any_shift:SI
- (match_operand:SI 1 "register_operand" " r")
- (match_operator 4 "subreg_lowpart_operator"
- [(and:SI
- (match_operand:SI 2 "register_operand" "r")
- (match_operand 3 "const_int_operand"))])))]
- "(INTVAL (operands[3]) & (GET_MODE_BITSIZE (SImode)-1))
- == GET_MODE_BITSIZE (SImode)-1"
- "#"
- "&& 1"
- [(set (match_dup 0)
- (any_shift:SI (match_dup 1)
- (match_dup 2)))]
- "operands[2] = gen_lowpart (QImode, operands[2]);"
- [(set_attr "type" "shift")
- (set_attr "mode" "SI")])
-
-(define_insn_and_split "*<optab>si3_mask_1"
- [(set (match_operand:SI 0 "register_operand" "= r")
- (any_shift:SI
- (match_operand:SI 1 "register_operand" " r")
- (match_operator 4 "subreg_lowpart_operator"
- [(and:DI
- (match_operand:DI 2 "register_operand" "r")
- (match_operand 3 "const_int_operand"))])))]
- "TARGET_64BIT
- && (INTVAL (operands[3]) & (GET_MODE_BITSIZE (SImode)-1))
- == GET_MODE_BITSIZE (SImode)-1"
- "#"
- "&& 1"
- [(set (match_dup 0)
- (any_shift:SI (match_dup 1)
- (match_dup 2)))]
- "operands[2] = gen_lowpart (QImode, operands[2]);"
- [(set_attr "type" "shift")
- (set_attr "mode" "SI")])
-
(define_insn "<optab>di3"
[(set (match_operand:DI 0 "register_operand" "= r")
(any_shift:DI
@@ -2103,45 +2064,23 @@ (define_insn "<optab>di3"
[(set_attr "type" "shift")
(set_attr "mode" "DI")])
-(define_insn_and_split "*<optab>di3_mask"
- [(set (match_operand:DI 0 "register_operand" "= r")
- (any_shift:DI
- (match_operand:DI 1 "register_operand" " r")
+(define_insn_and_split "*<optab><GPR:mode>3_mask_1"
+ [(set (match_operand:GPR 0 "register_operand" "= r")
+ (any_shift:GPR
+ (match_operand:GPR 1 "register_operand" " r")
(match_operator 4 "subreg_lowpart_operator"
- [(and:SI
- (match_operand:SI 2 "register_operand" "r")
- (match_operand 3 "const_int_operand"))])))]
- "TARGET_64BIT
- && (INTVAL (operands[3]) & (GET_MODE_BITSIZE (DImode)-1))
- == GET_MODE_BITSIZE (DImode)-1"
- "#"
- "&& 1"
- [(set (match_dup 0)
- (any_shift:DI (match_dup 1)
- (match_dup 2)))]
- "operands[2] = gen_lowpart (QImode, operands[2]);"
- [(set_attr "type" "shift")
- (set_attr "mode" "DI")])
-
-(define_insn_and_split "*<optab>di3_mask_1"
- [(set (match_operand:DI 0 "register_operand" "= r")
- (any_shift:DI
- (match_operand:DI 1 "register_operand" " r")
- (match_operator 4 "subreg_lowpart_operator"
- [(and:DI
- (match_operand:DI 2 "register_operand" "r")
- (match_operand 3 "const_int_operand"))])))]
- "TARGET_64BIT
- && (INTVAL (operands[3]) & (GET_MODE_BITSIZE (DImode)-1))
- == GET_MODE_BITSIZE (DImode)-1"
+ [(and:GPR2
+ (match_operand:GPR2 2 "register_operand" "r")
+ (match_operand 3 "<GPR:shiftm1>"))])))]
+ ""
"#"
"&& 1"
[(set (match_dup 0)
- (any_shift:DI (match_dup 1)
+ (any_shift:GPR (match_dup 1)
(match_dup 2)))]
"operands[2] = gen_lowpart (QImode, operands[2]);"
[(set_attr "type" "shift")
- (set_attr "mode" "DI")])
+ (set_attr "mode" "<GPR:MODE>")])
(define_insn "*<optab>si3_extend"
[(set (match_operand:DI 0 "register_operand" "= r")
@@ -2164,34 +2103,10 @@ (define_insn_and_split "*<optab>si3_extend_mask"
(any_shift:SI
(match_operand:SI 1 "register_operand" " r")
(match_operator 4 "subreg_lowpart_operator"
- [(and:SI
- (match_operand:SI 2 "register_operand" " r")
- (match_operand 3 "const_int_operand"))]))))]
- "TARGET_64BIT
- && (INTVAL (operands[3]) & (GET_MODE_BITSIZE (SImode)-1))
- == GET_MODE_BITSIZE (SImode)-1"
- "#"
- "&& 1"
- [(set (match_dup 0)
- (sign_extend:DI
- (any_shift:SI (match_dup 1)
- (match_dup 2))))]
- "operands[2] = gen_lowpart (QImode, operands[2]);"
- [(set_attr "type" "shift")
- (set_attr "mode" "SI")])
-
-(define_insn_and_split "*<optab>si3_extend_mask_1"
- [(set (match_operand:DI 0 "register_operand" "= r")
- (sign_extend:DI
- (any_shift:SI
- (match_operand:SI 1 "register_operand" " r")
- (match_operator 4 "subreg_lowpart_operator"
- [(and:DI
- (match_operand:DI 2 "register_operand" " r")
- (match_operand 3 "const_int_operand"))]))))]
- "TARGET_64BIT
- && (INTVAL (operands[3]) & (GET_MODE_BITSIZE (SImode)-1))
- == GET_MODE_BITSIZE (SImode)-1"
+ [(and:GPR
+ (match_operand:GPR 2 "register_operand" " r")
+ (match_operand 3 "const_si_mask_operand"))]))))]
+ "TARGET_64BIT"
"#"
"&& 1"
[(set (match_dup 0)
diff --git a/gcc/testsuite/gcc.target/riscv/shift-and-2.c
b/gcc/testsuite/gcc.target/riscv/shift-and-2.c
index 360d8417209..bc01e8ef992 100644
--- a/gcc/testsuite/gcc.target/riscv/shift-and-2.c
+++ b/gcc/testsuite/gcc.target/riscv/shift-and-2.c
@@ -11,10 +11,10 @@ sub2 (int i, long j)
}
/* Test for <optab>si3_extend_mask. */
-unsigned long
-sub3 (int mask)
+int
+sub3 (short mask)
{
- return 1 << (mask & 0xff);
+ return 1 << ((int)mask & 0x1f);
}
/* Test for <optab>si3_extend_mask_1. */
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-01.c
b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-01.c
index 20c1b2856ef..0a5b5e12eb2 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-01.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-01.c
@@ -14,4 +14,5 @@ unsigned long foo2(unsigned long rs1, unsigned long rs2)
}
/* { dg-final { scan-assembler-times "rol" 2 } } */
-/* { dg-final { scan-assembler-times "ror" 2 } } */
\ No newline at end of file
+/* { dg-final { scan-assembler-times "ror" 2 } } */
+/* { dg-final { scan-assembler-not "and" } } */
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-02.c
b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-02.c
index 14196c11fb9..d0d58135809 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-02.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-02.c
@@ -14,4 +14,5 @@ unsigned int foo2(unsigned int rs1, unsigned int rs2)
}
/* { dg-final { scan-assembler-times "rol" 2 } } */
-/* { dg-final { scan-assembler-times "ror" 2 } } */
\ No newline at end of file
+/* { dg-final { scan-assembler-times "ror" 2 } } */
+/* { dg-final { scan-assembler-not {and} { target { no-opts "-O0" } } } } */
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-03.c
b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-03.c
index ed4685dc7ac..b44d7fe8920 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-03.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-03.c
@@ -15,4 +15,5 @@ unsigned int ror(unsigned int rs1, unsigned int rs2)
}
/* { dg-final { scan-assembler-times "rolw" 1 } } */
-/* { dg-final { scan-assembler-times "rorw" 1 } } */
\ No newline at end of file
+/* { dg-final { scan-assembler-times "rorw" 1 } } */
+/* { dg-final { scan-assembler-not "and" } } */
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-04.c
b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-04.c
index 08053484cb2..7ef4c29dd5b 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-04.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-04.c
@@ -2,6 +2,7 @@
/* { dg-options "-march=rv64gc_zbb -mabi=lp64d -fno-lto -O2" } */
/* { dg-skip-if "" { *-*-* } { "-g" } } */
/* { dg-final { check-function-bodies "**" "" } } */
+/* { dg-final { scan-assembler-not "and" } } */
/*
**foo1:
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-05.c
b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-05.c
index 85090b1b0fc..2108ccc3e77 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-05.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-05.c
@@ -2,6 +2,7 @@
/* { dg-options "-march=rv32gc_zbb -mabi=ilp32 -fno-lto -O2" } */
/* { dg-skip-if "" { *-*-* } { "-g" } } */
/* { dg-final { check-function-bodies "**" "" } } */
+/* { dg-final { scan-assembler-not "and" } } */
/*
**foo1:
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-06.c
b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-06.c
index 70b79abb6ed..8c0711d6f94 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-06.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-06.c
@@ -2,6 +2,7 @@
/* { dg-options "-march=rv64gc_zbb -mabi=lp64d -fno-lto -O2" } */
/* { dg-skip-if "" { *-*-* } { "-g" } } */
/* { dg-final { check-function-bodies "**" "" } } */
+/* { dg-final { scan-assembler-not "and" } } */
/*
**foo1:
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-07.c
b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-07.c
index 3b6ab385a85..bda3f0e474d 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-07.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-rol-ror-07.c
@@ -2,6 +2,7 @@
/* { dg-options "-march=rv64gc_zbb -mabi=lp64d -fno-lto -O2" } */
/* { dg-skip-if "" { *-*-* } { "-g" } } */
/* { dg-final { check-function-bodies "**" "" } } */
+/* { dg-final { scan-assembler-not "and" } } */
/*
**foo1:
