[v1] RISC-V: Fix one typo of FRM dynamic definition

Message ID 20230703080805.1297008-1-pan2.li@intel.com
State Unresolved
Headers
Series [v1] RISC-V: Fix one typo of FRM dynamic definition |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Li, Pan2 via Gcc-patches July 3, 2023, 8:08 a.m. UTC
  From: Pan Li <pan2.li@intel.com>

This patch would like to fix one typo that take rdn instead of dyn by
mistake.

Signed-off-by: Pan Li <pan2.li@intel.com>

gcc/ChangeLog:

	* config/riscv/vector.md: Fix typo.
---
 gcc/config/riscv/vector.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

Comments

juzhe.zhong@rivai.ai July 3, 2023, 8:09 a.m. UTC | #1
Thanks for fixing it. LGTM.
I think you can merge it when Robin is ok since this is a simple typo fix.

Thanks.


juzhe.zhong@rivai.ai
 
From: pan2.li
Date: 2023-07-03 16:08
To: gcc-patches
CC: juzhe.zhong; jeffreyalaw; pan2.li; yanzhang.wang; kito.cheng
Subject: [PATCH v1] RISC-V: Fix one typo of FRM dynamic definition
From: Pan Li <pan2.li@intel.com>
 
This patch would like to fix one typo that take rdn instead of dyn by
mistake.
 
Signed-off-by: Pan Li <pan2.li@intel.com>
 
gcc/ChangeLog:
 
* config/riscv/vector.md: Fix typo.
---
gcc/config/riscv/vector.md | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
 
diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md
index a6174f9483e..2864475b35a 100644
--- a/gcc/config/riscv/vector.md
+++ b/gcc/config/riscv/vector.md
@@ -491,8 +491,8 @@ (define_attr "frm_mode" "rne,rtz,rdn,rup,rmm,dyn,none"
  (match_test "INTVAL (operands[9]) == riscv_vector::FRM_RMM")
  (const_string "rmm")
-   (match_test "INTVAL (operands[9]) == riscv_vector::FRM_RDN")
-   (const_string "rdn")
+   (match_test "INTVAL (operands[9]) == riscv_vector::FRM_DYN")
+   (const_string "dyn")
]
(const_string "none")
       )
-- 
2.34.1
  
Robin Dapp July 3, 2023, 8:14 a.m. UTC | #2
> Thanks for fixing it. LGTM.
> I think you can merge it when Robin is ok since this is a simple typo
> fix.

Yes, that's definitely simple enough :)

Regards
 Robin
  
Li, Pan2 via Gcc-patches July 3, 2023, 8:17 a.m. UTC | #3
Committed, thanks Robin and Juzhe.

Pan

-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com> 
Sent: Monday, July 3, 2023 4:15 PM
To: juzhe.zhong@rivai.ai; Li, Pan2 <pan2.li@intel.com>; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: rdapp.gcc@gmail.com; jeffreyalaw <jeffreyalaw@gmail.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng <kito.cheng@gmail.com>
Subject: Re: [PATCH v1] RISC-V: Fix one typo of FRM dynamic definition


> Thanks for fixing it. LGTM.
> I think you can merge it when Robin is ok since this is a simple typo
> fix.

Yes, that's definitely simple enough :)

Regards
 Robin
  
Robin Dapp July 3, 2023, 2:49 p.m. UTC | #4
Hmm, looks like it wasn't simple enough...

I'm seeing execution fails for various floating point test cases.
This is due to a mismatch between the FRM_DYN definition (0b111 == 7)
and the attribute value (== 5).  Therefore we set the rounding mode
to 5 instead of 7.

Regards
 Robin
  
Li, Pan2 via Gcc-patches July 3, 2023, 2:53 p.m. UTC | #5
Sorry for inconvenient, still working on fix it. If urgent I can revert this change to unblock your work ASAP.

Pan

-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com> 
Sent: Monday, July 3, 2023 10:49 PM
To: Li, Pan2 <pan2.li@intel.com>; juzhe.zhong@rivai.ai; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: rdapp.gcc@gmail.com; jeffreyalaw <jeffreyalaw@gmail.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng <kito.cheng@gmail.com>
Subject: Re: [PATCH v1] RISC-V: Fix one typo of FRM dynamic definition

Hmm, looks like it wasn't simple enough...

I'm seeing execution fails for various floating point test cases.
This is due to a mismatch between the FRM_DYN definition (0b111 == 7)
and the attribute value (== 5).  Therefore we set the rounding mode
to 5 instead of 7.

Regards
 Robin
  
Robin Dapp July 3, 2023, 2:56 p.m. UTC | #6
> Sorry for inconvenient, still working on fix it. If urgent I can
> revert this change to unblock your work ASAP.

I'm not blocked by this, thanks, just wanted to document it here.
I was testing another patch and needed to dig for a while until
I realized the FAILs come from this one.  In general I would
assume that even obvious patches are tested before (I have
introduced bugs by obvious ones before so I make sure to).

Regards
 Robin
  
Li, Pan2 via Gcc-patches July 3, 2023, 2:59 p.m. UTC | #7
Sure, every change need test and will pay attention for this in future.

Pan

-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com> 
Sent: Monday, July 3, 2023 10:57 PM
To: Li, Pan2 <pan2.li@intel.com>; juzhe.zhong@rivai.ai; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: rdapp.gcc@gmail.com; jeffreyalaw <jeffreyalaw@gmail.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng <kito.cheng@gmail.com>
Subject: Re: [PATCH v1] RISC-V: Fix one typo of FRM dynamic definition

> Sorry for inconvenient, still working on fix it. If urgent I can
> revert this change to unblock your work ASAP.

I'm not blocked by this, thanks, just wanted to document it here.
I was testing another patch and needed to dig for a while until
I realized the FAILs come from this one.  In general I would
assume that even obvious patches are tested before (I have
introduced bugs by obvious ones before so I make sure to).

Regards
 Robin
  
Li, Pan2 via Gcc-patches July 4, 2023, 3:21 a.m. UTC | #8
Hi Robin,

Just revert this patch, it reports some weird illegal instr, I may need more time for this.

Pan

-----Original Message-----
From: Li, Pan2 
Sent: Monday, July 3, 2023 11:00 PM
To: Robin Dapp <rdapp.gcc@gmail.com>; juzhe.zhong@rivai.ai; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: jeffreyalaw <jeffreyalaw@gmail.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng <kito.cheng@gmail.com>
Subject: RE: [PATCH v1] RISC-V: Fix one typo of FRM dynamic definition

Sure, every change need test and will pay attention for this in future.

Pan

-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com> 
Sent: Monday, July 3, 2023 10:57 PM
To: Li, Pan2 <pan2.li@intel.com>; juzhe.zhong@rivai.ai; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: rdapp.gcc@gmail.com; jeffreyalaw <jeffreyalaw@gmail.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng <kito.cheng@gmail.com>
Subject: Re: [PATCH v1] RISC-V: Fix one typo of FRM dynamic definition

> Sorry for inconvenient, still working on fix it. If urgent I can
> revert this change to unblock your work ASAP.

I'm not blocked by this, thanks, just wanted to document it here.
I was testing another patch and needed to dig for a while until
I realized the FAILs come from this one.  In general I would
assume that even obvious patches are tested before (I have
introduced bugs by obvious ones before so I make sure to).

Regards
 Robin
  
Robin Dapp July 4, 2023, 1:14 p.m. UTC | #9
> Just revert this patch, it reports some weird illegal instr, I may
> need more time for this.

The illegal instruction is due to the wrong rounding mode.  We set
5 instead of 7 because the two enums don't match.  A simple but ugly
fix would be two dummy entries so that FRM_MODE_DYN is entry 7 in
enum attr_frm_mode but I'd prefer not to do that :) 

Regards
 Robin
  
Li, Pan2 via Gcc-patches July 4, 2023, 1:18 p.m. UTC | #10
Yes, thanks Robin, we can move to another mail thread for this, avoiding misleading.

Pan

-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com> 
Sent: Tuesday, July 4, 2023 9:15 PM
To: Li, Pan2 <pan2.li@intel.com>; juzhe.zhong@rivai.ai; gcc-patches <gcc-patches@gcc.gnu.org>
Cc: rdapp.gcc@gmail.com; jeffreyalaw <jeffreyalaw@gmail.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng <kito.cheng@gmail.com>
Subject: Re: [PATCH v1] RISC-V: Fix one typo of FRM dynamic definition

> Just revert this patch, it reports some weird illegal instr, I may
> need more time for this.

The illegal instruction is due to the wrong rounding mode.  We set
5 instead of 7 because the two enums don't match.  A simple but ugly
fix would be two dummy entries so that FRM_MODE_DYN is entry 7 in
enum attr_frm_mode but I'd prefer not to do that :) 

Regards
 Robin
  

Patch

diff --git a/gcc/config/riscv/vector.md b/gcc/config/riscv/vector.md
index a6174f9483e..2864475b35a 100644
--- a/gcc/config/riscv/vector.md
+++ b/gcc/config/riscv/vector.md
@@ -491,8 +491,8 @@  (define_attr "frm_mode" "rne,rtz,rdn,rup,rmm,dyn,none"
 	  (match_test "INTVAL (operands[9]) == riscv_vector::FRM_RMM")
 	  (const_string "rmm")
 
-	  (match_test "INTVAL (operands[9]) == riscv_vector::FRM_RDN")
-	  (const_string "rdn")
+	  (match_test "INTVAL (operands[9]) == riscv_vector::FRM_DYN")
+	  (const_string "dyn")
 	]
 	(const_string "none")
       )