Re: [PATCH] rs6000: Guard bifs {un, }pack_{longdouble, ibm128} under hard float [PR103623]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Segher,

on 2022/4/9 1:31 AM, Segher Boessenkool wrote:
> On Fri, Apr 08, 2022 at 10:09:44AM +0800, Kewen.Lin wrote:
>> As Jakub noted here, we don't have the soft-float support for both m32 and m64
>> before, as the bifs are always guarded under hard-float previously.
> 
> But that bug was fixed for GCC 12.  Or we thought so, at least :-(
> 

Actually it wasn't fixed due to the incomplete support.  :(

>>>> What makes it ICE on (at least some configurations of) 32-bit now?  Can
>>>> you exclude just 32-bit soft float?
>>
>> As clarified above, both 32-bit and 64-bit has the same root cause for the ICE,
>> the existing define_insn* supports for these bifs only consider hard-float, such
>> as for the given test case in the PR, it fails in reload as the recognized
>> unpacktf_nodm doesn't have any available alternatives at soft-float.  eg: we only
>> have register constraint "d" for
>>   (match_operand:FMOVE128 1 "register_operand" "d,d") 
>> but it's only available for hard-float.
> 
> For me it fails during combine: the unspec suddenly doesn't recog
> anymore.  That might be that "d" thing yes, that is problematical.
> 

I must miss something, I found in combine pass we still have the
the insn_code unpacktf_nodm (recog-ed).

> If you want to add "nosoft" now, please add a FIXME comment everywhere
> you do, so we do not forget to fix this for GCC 13.
> 

Does the patch v3 look good to you?

https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593053.html

> Or, try this patch?
> 
> ===
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index fdfbc6566a5c..f05b8358ba0a 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -14580,10 +14580,10 @@ (define_insn_and_split "unpack<mode>_dm"
>    [(set_attr "type" "fp,fpstore,mtvsr,mfvsr,store")])
>  
>  (define_insn_and_split "unpack<mode>_nodm"
> -  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m")
> +  [(set (match_operand:<FP128_64> 0 "nonimmediate_operand" "=d,m,m")
>         (unspec:<FP128_64>
> -        [(match_operand:FMOVE128 1 "register_operand" "d,d")
> -         (match_operand:QI 2 "const_0_to_1_operand" "i,i")]
> +        [(match_operand:FMOVE128 1 "register_operand" "d,d,r")
> +         (match_operand:QI 2 "const_0_to_1_operand" "i,i,i")]
>          UNSPEC_UNPACK_128BIT))]
>    "(!TARGET_POWERPC64 || !TARGET_DIRECT_MOVE) && FLOAT128_2REG_P (<MODE>mode)"
>    "#"
> @@ -14600,7 +14600,7 @@ (define_insn_and_split "unpack<mode>_nodm"
>  
>    operands[3] = gen_rtx_REG (<FP128_64>mode, fp_regno);
>  }
> -  [(set_attr "type" "fp,fpstore")])
> +  [(set_attr "type" "fp,fpstore,store")])
>  
>  (define_insn_and_split "pack<mode>"
>    [(set (match_operand:FMOVE128 0 "register_operand" "=&d")
> ===
> 
> 

Nice, I confirmed this makes ICE gone, I've filed one new PR
PR105213 for GCC13 further tracking by associating this patch there.

BR,
Kewen