Skip to content
Snippets Groups Projects
Commit cab2e123 authored by Richard Sandiford's avatar Richard Sandiford Committed by Jeff Law
Browse files

[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 :slight_frown:

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 :slight_smile:



	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: default avatarJeff Law <jlaw@ventanamicro.com>
parent 3b3b3f88
No related merge requests found
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment