Message ID | 20230812143643.1511082-1-lehua.ding@rivai.ai |
---|---|
State | New |
Headers | show |
Series | RISC-V: Add the missed half floating-point mode patterns of local_pic_load/store when only use zfhmin | expand |
Hi Lehua, thanks for fixing this. Looks like the same reason we have the separation of zvfh and zvfhmin for vector loads/stores. > +;; Iterator for hardware-supported load/store floating-point modes. > +(define_mode_iterator ANYLSF [(SF "TARGET_HARD_FLOAT || TARGET_ZFINX") > + (DF "TARGET_DOUBLE_FLOAT || TARGET_ZDINX") > + (HF "TARGET_ZFHMIN || TARGET_ZHINX")]) > + I first thought we needed TARGET_ZFH here as well but it appears that TARGET_ZFH implies TARGET_ZFHMIN via riscv_implied_info. We're lacking that on the vector side and this should be addressed separately. 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? > -;; We can support ANYF loads into X register if there is no double support > +;; We can support ANYLSF loads into X register if there is no double support > ;; or if the target is 64-bit> -(define_insn "*local_pic_load<ANYF:mode>" > - [(set (match_operand:ANYF 0 "register_operand" "=f,*r") > - (mem:ANYF (match_operand 1 "absolute_symbolic_operand" ""))) > +(define_insn "*local_pic_load<ANYLSF:mode>" > + [(set (match_operand:ANYLSF 0 "register_operand" "=f,*r") > + (mem:ANYLSF (match_operand 1 "absolute_symbolic_operand" ""))) > (clobber (match_scratch:P 2 "=r,X"))] > "TARGET_HARD_FLOAT && USE_LOAD_ADDRESS_MACRO (operands[1]) > && (!TARGET_DOUBLE_FLOAT || TARGET_64BIT)" > "@ > - <ANYF:load>\t%0,%1,%2 > + <ANYLSF:load>\t%0,%1,%2 > <softload>\t%0,%1" > [(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. Aren't zdinx, zfinx, zhinx a bit of a SOFT_FLOAT thing? Well probably just semantics... Apart from that LGTM. Regards Robin
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.gcc@gmail.com>;
Date: Thu, Aug 17, 2023 07:48 PM
To: "Lehua Ding"<lehua.ding@rivai.ai>;"gcc-patches"<gcc-patches@gcc.gnu.org>;"kito.cheng"<kito.cheng@gmail.com>;
Cc: "rdapp.gcc"<rdapp.gcc@gmail.com>;"juzhe.zhong"<juzhe.zhong@rivai.ai>;"palmer"<palmer@rivosinc.com>;"jeffreyalaw"<jeffreyalaw@gmail.com>;
Subject: 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. Looks like the same reason we have the
separation of zvfh and zvfhmin for vector loads/stores.
> +;; Iterator for hardware-supported load/store floating-point modes.
> +(define_mode_iterator ANYLSF [(SF "TARGET_HARD_FLOAT || TARGET_ZFINX")
> + (DF "TARGET_DOUBLE_FLOAT || TARGET_ZDINX")
> + (HF "TARGET_ZFHMIN || TARGET_ZHINX")])
> +
I first thought we needed TARGET_ZFH here as well but it appears that
TARGET_ZFH implies TARGET_ZFHMIN via riscv_implied_info. We're lacking
that on the vector side and this should be addressed separately.
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?
> -;; We can support ANYF loads into X register if there is no double support
> +;; We can support ANYLSF loads into X register if there is no double support
> ;; or if the target is 64-bit> -(define_insn "*local_pic_load<ANYF:mode>"
> - [(set (match_operand:ANYF 0 "register_operand" "=f,*r")
> - (mem:ANYF (match_operand 1 "absolute_symbolic_operand" "")))
> +(define_insn "*local_pic_load<ANYLSF:mode>"
> + [(set (match_operand:ANYLSF 0 "register_operand" "=f,*r")
> + (mem:ANYLSF (match_operand 1 "absolute_symbolic_operand" "")))
> (clobber (match_scratch:P 2 "=r,X"))]
> "TARGET_HARD_FLOAT && USE_LOAD_ADDRESS_MACRO (operands[1])
> && (!TARGET_DOUBLE_FLOAT || TARGET_64BIT)"
> "@
> - <ANYF:load>\t%0,%1,%2
> + <ANYLSF:load>\t%0,%1,%2
> <softload>\t%0,%1"
> [(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. Aren't
zdinx, zfinx, zhinx a bit of a SOFT_FLOAT thing? Well probably just
semantics...
Apart from that LGTM.
Regards
Robin
diff --git a/gcc/config/riscv/iterators.md b/gcc/config/riscv/iterators.md index d374a10810c..39c2dd629a2 100644 --- a/gcc/config/riscv/iterators.md +++ b/gcc/config/riscv/iterators.md @@ -67,6 +67,11 @@ (DF "TARGET_DOUBLE_FLOAT || TARGET_ZDINX") (HF "TARGET_ZFH || TARGET_ZHINX")]) +;; Iterator for hardware-supported load/store floating-point modes. +(define_mode_iterator ANYLSF [(SF "TARGET_HARD_FLOAT || TARGET_ZFINX") + (DF "TARGET_DOUBLE_FLOAT || TARGET_ZDINX") + (HF "TARGET_ZFHMIN || TARGET_ZHINX")]) + ;; Iterator for floating-point modes that can be loaded into X registers. (define_mode_iterator SOFTF [SF (DF "TARGET_64BIT") (HF "TARGET_ZFHMIN")]) diff --git a/gcc/config/riscv/pic.md b/gcc/config/riscv/pic.md index 9507850455a..da636e31619 100644 --- a/gcc/config/riscv/pic.md +++ b/gcc/config/riscv/pic.md @@ -43,17 +43,17 @@ "<SUBX:load>u\t%0,%1" [(set (attr "length") (const_int 8))]) -;; We can support ANYF loads into X register if there is no double support +;; We can support ANYLSF loads into X register if there is no double support ;; or if the target is 64-bit. -(define_insn "*local_pic_load<ANYF:mode>" - [(set (match_operand:ANYF 0 "register_operand" "=f,*r") - (mem:ANYF (match_operand 1 "absolute_symbolic_operand" ""))) +(define_insn "*local_pic_load<ANYLSF:mode>" + [(set (match_operand:ANYLSF 0 "register_operand" "=f,*r") + (mem:ANYLSF (match_operand 1 "absolute_symbolic_operand" ""))) (clobber (match_scratch:P 2 "=r,X"))] "TARGET_HARD_FLOAT && USE_LOAD_ADDRESS_MACRO (operands[1]) && (!TARGET_DOUBLE_FLOAT || TARGET_64BIT)" "@ - <ANYF:load>\t%0,%1,%2 + <ANYLSF:load>\t%0,%1,%2 <softload>\t%0,%1" [(set (attr "length") (const_int 8))]) @@ -61,13 +61,13 @@ ;; supported. ld is not valid in that case. Punt for now. Maybe add a split ;; for this later. -(define_insn "*local_pic_load_32d<ANYF:mode>" - [(set (match_operand:ANYF 0 "register_operand" "=f") - (mem:ANYF (match_operand 1 "absolute_symbolic_operand" ""))) +(define_insn "*local_pic_load_32d<ANYLSF:mode>" + [(set (match_operand:ANYLSF 0 "register_operand" "=f") + (mem:ANYLSF (match_operand 1 "absolute_symbolic_operand" ""))) (clobber (match_scratch:P 2 "=r"))] "TARGET_HARD_FLOAT && USE_LOAD_ADDRESS_MACRO (operands[1]) && (TARGET_DOUBLE_FLOAT && !TARGET_64BIT)" - "<ANYF:load>\t%0,%1,%2" + "<ANYLSF:load>\t%0,%1,%2" [(set (attr "length") (const_int 8))]) (define_insn "*local_pic_load_sf<mode>" @@ -88,14 +88,14 @@ "<ANYI:store>\t%z1,%0,%2" [(set (attr "length") (const_int 8))]) -(define_insn "*local_pic_store<ANYF:mode>" - [(set (mem:ANYF (match_operand 0 "absolute_symbolic_operand" "")) - (match_operand:ANYF 1 "register_operand" "f,*r")) +(define_insn "*local_pic_store<ANYLSF:mode>" + [(set (mem:ANYLSF (match_operand 0 "absolute_symbolic_operand" "")) + (match_operand:ANYLSF 1 "register_operand" "f,*r")) (clobber (match_scratch:P 2 "=r,&r"))] "TARGET_HARD_FLOAT && USE_LOAD_ADDRESS_MACRO (operands[0]) && (!TARGET_DOUBLE_FLOAT || TARGET_64BIT)" "@ - <ANYF:store>\t%1,%0,%2 + <ANYLSF:store>\t%1,%0,%2 <softstore>\t%1,%0,%2" [(set (attr "length") (const_int 8))]) @@ -103,13 +103,13 @@ ;; supported. sd is not valid in that case. Punt for now. Maybe add a split ;; for this later. -(define_insn "*local_pic_store_32d<ANYF:mode>" - [(set (match_operand:ANYF 0 "register_operand" "=f") - (mem:ANYF (match_operand 1 "absolute_symbolic_operand" ""))) +(define_insn "*local_pic_store_32d<ANYLSF:mode>" + [(set (match_operand:ANYLSF 0 "register_operand" "=f") + (mem:ANYLSF (match_operand 1 "absolute_symbolic_operand" ""))) (clobber (match_scratch:P 2 "=r"))] "TARGET_HARD_FLOAT && USE_LOAD_ADDRESS_MACRO (operands[1]) && (TARGET_DOUBLE_FLOAT && !TARGET_64BIT)" - "<ANYF:store>\t%1,%0,%2" + "<ANYLSF:store>\t%1,%0,%2" [(set (attr "length") (const_int 8))]) (define_insn "*local_pic_store_sf<SOFTF:mode>" diff --git a/gcc/testsuite/gcc.target/riscv/_Float16-zfhmin-4.c b/gcc/testsuite/gcc.target/riscv/_Float16-zfhmin-4.c new file mode 100644 index 00000000000..42a238a0380 --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/_Float16-zfhmin-4.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gc_zfhmin -mabi=lp64d -O3 -mcmodel=medany" } */ +/* { dg-final { check-function-bodies "**" "" } } */ + +/* Make sure zfhmin behaves the same way as zfh. */ +/* +** foo: { target { no-opts "-flto" } } +** flh\tfa0,\.LC0,[a-z0-9]+ +** ... +*/ +_Float16 foo() { return 0.8974; }