Re: [PATCH] RISC-V: Add the missed half floating-point mode patterns of local_pic_load/store when only use zfhmin

[Lehua Ding]

Hi Robin,


> You likely want TARGET_ZHINXMIN instead of ZHINX though?  I mean the
> hardware support is obviously always there but the patterns should
> be available for the min extension already.  Please double check as
> I haven't worked with that extension before.
> Our test coverage for the *inx extensions is honestly a bit sparse,
> maybe you would also want to add a testcase for a similar scenario?


Indeed, thanks for the reminder. I'll add the missing ones and add V2 patch.


> Unrelated to your patch - but from a quick glimpse here I didn't see
> why we require TARGET_HARD_FLOAT for the softload alternatives.  
Aren't
> zdinx, zfinx, zhinx a bit of a SOFT_FLOAT thing?  Well probably just
> semantics...


Looking closely at this condition is a bit odd for me too.




Best,
Lehua


------------------ Original ------------------
From:                                                                           
                                             "Robin Dapp"                       
                                                             
<rdapp....@gmail.com&gt;;
Date:&nbsp;Thu, Aug 17, 2023 07:48 PM
To:&nbsp;"Lehua 
Ding"<lehua.d...@rivai.ai&gt;;"gcc-patches"<gcc-patches@gcc.gnu.org&gt;;"kito.cheng"<kito.ch...@gmail.com&gt;;
Cc:&nbsp;"rdapp.gcc"<rdapp....@gmail.com&gt;;"juzhe.zhong"<juzhe.zh...@rivai.ai&gt;;"palmer"<pal...@rivosinc.com&gt;;"jeffreyalaw"<jeffreya...@gmail.com&gt;;
Subject:&nbsp;Re: [PATCH] RISC-V: Add the missed half floating-point mode 
patterns of local_pic_load/store when only use zfhmin



Hi Lehua,

thanks for fixing this.&nbsp; Looks like the same reason we have the
separation of zvfh and zvfhmin for vector loads/stores.

&gt; +;; Iterator for hardware-supported load/store floating-point modes.
&gt; +(define_mode_iterator ANYLSF [(SF "TARGET_HARD_FLOAT || TARGET_ZFINX")
&gt; + &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (DF "TARGET_DOUBLE_FLOAT || TARGET_ZDINX")
&gt; + &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (HF "TARGET_ZFHMIN || TARGET_ZHINX")])
&gt; +

I first thought we needed TARGET_ZFH here as well but it appears that
TARGET_ZFH implies TARGET_ZFHMIN via riscv_implied_info.&nbsp; We're lacking
that on the vector side and this should be addressed separately.

You likely want TARGET_ZHINXMIN instead of ZHINX though?&nbsp; I mean the
hardware support is obviously always there but the patterns should
be available for the min extension already.&nbsp; Please double check as
I haven't worked with that extension before.
Our test coverage for the *inx extensions is honestly a bit sparse,
maybe you would also want to add a testcase for a similar scenario?

&gt; -;; We can support ANYF loads into X register if there is no double support
&gt; +;; We can support ANYLSF loads into X register if there is no double 
support
&gt;&nbsp; ;; or if the target is 64-bit&gt; -(define_insn 
"*local_pic_load<ANYF:mode&gt;"
&gt; -&nbsp; [(set (match_operand:ANYF 0 "register_operand" "=f,*r")
&gt; - (mem:ANYF (match_operand 1 "absolute_symbolic_operand" "")))
&gt; +(define_insn "*local_pic_load<ANYLSF:mode&gt;"
&gt; +&nbsp; [(set (match_operand:ANYLSF 0 "register_operand" "=f,*r")
&gt; + (mem:ANYLSF (match_operand 1 "absolute_symbolic_operand" "")))
&gt;&nbsp;&nbsp;&nbsp;&nbsp; (clobber (match_scratch:P 2 "=r,X"))]
&gt;&nbsp;&nbsp;&nbsp; "TARGET_HARD_FLOAT &amp;&amp; USE_LOAD_ADDRESS_MACRO 
(operands[1])
&gt;&nbsp;&nbsp;&nbsp;&nbsp; &amp;&amp; (!TARGET_DOUBLE_FLOAT || TARGET_64BIT)"
&gt;&nbsp;&nbsp;&nbsp; "@
&gt; -&nbsp;&nbsp; <ANYF:load&gt;\t%0,%1,%2
&gt; +&nbsp;&nbsp; <ANYLSF:load&gt;\t%0,%1,%2
&gt;&nbsp;&nbsp;&nbsp;&nbsp; <softload&gt;\t%0,%1"
&gt;&nbsp;&nbsp;&nbsp; [(set (attr "length") (const_int 8))])

Unrelated to your patch - but from a quick glimpse here I didn't see
why we require TARGET_HARD_FLOAT for the softload alternatives.&nbsp; Aren't
zdinx, zfinx, zhinx a bit of a SOFT_FLOAT thing?&nbsp; Well probably just
semantics... 

Apart from that LGTM.

Regards
Robin