Redundant sign-extension instructions on RISC-V
Richard Biener
richard.guenther@gmail.com
Wed Aug 30 09:44:00 GMT 2017
On Wed, Aug 30, 2017 at 2:36 AM, Michael Clark <michaeljclark@mac.com> wrote:
> Dear GCC folk,
>
>
> # Issue Background
>
> We’re investigating an issue with redundant sign-extension instructions being emitted with the riscv backend. Firstly I would like to state that riscv is possibly a unique backend with respect to its canonical sign-extended register form due to the following:
>
> - POINTERS_EXTEND_UNSIGNED is false, and the canonical form after 32-bit operations on RV64 is sign-extended not zero-extended.
>
> - PROMOTE_MODE is defined on RV64 such that SI mode values are promoted to signed DI mode values holding SI mode subregs
>
> - RISC-V does not have register aliases for these different modes, rather different instructions are selected to operate on different register widths.
>
> - The 32-bit instructions sign-extend results. e.g. all shifts, add, sub, etc.
>
> - Some instructions such as logical ops only have full word width variants (AND, OR, XOR) but these instructions maintain canonical form as there is no horizontal movement of bits.
>
>
> # Issue Details
>
> During porting from GCC 6.1 to GCC 7.1, the RISC-V port was changed to define TRULY_NOOP_TRUNCATION to 1 and PROMOTE_MODE was set up to promote values to wider modes. This was done to ensure ABI correctness so that registers holding narrower modes always contain canonical sign extended values.
>
> The result of this is that sign_extend operations are inserted but later eliminated in ree/simplyfy_rtx. In testcase 3 the sign_extend is correctly eliminated, and indeed most 32-bit instructions are handled correctly. This is what the pattern looks like for a typical 32-bit instruction after expand:
>
> ;;
> ;; Full RTL generated for this function:
> ;;
> (note 1 0 4 NOTE_INSN_DELETED)
> (note 4 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
> (insn 2 4 3 2 (set (reg/v:DI 73 [ rs1 ])
> (reg:DI 10 a0 [ rs1 ])) "lshift.c":1 -1
> (nil))
> (note 3 2 6 2 NOTE_INSN_FUNCTION_BEG)
> (insn 6 3 7 2 (set (reg:SI 74)
> (ashift:SI (subreg/s/u:SI (reg/v:DI 73 [ rs1 ]) 0)
> (const_int 24 [0x18]))) "lshift.c":1 -1
> (nil))
> (insn 7 6 8 2 (set (reg:DI 75)
> (sign_extend:DI (reg:SI 74))) "lshift.c":1 -1
> (nil))
> (insn 8 7 12 2 (set (reg:DI 72 [ <retval> ])
> (reg:DI 75)) "lshift.c":1 -1
> (nil))
> (insn 12 8 13 2 (set (reg/i:DI 10 a0)
> (reg:DI 72 [ <retval> ])) "lshift.c":1 -1
> (nil))
> (insn 13 12 0 2 (use (reg/i:DI 10 a0)) "lshift.c":1 -1
> (nil))
>
>
> # Problem test cases
>
> - testcase 1 - open coded bswap - redundant sign extension instructions
> - testcase 2 - open coded right shift - redundant sign extension instructions
> - testcase 3 - open coded left shift - control / correct behaviour
>
> It appears that the downside to the PROMOTE_MODE transition is that several redundant sign-extension operations are cropping up under specific circumstances. One of the first small isolators is testcase 1 (below), which is an open-coded bswap. You’ll notice the SEXT.W emitted before the return. This was then further isolated to an even simpler testcase 2 which is a single right shift which also emits SEXT.W before the return. A 32-bit left shift or other 32-bit operations correctly have the redundant sign_extend eliminated.
>
> We have isolated the code that prevented the right shift from having its redundant sign extension eliminated and it is target independent code in simplify-rtx. Code in simplify_unary_operation_1 assumes zero extension is more efficient, likely an assumption based on the fact that most platforms define POINTERS_EXTEND_UNSIGNED to true and naturally promote words to zero extended form vs sign extended form. The redundant sign extension appears to be due to an optimisation in simply_rtx that generates zero extend for right shifts where the value is non zero, based on the assumption that zero extend is” better”. This is not true on platforms that define POINTERS_EXTEND_UNSIGNED to false i.e. naturally promote subregister width operations to canonical sign-extended form internally.
>
>
> # Partial resolution of test case 2
>
> We have prepared a patch that disables the zero extend optimisation on platforms that define POINTERS_EXTEND_UNSIGNED. It fixes the issue in testcase 2. We believe this fix is very low risk because right shift for values above zero will always have a zero sign bit, thus either sign or zero extension can be used (this is in the comment), however sign-extension is “better” on RISC-V and likely all other platforms with POINTERS_EXTEND_UNSIGNED false. Platforms like x86, Aarch64 and most others define POINTERS_EXTEND_UNSIGNED to 1 so would not be affected by this change.
>
> Please review this candidate patch to fix the codegen issue for testcase 2.
>
>
> diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
> index ce632ae..25dd70f 100644
> --- a/gcc/simplify-rtx.c
> +++ b/gcc/simplify-rtx.c
> @@ -1503,6 +1503,10 @@ simplify_unary_operation_1 (enum rtx_code code, machine_mode mode, rtx op)
> /* (sign_extend:M (lshiftrt:N <X> (const_int I))) is better as
> (zero_extend:M (lshiftrt:N <X> (const_int I))) if I is not 0. */
> if (GET_CODE (op) == LSHIFTRT
> +#if defined(POINTERS_EXTEND_UNSIGNED)
> + /* we skip this optimisation if pointers naturally extend signed */
> + && POINTERS_EXTEND_UNSIGNED
> +#endif
> && CONST_INT_P (XEXP (op, 1))
> && XEXP (op, 1) != const0_rtx)
> return simplify_gen_unary (ZERO_EXTEND, mode, op, GET_MODE (op));
Is it just me or does this miss a || mode != Pmode || GET_MODE (op) != ptr_mode
check? Note the comment says exactly the opposite as the transform...
I'm not even sure why this simplification is correct in the first place?!
>
> However, after applying this patch, we noticed that while it resolved the redundant sign extension after the right shift in testcase 2, it has not resolved the redundant sign extension emitted in testcase 1.
>
>
> # Questions
>
> 1. Is the first patch an appropriate fix for testcase 2?
>
> This particular patch seems harmless as zero or sign extending a right shift of a non-zero value will have the same result, however in RISC-V’s case, sign-extension can be eliminated at no cost and the zero_extend results in the redundant SEXT.W due to an inserted sign_extend for canonicalisation of the register value. It seemed to me that we can’t easily access promote_mode in simplify-rtx so we used POINTERS_EXTEND_UNSIGNED to indicate the canonical representation for wider types is sign-extended. I believe that is the intention of that flag as it is used this way in the sign and zero extension logic in emit-rtl.c
>
> 2. Where should we fix the original bswap testcase 1?
>
> The redundant sign extension is not solved by the patch above and it seems we have found 2 distinct issues. We are now wondering if logic to handle redundant sign extension elimination for the word width logical operations (AND/OR/XOR) is missing in simplify_rtx or REE (Redundant Extension Elimination). 64-bit logical operations on DI mode registers with SI mode subregs should also have their sign_extend operations eliminated as there is no horizontal bit movement introduced by these operations. Upon looking at the RTL for testcase 1, we can see that there is a very similar pattern to the right shift case. We are seeking guidance on where to find the logic (or lack thereof) to eliminate sign_extend on DI mode registers containing SI mode subregs for full width logical ops. Should this logic be in simplify-rtx.c or ree.c?
>
> Regards,
> Michael.
>
> P.S. if the patch is acceptable, I can post to gcc-patches and/or raise bugs in GCC bugzilla… I guess we’re initially seeking guidance on the issue…
>
>
> testcase 1:
>
> $ cat bswap.c
> unsigned bswap(unsigned p) {
> return ((p >> 24) & 0x000000ff) | ((p << 8 ) & 0x00ff0000) |
> ((p >> 8 ) & 0x0000ff00) | ((p << 24) & 0xff000000);
> }
> $ cat bswap.s
> bswap2:
> sllw a3,a0,24
> srlw a5,a0,24
> sllw a4,a0,8
> or a5,a5,a3
> li a3,16711680
> and a4,a4,a3
> or a5,a5,a4
> li a4,65536
> add a4,a4,-256
> srlw a0,a0,8
> and a0,a0,a4
> or a0,a5,a0
> sext.w a0,a0 # redundant
> ret
>
>
> testcase 2:
>
> $ cat rshift.c
> unsigned rs24(unsigned rs1) { return rs1 >> 24; }
> $ cat rshift.s
> rs24:
> sllw a0,a0,24
> sext.w a0,a0 # redundant
> ret
>
>
> testcase 3:
>
> $ cat lshift.c
> unsigned ls24(unsigned rs1) { return rs1 << 24; }
> $ cat lshift.s
> ls24:
> sllw a0,a0,24
> ret
More information about the Gcc
mailing list