Re: [PATCH-1, combine] Don't widen shift mode when target has rotate/mask instruction on original mode [PR93738]

[Jeff Law <jeffreyalaw@gmail.com>]

On 7/18/23 21:06, HAO CHEN GUI via Gcc-patches wrote:
> Hi,
>    The shift mode will be widen in combine pass if the operand has a normal
> subreg. But when the target already has rotate/mask/insert instructions on
> the narrow mode, it's unnecessary to widen the mode for lshiftrt. As
> the lshiftrt is commonly converted to rotate/mask insn, the widen mode
> blocks it to be further combined to rotate/mask/insert insn. The PR93738
> shows the case.
> 
> The lshiftrt:SI (subreg:SI (reg:DI)) is converted to
> subreg:SI (lshiftrt:DI (reg:DI)) and fails to match rotate/mask pattern.
> 
> Trying 13, 10 -> 14:
>     13: r127:SI=r125:SI&0xfffffffffffff0ff
>        REG_DEAD r125:SI
>     10: r124:SI=r129:DI#4 0>>0xc&0xf00
>        REG_DEAD r129:DI
>     14: r128:SI=r127:SI|r124:SI
> 
> Failed to match this instruction:
> (set (reg:SI 128)
>      (ior:SI (and:SI (reg:SI 125 [+-2 ])
>              (const_int -3841 [0xfffffffffffff0ff]))
>          (and:SI (subreg:SI (zero_extract:DI (reg:DI 129)
>                      (const_int 32 [0x20])
>                      (const_int 20 [0x14])) 4)
>              (const_int 3840 [0xf00]))))
> Failed to match this instruction:
> (set (reg:SI 128)
>      (ior:SI (and:SI (reg:SI 125 [+-2 ])
>              (const_int -3841 [0xfffffffffffff0ff]))
>          (and:SI (subreg:SI (and:DI (lshiftrt:DI (reg:DI 129)
>                          (const_int 12 [0xc]))
>                      (const_int 4294967295 [0xffffffff])) 4)
>              (const_int 3840 [0xf00]))))
> 
> If not widen the shift mode, it can be combined to rotate/mask/insert insn
> as expected.
> 
> Trying 13, 10 -> 14:
>     13: r127:SI=r125:SI&0xfffffffffffff0ff
>        REG_DEAD r125:SI
>     10: r124:SI=r129:DI#4 0>>0xc&0xf00
>        REG_DEAD r129:DI
>     14: r128:SI=r127:SI|r124:SI
>        REG_DEAD r127:SI
>        REG_DEAD r124:SI
> Successfully matched this instruction:
> (set (reg:SI 128)
>      (ior:SI (and:SI (reg:SI 125 [+-2 ])
>              (const_int -3841 [0xfffffffffffff0ff]))
>          (and:SI (lshiftrt:SI (subreg:SI (reg:DI 129) 4)
>                  (const_int 12 [0xc]))
>              (const_int 3840 [0xf00]))))
> 
> 
>    This patch adds a target hook to indicate if rotate/mask instructions are
> supported on certain mode. If it's true, widen lshiftrt mode is skipped
> and shift is done on original mode.
> 
>    The patch fixes the regression of other rs6000 test cases. They're listed
> in the second patch.
> 
>    The patch passed regression test on Power Linux and x86 platforms.
> 
> Thanks
> Gui Haochen
> 
> ChangeLog
> combine: Not winden shift mode when target has rotate/mask instruction on
> original mode
> 
> To winden shift mode is unnecessary when target already has rotate/mask
> instuctions on the original mode.  It might blocks the further combine
> optimization on the original mode.  For instance, further combine the insns
> to a rotate/mask/insert instruction on the original mode.
> 
> This patch adds a hook to indicate if a target supports rotate/mask
> instructions on the certain mode.  If it returns true, the widen shift
> mode will be skipped on lshiftrt.
> 
> gcc/
> 	PR target/93738
> 	* combine.cc (try_widen_shift_mode): Skip to widen mode for lshiftrt
> 	when the target has rotate/mask instructions on original mode.
> 	* doc/tm.texi: Regenerate.
> 	* doc/tm.texi.in (TARGET_HAVE_ROTATE_AND_MASK): Add.
> 	* target.def (have_rotate_and_mask): New target hook.
> 	* targhooks.cc (default_have_rotate_and_mask): New function.
> 	* targhooks.h (default_have_rotate_and_mask): Declare.
Wouldn't it make more sense to just try rotate/mask in the original mode 
before trying a shift in a widened mode?  I'm not sure why we need a 
target hook here.

jeff