Re: [PATCH, RFC] combine: Don't truncate const operand of AND if it's no benefits

[Jeff Law <jeffreyalaw@gmail.com>]

On 3/10/24 11:41 PM, HAO CHEN GUI wrote:
> Hi,
>    This patch tries to fix the problem when a canonical form doesn't benefit
> on a specific target. The const operand of AND is and with the nonzero
> bits of another operand in combine pass. It's a canonical form, but it's no
> benefits for the target which has rotate and mask insns. As the mask is
> truncated, it can't match the insn conditions which it originally matches.
> For example, the following insn condition checks the sum of two AND masks.
> When one of the mask is truncated, the condition breaks.
> 
> (define_insn "*rotlsi3_insert_5"
>    [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r")
> 	(ior:SI (and:SI (match_operand:SI 1 "gpc_reg_operand" "0,r")
> 			(match_operand:SI 2 "const_int_operand" "n,n"))
> 		(and:SI (match_operand:SI 3 "gpc_reg_operand" "r,0")
> 			(match_operand:SI 4 "const_int_operand" "n,n"))))]
>    "rs6000_is_valid_mask (operands[2], NULL, NULL, SImode)
>     && UINTVAL (operands[2]) != 0 && UINTVAL (operands[4]) != 0
>     && UINTVAL (operands[2]) + UINTVAL (operands[4]) + 1 == 0"
> ...
> 
>    This patch tries to fix the problem by comparing the rtx cost. If another
> operand (varop) is not changed and rtx cost with new mask is not less than
> the original one, the mask is restored to original one.
> 
>    I'm not sure if comparison of rtx cost here is proper. The outer code is
> unknown and I suppose it as "SET". Also the rtx cost might not be accurate.
>  From my understanding, the canonical forms should always benefit as it can't
> be undo in combine pass. Do we have a perfect solution for this kind of
> issues? Looking forward for your advice.
> 
>    Another similar issues for canonical forms. Whether the widen mode for
> lshiftrt is always good?
> https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624852.html
> 
> Thanks
> Gui Haochen
> 
> ChangeLog
> Combine: Don't truncate const operand of AND if it's no benefits
> 
> In combine pass, the canonical form is to turn off all bits in the constant
> that are know to already be zero for AND.
> 
>    /* Turn off all bits in the constant that are known to already be zero.
>       Thus, if the AND isn't needed at all, we will have CONSTOP == NONZERO_BITS
>       which is tested below.  */
> 
>    constop &= nonzero;
> 
> But it doesn't benefit when the target has rotate and mask insert insns.
> The AND mask is truncated and lost its information.  Thus it can't match
> the insn conditions.  For example, the following insn condition checks
> the sum of two AND masks.
> 
> (define_insn "*rotlsi3_insert_5"
>    [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r")
> 	(ior:SI (and:SI (match_operand:SI 1 "gpc_reg_operand" "0,r")
> 			(match_operand:SI 2 "const_int_operand" "n,n"))
> 		(and:SI (match_operand:SI 3 "gpc_reg_operand" "r,0")
> 			(match_operand:SI 4 "const_int_operand" "n,n"))))]
>    "rs6000_is_valid_mask (operands[2], NULL, NULL, SImode)
>     && UINTVAL (operands[2]) != 0 && UINTVAL (operands[4]) != 0
>     && UINTVAL (operands[2]) + UINTVAL (operands[4]) + 1 == 0"
> ...
> 
> This patch restores the const operand of AND if the another operand is
> not optimized and the truncated const operand doesn't save the rtx cost.
> 
> gcc/
> 	* combine.cc (simplify_and_const_int_1): Restore the const operand
> 	of AND if varop is not optimized and the rtx cost of the new const
> 	operand is not reduced.
> 
> gcc/testsuite/
> 	* gcc.target/powerpc/rlwimi-0.c: Reduced total number of insns and
> 	adjust the number of rotate and mask insns.
> 	* gcc.target/powerpc/rlwimi-1.c: Likewise.
> 	* gcc.target/powerpc/rlwimi-2.c: Likewise.
It seems like this should defer to gcc-15.

jeff