| Message ID | b191dd78-0e5e-4e96-ab90-299f23319443@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | [to-be-committed,RISC-V] Eliminate redundant bitmanip operation | expand |
On Sun, May 19, 2024 at 10:58 AM Jeff Law <jlaw@ventanamicro.com> wrote: > > perl has some internal bitmap code. One of its implementation > properties is that if you ask it to set a bit, the bit is first cleared. > > > Unfortunately this is fairly hard to see in gimple/match due to type > changes in the IL. But it is easy to see in the code we get from > combine. So we just match the relevant cases. So looking into this from a gimple point of view, we can see the issue on x86_64 if you used explicitly `unsigned char`. We have: ``` c_8 = (unsigned char) _1; _2 = *a_10(D); c.0_3 = (signed char) _1; _4 = ~c.0_3; _12 = (unsigned char) _4; `` So for this, we could push the no_op cast from `signed char` to `unsigned char` past the `bit_not` and I think it will fix the issue on the gimple level. So something like: ``` /* Push no_op conversion past the bit_not expression if it was single use. */ (simplify (convert (bit_not:s @0)) (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) (bit_not (convert @0)))) ``` Thanks, Andrew Pinski > > > > Regression tested in Ventana's CI system as well as my own. Waiting on > the Rivos CI system before moving forward. > > > > Jeff
On 5/19/24 1:59 PM, Andrew Pinski wrote: > On Sun, May 19, 2024 at 10:58 AM Jeff Law <jlaw@ventanamicro.com> wrote: >> >> perl has some internal bitmap code. One of its implementation >> properties is that if you ask it to set a bit, the bit is first cleared. >> >> >> Unfortunately this is fairly hard to see in gimple/match due to type >> changes in the IL. But it is easy to see in the code we get from >> combine. So we just match the relevant cases. > > > So looking into this from a gimple point of view, we can see the issue > on x86_64 if you used explicitly `unsigned char`. > We have: > ``` > c_8 = (unsigned char) _1; > _2 = *a_10(D); > c.0_3 = (signed char) _1; > _4 = ~c.0_3; > _12 = (unsigned char) _4; > `` > So for this, we could push the no_op cast from `signed char` to > `unsigned char` past the `bit_not` and I think it will fix the issue > on the gimple level. > So something like: > ``` > /* Push no_op conversion past the bit_not expression if it was single use. */ > (simplify > (convert (bit_not:s @0)) > (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) > (bit_not (convert @0)))) I'm not sure where the best place to put the conversion would be in gimple. I bet there's times when we want the conversion at the outer level and others times at the inner level. Just not sure it's going to be clear cut with either solution likely causing regressions somewhere. What we can (and probably should) do is put this simplification into simplify-rtx. It's target independent and shouldn't be hard to capture there. Jeff
diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md index 8769a6b818b..9d4247ec8b9 100644 --- a/gcc/config/riscv/bitmanip.md +++ b/gcc/config/riscv/bitmanip.md @@ -877,6 +877,29 @@ (define_insn_and_split "" }" [(set_attr "type" "bitmanip")]) +;; In theory these might be better handled with match.pd patterns, but +;; the type changes tend to make it ugly, at least for the perl testcases +(define_insn "" + [(set (match_operand:X 0 "register_operand" "=r") + (ior:X (and:X (rotate:X (const_int -2) + (match_operand:QI 1 "register_operand" "r")) + (match_operand:X 2 "register_operand" "r")) + (ashift:X (const_int 1) (match_operand:QI 3 "register_operand" "r"))))] + "TARGET_ZBS && rtx_equal_p (operands[1], operands[3])" + "bset\t%0,%2,%1" + [(set_attr "type" "bitmanip")]) + +(define_insn "" + [(set (match_operand:X 0 "register_operand" "=r") + (and:X (any_or:X (ashift:X (const_int 1) + (match_operand:QI 1 "register_operand" "r")) + (match_operand:X 2 "register_operand" "r")) + (rotate:X (const_int -2) + (match_operand:QI 3 "register_operand" "r"))))] + "TARGET_ZBS && rtx_equal_p (operands[1], operands[3])" + "bclr\t%0,%2,%1" + [(set_attr "type" "bitmanip")]) + ;; IF_THEN_ELSE: test for 2 bits of opposite polarity (define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit" [(set (pc) diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index b0a14a2a82d..78a4a1cd554 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -3712,6 +3712,22 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN return true; } + /* Special case for bset followed by bclr. */ + if (GET_CODE (x) == AND + && (GET_CODE (XEXP (x, 0)) == IOR + || GET_CODE (XEXP (x, 0)) == XOR) + && GET_CODE (XEXP (XEXP (x, 0), 0)) == ASHIFT + && XEXP (XEXP (XEXP (x, 0), 0), 0) == CONST1_RTX (word_mode) + && GET_CODE (XEXP (x, 1)) == ROTATE + && CONST_INT_P (XEXP (XEXP (x, 1), 0)) + && INTVAL (XEXP (XEXP (x, 1), 0)) == -2 + && rtx_equal_p (XEXP (XEXP (XEXP (x, 0), 0), 1), + (XEXP (XEXP (x, 1), 1)))) + { + *total = COSTS_N_INSNS (1); + return true; + } + gcc_fallthrough (); case IOR: case XOR: @@ -3734,6 +3750,21 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN return true; } + /* Special case for bclr followed by bset. */ + if (GET_CODE (x) == IOR + && GET_CODE (XEXP (x, 0)) == AND + && GET_CODE (XEXP (XEXP (x, 0), 0)) == ROTATE + && CONST_INT_P (XEXP (XEXP (XEXP (x, 0), 0), 0)) + && INTVAL (XEXP (XEXP (XEXP (x, 0), 0), 0)) == -2 + && GET_CODE (XEXP (x, 1)) == ASHIFT + && XEXP (XEXP (x, 1), 0) == CONST1_RTX (word_mode) + && rtx_equal_p (XEXP (XEXP (XEXP (x, 0), 0), 1), + (XEXP (XEXP (x, 1), 1)))) + { + *total = COSTS_N_INSNS (1); + return true; + } + /* Double-word operations use two single-word operations. */ *total = riscv_binary_cost (x, 1, 2); return false; diff --git a/gcc/testsuite/g++.target/riscv/redundant-bitmap-1.C b/gcc/testsuite/g++.target/riscv/redundant-bitmap-1.C new file mode 100644 index 00000000000..85be608bdc8 --- /dev/null +++ b/gcc/testsuite/g++.target/riscv/redundant-bitmap-1.C @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gc_zbs -mabi=lp64" } */ +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */ + +void setBit(char &a, int b) { + char c = 0x1UL << b; + a &= ~c; + a |= c; +} + +/* { dg-final { scan-assembler-not "bclr\t" } } */ + diff --git a/gcc/testsuite/g++.target/riscv/redundant-bitmap-2.C b/gcc/testsuite/g++.target/riscv/redundant-bitmap-2.C new file mode 100644 index 00000000000..9060eb1d769 --- /dev/null +++ b/gcc/testsuite/g++.target/riscv/redundant-bitmap-2.C @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gc_zbs -mabi=lp64" } */ +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */ + +void setBit(char &a, int b) { + char c = 0x1UL << b; + a |= c; + a &= ~c; +} + +/* { dg-final { scan-assembler-not "bset\t" } } */ + diff --git a/gcc/testsuite/g++.target/riscv/redundant-bitmap-3.C b/gcc/testsuite/g++.target/riscv/redundant-bitmap-3.C new file mode 100644 index 00000000000..a33d4d11969 --- /dev/null +++ b/gcc/testsuite/g++.target/riscv/redundant-bitmap-3.C @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gc_zbs -mabi=lp64" } */ +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */ + +void setBit(char &a, int b) { + char c = 0x1UL << b; + a ^= c; + a &= ~c; +} + +/* { dg-final { scan-assembler-not "binv\t" } } */ +