Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors

[guojiufu <guojiufu@linux.ibm.com>]

Hi,

On 2023-06-09 16:00, Richard Biener wrote:
> On Fri, 9 Jun 2023, Jiufu Guo wrote:
> 
>> Hi,
>> 
>> As checking the code, there is a "gcc_assert (SCALAR_INT_MODE_P 
>> (mode))"
>> in "try_const_anchors".
>> This assert seems correct because the function try_const_anchors cares
>> about integer values currently, and modes other than SCALAR_INT_MODE_P
>> are not needed to support.
>> 
>> This patch makes sure SCALAR_INT_MODE_P when calling 
>> try_const_anchors.
>> 
>> This patch is raised when drafting below one.
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html.
>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into
>> try_const_anchors, and hits the assert/ice.
>> 
>> Boostrap and regtest pass on ppc64{,le} and x86_64.
>> Is this ok for trunk?
> 
> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then
> I suggest to instead fix try_const_anchors to change
> 
>   /* CONST_INT is used for CC modes, but we should leave those alone.  
> */
>   if (GET_MODE_CLASS (mode) == MODE_CC)
>     return NULL_RTX;
> 
>   gcc_assert (SCALAR_INT_MODE_P (mode));
> 
> to
> 
>   /* CONST_INT is used for CC modes, leave any non-scalar-int mode 
> alone.  */
>   if (!SCALAR_INT_MODE_P (mode))
>     return NULL_RTX;
> 

This is also able to fix this issue.  there is a "Punt on CC modes" 
patch
to return NULL_RTX in try_const_anchors.

> but as said I wonder how we arrive at a BLKmode CONST_INT and whether
> we should have fended this off earlier.  Can you share more complete
> RTL of that stack_tie?


(insn 15 14 16 3 (parallel [
             (set (mem/c:BLK (reg/f:DI 1 1) [1  A8])
                 (const_int 0 [0]))
         ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie}
      (nil))

It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])".

This is generated by:

rs6000.md
(define_expand "restore_stack_block"
   [(set (match_dup 2) (match_dup 3))
    (set (match_dup 4) (match_dup 2))
    (match_dup 5)
    (set (match_operand 0 "register_operand")
         (match_operand 1 "register_operand"))]
   ""
{
   rtvec p;

   operands[1] = force_reg (Pmode, operands[1]);
   operands[2] = gen_reg_rtx (Pmode);
   operands[3] = gen_frame_mem (Pmode, operands[0]);
   operands[4] = gen_frame_mem (Pmode, operands[1]);
   p = rtvec_alloc (1);
   RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
                                   const0_rtx);
   operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
})

This kind of case (like BLK with const0) is rare, but this would be an 
intended
RTL, and seems not invalid.

Thanks so much for your quick and very helpful comments!!

BR,
Jeff (Jiufu Guo)


> 
>> 
>> BR,
>> Jeff (Jiufu Guo)
>> 
>> gcc/ChangeLog:
>> 
>> 	* cse.cc (cse_insn): Add SCALAR_INT_MODE_P condition.
>> 
>> ---
>>  gcc/cse.cc | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gcc/cse.cc b/gcc/cse.cc
>> index 2bb63ac4105..f213fa0faf7 100644
>> *** a/gcc/cse.cc
>> --- b/gcc/cse.cc
>> ***************
>> *** 5003,5009 ****
>>         if (targetm.const_anchor
>>   	  && !src_related
>>   	  && src_const
>> ! 	  && GET_CODE (src_const) == CONST_INT)
>>   	{
>>   	  src_related = try_const_anchors (src_const, mode);
>>   	  src_related_is_const_anchor = src_related != NULL_RTX;
>> - -
>> --- 5003,5010 ----
>>         if (targetm.const_anchor
>>   	  && !src_related
>>   	  && src_const
>> ! 	  && GET_CODE (src_const) == CONST_INT
>> ! 	  && SCALAR_INT_MODE_P (mode))
>>   	{
>>   	  src_related = try_const_anchors (src_const, mode);
>>   	  src_related_is_const_anchor = src_related != NULL_RTX;
>> 2.39.3
>> 
>>