| Message ID | bc60f2f0-4ce5-461c-a49f-b1e51012f5c8@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [to-be-committed,RISC-V,PR,target/115921] Improve reassociation for rv64 | expand |
On Mon, Sep 2, 2024 at 11:54 AM Jeff Law <jeffreyalaw@gmail.com> wrote: > > This time with the RISC-V marker so the pre-commit testing system will > pick it up... Hi Jeff, Just a meta-note about precommit: If the patch contains riscv or risc-v anywhere in the patch file pre commit will run [1]. You can also cc patchworks-ci@rivosinc.com to run it. Since this patch edits something in the riscv/ directory, it doesn’t need the risc-v tag to trigger pre-commit [2]. Regardless, thanks for using the patchworks tester! :) Patrick [1] https://github.com/patrick-rivos/riscv-gnu-toolchain/blob/1496f76a9ad4081c0afdde8f7f8ffb22573a1789/scripts/create_patches_files.py#L89 [2] https://patchwork.sourceware.org/project/gcc/patch/7c038242-8663-4d94-9175-ea23397faae2@gmail.com/ > > > -------- Forwarded Message -------- > Subject: [to-be-committed] [PR target/115921] Improve reassociation for > rv64 > Date: Mon, 2 Sep 2024 11:53:44 -0600 > From: Jeff Law <jeffreyalaw@gmail.com> > To: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org> > > > As Jovan pointed out in pr115921, we're not reassociating expressions > like this on rv64: > > (x & 0x3e) << 12 > > It generates something like this: > > li a5,258048 > slli a0,a0,12 > and a0,a0,a5 > > > We have a pattern that's designed to clean this up. Essentially > reassociating the operations so that we don't need to load the constant > resulting in something like this: > > andi a0,a0,63 > slli a0,a0,12 > > > That pattern wasn't working for certain constants due to its condition. > The condition is trying to avoid cases where this kind of reassociation > would hinder shadd generation on rv64. That condition was just written > poorly. > > This patch tightens up that condition in a few ways. First, there's no > need to worry about shadd cases if ZBA is not enabled. Second we can't > use shadd if the shift value isn't 1, 2 or 3. Finally rather than > open-coding one of the tests, we can use an existing operand predicate. > > The net is we'll start performing this transformation in more cases on > rv64 while still avoiding reassociation if it would spoil shadd generation. > > Waiting on the pre-commit testing before taking any further action. > > Jeff
Hi Jeff, On Mon, 2024-09-02 at 12:53 -0600, Jeff Law wrote: > (define_insn_and_split "<optab>_shift_reverse<X:mode>" > [(set (match_operand:X 0 "register_operand" "=r") > (any_bitwise:X (ashift:X (match_operand:X 1 "register_operand" "r") > @@ -2934,9 +2936,9 @@ (define_insn_and_split "<optab>_shift_reverse<X:mode>" > "(!SMALL_OPERAND (INTVAL (operands[3])) > && SMALL_OPERAND (INTVAL (operands[3]) >> INTVAL (operands[2])) > && popcount_hwi (INTVAL (operands[3])) > 1 I'm wondering why we need to check the popcount. With this patch applied: long test1 (long x) { return (x & 0x110) << 12; } is compiled to: test1: andi a0,a0,272 slli a0,a0,12 ret as we've expected, but: long test2 (long x) { return (x & 0x100) << 12; } is compiled to: test: li a5,1048576 slli a0,a0,12 and a0,a0,a5 ret So why must we spend an instruction to load the single-bit immediate?
On 9/4/24 8:08 AM, Xi Ruoyao wrote: > Hi Jeff, > > On Mon, 2024-09-02 at 12:53 -0600, Jeff Law wrote: >> (define_insn_and_split "<optab>_shift_reverse<X:mode>" >> [(set (match_operand:X 0 "register_operand" "=r") >> (any_bitwise:X (ashift:X (match_operand:X 1 "register_operand" "r") >> @@ -2934,9 +2936,9 @@ (define_insn_and_split "<optab>_shift_reverse<X:mode>" >> "(!SMALL_OPERAND (INTVAL (operands[3])) >> && SMALL_OPERAND (INTVAL (operands[3]) >> INTVAL (operands[2])) >> && popcount_hwi (INTVAL (operands[3])) > 1 > > I'm wondering why we need to check the popcount. With this patch > applied: Zbs can do these things with the single bit manipulation instructions. It would be reasonable to make it (TARGET_ZBS && popcount_hwi ...) jeff
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md index 3289ed2155a..58b31658e0a 100644 --- a/gcc/config/riscv/riscv.md +++ b/gcc/config/riscv/riscv.md @@ -2925,7 +2925,9 @@ (define_insn_and_split "*<optab>si3_extend_mask" ;; for IOR/XOR. It probably doesn't matter for AND. ;; ;; We also don't want to do this if the immediate already fits in a simm12 -;; field. +;; field, or is a single bit operand, or when we might be able to generate +;; a shift-add sequence via the splitter in bitmanip.md +;; in bitmanip.md for masks that are a run of consecutive ones. (define_insn_and_split "<optab>_shift_reverse<X:mode>" [(set (match_operand:X 0 "register_operand" "=r") (any_bitwise:X (ashift:X (match_operand:X 1 "register_operand" "r") @@ -2934,9 +2936,9 @@ (define_insn_and_split "<optab>_shift_reverse<X:mode>" "(!SMALL_OPERAND (INTVAL (operands[3])) && SMALL_OPERAND (INTVAL (operands[3]) >> INTVAL (operands[2])) && popcount_hwi (INTVAL (operands[3])) > 1 - && (!TARGET_64BIT - || (exact_log2 ((INTVAL (operands[3]) >> INTVAL (operands[2])) + 1) - == -1)) + && (!(TARGET_64BIT && TARGET_ZBA) + || !consecutive_bits_operand (operands[3], VOIDmode) + || !imm123_operand (operands[2], VOIDmode)) && (INTVAL (operands[3]) & ((1ULL << INTVAL (operands[2])) - 1)) == 0)" "#" "&& 1" diff --git a/gcc/testsuite/gcc.target/riscv/pr115921.c b/gcc/testsuite/gcc.target/riscv/pr115921.c new file mode 100644 index 00000000000..e508e7ce790 --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/pr115921.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -march=rv64gc_zba" { target { rv64 } } } */ +/* { dg-options "-O2 -march=rv32gc_zba" { target { rv32 } } } */ + +typedef unsigned long target_wide_uint_t; + +target_wide_uint_t test_ashift_and(target_wide_uint_t x) { + return (x & 0x3f) << 12; +} + +/* { dg-final { scan-assembler-times "\\sandi" 1 } } */ +/* { dg-final { scan-assembler-times "\\sslli" 1 } } */ +
This time with the RISC-V marker so the pre-commit testing system will pick it up... -------- Forwarded Message -------- Subject: [to-be-committed] [PR target/115921] Improve reassociation for rv64 Date: Mon, 2 Sep 2024 11:53:44 -0600 From: Jeff Law <jeffreyalaw@gmail.com> To: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org> As Jovan pointed out in pr115921, we're not reassociating expressions like this on rv64: (x & 0x3e) << 12 It generates something like this: li a5,258048 slli a0,a0,12 and a0,a0,a5 We have a pattern that's designed to clean this up. Essentially reassociating the operations so that we don't need to load the constant resulting in something like this: andi a0,a0,63 slli a0,a0,12 That pattern wasn't working for certain constants due to its condition. The condition is trying to avoid cases where this kind of reassociation would hinder shadd generation on rv64. That condition was just written poorly. This patch tightens up that condition in a few ways. First, there's no need to worry about shadd cases if ZBA is not enabled. Second we can't use shadd if the shift value isn't 1, 2 or 3. Finally rather than open-coding one of the tests, we can use an existing operand predicate. The net is we'll start performing this transformation in more cases on rv64 while still avoiding reassociation if it would spoil shadd generation. Waiting on the pre-commit testing before taking any further action. Jeff