Re: [Patch][Aarch64]PR rtl-optimization/87763 - Fix lsl_asr_sbfiz.c test by checking for subregs
Hi, Segher wrote: >On Tue, Jan 29, 2019 at 02:51:30PM -0800, Andrew Pinski wrote: > >> Seems to me rather this should have been simplified to just: >> (set (reg:SI 93) >> (ashift:SI (sign_extract:SI (reg:SI 95) >> (const_int 3 [0x3]) >> (const_int 0 [0])) >> (const_int 19 [0x13]))) > > Yes. > Because the two subreg cancel each other out. > Well, why did it ever think of using DI at all? I looked at this last week - the underlying issue is due to using extv optab without a mode. This then defaults to DI mode only. There is a newer extv optab, and using that to enable zero/sign_extract for SImode fixes this particular issue. However this triggers a bug in expmed using SI->HF lvalue-subregs which isn't legal. Unfortunately generated code after the change ends up worse overall, so it's not clear how to fix this. >> This would be a thing to add to simplify-rtx.c. > > This is probably specific to combine actually. Simplifying the subreg still won't allow the pattern to match since we don't enable sign_extract for SImode. Cheers, Wilco
Re: [Patch][Aarch64]PR rtl-optimization/87763 - Fix lsl_asr_sbfiz.c test by checking for subregs
On Tue, Jan 29, 2019 at 02:51:30PM -0800, Andrew Pinski wrote: > On Tue, Jan 29, 2019 at 2:36 PM Steve Ellcey wrote: > > So the various tests that started failing with r265398 seem to need > > different fixes. This particular fix is for the > > gcc.target/aarch64/lsl_asr_sbfiz.c failure. The problem is that the > > instructions we are trying to match to *ashiftsi_extv_bfiz now have > > explicit subregs in them where they didn't before. The new version > > is: > > > > (set (reg:SI 93) > > (ashift:SI (subreg:SI (sign_extract:DI (subreg:DI (reg:SI 95) 0) > > (const_int 3 [0x3]) > > (const_int 0 [0])) 0) > > (const_int 19 [0x13]))) > > > > > > The subreg's were not there before. My proposed fix is to add an new > > instruction like *ashiftsi_extv_bfiz but with the subregs. This fixes > > lsl_asr_sbfiz.c. Does this seem like the right way to fix this? > > Seems to me rather this should have been simplified to just: > (set (reg:SI 93) > (ashift:SI (sign_extract:SI (reg:SI 95) > (const_int 3 [0x3]) > (const_int 0 [0])) > (const_int 19 [0x13]))) Yes. > Because the two subreg cancel each other out. Well, why did it ever think of using DI at all? > This would be a thing to add to simplify-rtx.c. This is probably specific to combine actually. Segher
Re: [Patch][Aarch64]PR rtl-optimization/87763 - Fix lsl_asr_sbfiz.c test by checking for subregs
On Tue, Jan 29, 2019 at 2:36 PM Steve Ellcey wrote: > > So the various tests that started failing with r265398 seem to need > different fixes. This particular fix is for the > gcc.target/aarch64/lsl_asr_sbfiz.c failure. The problem is that the > instructions we are trying to match to *ashiftsi_extv_bfiz now have > explicit subregs in them where they didn't before. The new version > is: > > (set (reg:SI 93) > (ashift:SI (subreg:SI (sign_extract:DI (subreg:DI (reg:SI 95) 0) > (const_int 3 [0x3]) > (const_int 0 [0])) 0) > (const_int 19 [0x13]))) > > > The subreg's were not there before. My proposed fix is to add an new > instruction like *ashiftsi_extv_bfiz but with the subregs. This fixes > lsl_asr_sbfiz.c. Does this seem like the right way to fix this? Seems to me rather this should have been simplified to just: (set (reg:SI 93) (ashift:SI (sign_extract:SI (reg:SI 95) (const_int 3 [0x3]) (const_int 0 [0])) (const_int 19 [0x13]))) Because the two subreg cancel each other out. This would be a thing to add to simplify-rtx.c. Thanks, Andrew > > Steve Ellcey > sell...@marvell.com > > > 2018-01-29 Steve Ellcey > > PR rtl-optimization/87763 > * config/aarch64/aarch64.md (*ashiftsi_extv_bfiz_alt): > New Instruction. > > diff --git a/gcc/config/aarch64/aarch64.md > b/gcc/config/aarch64/aarch64.md > index b7f6fe0f135..d65230c4837 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -5531,6 +5531,22 @@ >[(set_attr "type" "bfx")] > ) > > +(define_insn "*ashiftsi_extv_bfiz_alt" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (ashift:SI > + (subreg:SI > + (sign_extract:DI > + (subreg:DI (match_operand:SI 1 "register_operand" "r") 0) > + (match_operand 2 "aarch64_simd_shift_imm_offset_si" "n") > + (const_int 0)) > + 0) > + (match_operand 3 "aarch64_simd_shift_imm_si" "n")))] > + "IN_RANGE (INTVAL (operands[2]) + INTVAL (operands[3]), > +1, GET_MODE_BITSIZE (SImode) - 1)" > + "sbfiz\\t%w0, %w1, %3, %2" > + [(set_attr "type" "bfx")] > +) > + > ;; When the bit position and width of the equivalent extraction add up > to 32 > ;; we can use a W-reg LSL instruction taking advantage of the implicit > ;; zero-extension of the X-reg.
[Patch][Aarch64]PR rtl-optimization/87763 - Fix lsl_asr_sbfiz.c test by checking for subregs
So the various tests that started failing with r265398 seem to need different fixes. This particular fix is for the gcc.target/aarch64/lsl_asr_sbfiz.c failure. The problem is that the instructions we are trying to match to *ashiftsi_extv_bfiz now have explicit subregs in them where they didn't before. The new version is: (set (reg:SI 93) (ashift:SI (subreg:SI (sign_extract:DI (subreg:DI (reg:SI 95) 0) (const_int 3 [0x3]) (const_int 0 [0])) 0) (const_int 19 [0x13]))) The subreg's were not there before. My proposed fix is to add an new instruction like *ashiftsi_extv_bfiz but with the subregs. This fixes lsl_asr_sbfiz.c. Does this seem like the right way to fix this? Steve Ellcey sell...@marvell.com 2018-01-29 Steve Ellcey PR rtl-optimization/87763 * config/aarch64/aarch64.md (*ashiftsi_extv_bfiz_alt): New Instruction. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b7f6fe0f135..d65230c4837 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -5531,6 +5531,22 @@ [(set_attr "type" "bfx")] ) +(define_insn "*ashiftsi_extv_bfiz_alt" + [(set (match_operand:SI 0 "register_operand" "=r") + (ashift:SI + (subreg:SI + (sign_extract:DI + (subreg:DI (match_operand:SI 1 "register_operand" "r") 0) + (match_operand 2 "aarch64_simd_shift_imm_offset_si" "n") + (const_int 0)) + 0) + (match_operand 3 "aarch64_simd_shift_imm_si" "n")))] + "IN_RANGE (INTVAL (operands[2]) + INTVAL (operands[3]), +1, GET_MODE_BITSIZE (SImode) - 1)" + "sbfiz\\t%w0, %w1, %3, %2" + [(set_attr "type" "bfx")] +) + ;; When the bit position and width of the equivalent extraction add up to 32 ;; we can use a W-reg LSL instruction taking advantage of the implicit ;; zero-extension of the X-reg.
