https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109592
--- Comment #12 from GCC Commits <cvs-commit at gcc dot gnu.org> --- The master branch has been updated by Jeff Law <l...@gcc.gnu.org>: https://gcc.gnu.org/g:cab2e12362287bd50f9abdd7fb5a775138e02d1b commit r15-6902-gcab2e12362287bd50f9abdd7fb5a775138e02d1b Author: Richard Sandiford <richard.sandif...@arm.com> Date: Tue Jan 14 21:51:41 2025 -0700 [PR rtl-optimization/109592] Simplify nested shifts > The BZ in question is a failure to recognize a pair of shifts as a sign > extension. > > I originally thought simplify-rtx would be the right framework to > address this problem, but fwprop is actually better. We can write the > recognizer much simpler in that framework. > > fwprop already simplifies nested shifts/extensions to the desired RTL, > but it's not considered profitable and we throw away the good work done > by fwprop & simplifiers. > > It's hard to see a scenario where nested shifts or nested extensions > that simplify down to a single sign/zero extension isn't a profitable > transformation. So when fwprop has nested shifts/extensions that > simplifies to an extension, we consider it profitable. > > This allow us to simplify the testcase on rv64 with ZBB enabled from a > pair of shifts to a single byte or half-word sign extension. Hmm. So just to summarise something that was discussed in the PR comments, this is a case where combine's expand_compound_operation/ make_compound_operation wrangler hurts us, because the process isn't idempotent, and combine produces two complex instructions: (insn 6 3 7 2 (set (reg:DI 137 [ _3 ]) (ashift:DI (reg:DI 139 [ x ]) (const_int 24 [0x18]))) "foo.c":2:20 305 {ashldi3} (expr_list:REG_DEAD (reg:DI 139 [ x ]) (nil))) (insn 12 7 13 2 (set (reg/i:DI 10 a0) (sign_extend:DI (ashiftrt:SI (subreg:SI (reg:DI 137 [ _3 ]) 0) (const_int 24 [0x18])))) "foo.c":2:27 321 {ashrsi3_extend} (expr_list:REG_DEAD (reg:DI 137 [ _3 ]) (nil))) given two simple instructions: (insn 6 3 7 2 (set (reg:SI 137 [ _3 ]) (sign_extend:SI (subreg:QI (reg/v:DI 136 [ x ]) 0))) "foo.c":2:20 533 {*extendqisi2_bitmanip} (expr_list:REG_DEAD (reg/v:DI 136 [ x ]) (nil))) (insn 7 6 12 2 (set (reg:DI 138 [ _3 ]) (sign_extend:DI (reg:SI 137 [ _3 ]))) "foo.c":2:20 discrim 1 133 {*extendsidi2_internal} (expr_list:REG_DEAD (reg:SI 137 [ _3 ]) (nil))) If I run with -fdisable-rtl-combine then late_combine1 already does the expected transformation. Although it would be nice to fix combine, that might be difficult. If we treat combine as immutable then the options are: (1) Teach simplify-rtx to simplify combine's output into a single sign_extend. (2) Allow fwprop1 to get in first, before combine has a chance to mess things up. The patch goes for (2). Is that a fair summary? Playing devil's advocate, I suppose one advantage of (1) is that it would allow the optimisation even if the original rtl looked like combine's output. And fwprop1 doesn't distinguish between cases in which the source instruction disappears from cases in which the source instruction is kept. Thus we could transform: (set (reg:SI R2) (sign_extend:SI (reg:QI R1))) (set (reg:DI R3) (sign_extend:DI (reg:SI R2))) into: (set (reg:SI R2) (sign_extend:SI (reg:QI R1))) (set (reg:DI R3) (sign_extend:DI (reg:QI R1))) which increases the register pressure between the two instructions (since R2 and R1 are both now live). In general, there could be quite a gap between the two instructions. On the other hand, even in that case, fwprop1 would be parallelising the extensions. And since we're talking about unary operations, even two-address targets would allow R1 to be extended without tying the source and destination. Also, it seems relatively unlikely that expand would produce code that looks like combine's, since the gimple optimisers should have simplified it into conversions. So initially I was going to agree that it's worth trying in fwprop. But... [ commentary on Jeff's original approach dropped. ] So it seems like it's a bit of a mess ð If we do try to fix combine, I think something like the attached would fit within the current scheme. It is a pure shift-for-shift transformation, avoiding any extensions. Will think more about it, but wanted to get the above stream of consciousness out before I finish for the day ð PR rtl-optimization/109592 gcc/ * simplify-rtx.cc (simplify_context::simplify_binary_operation_1): Simplify nested shifts with subregs. gcc/testsuite * gcc.target/riscv/pr109592.c: New test. * gcc.target/riscv/sign-extend-rshift.c: Adjust expected output Co-authored-by: Jeff Law <j...@ventanamicro.com>
