diff mbox series

RISC-V: Fix RISCV_FUSE_ZEXTWS fusion condition

Message ID CAHtqR7Ut6SzciCSpBkUvpN=Rook1q4pOWPZ3uC+tprZRa4YdMQ@mail.gmail.com
State Corrupt patch
Headers
Series RISC-V: Fix RISCV_FUSE_ZEXTWS fusion condition |

Checks

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

Commit Message

Wang Pengcheng Dec. 20, 2023, 8:39 a.m. UTC 
  From: wangpc

The condition is RISCV_FUSE_ZEXTH, which is a mistake.

gcc/ChangeLog:

* config/riscv/riscv.cc (riscv_macro_fusion_pair_p): Fix condition.
---
gcc/config/riscv/riscv.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

prev (slli) == (set (reg:DI rD)

Comments

Jeff Law Dec. 20, 2023, 5:08 p.m. UTC | #1 
On 12/20/23 01:39, Wang Pengcheng wrote:
> From: wangpc
> 
> The condition is RISCV_FUSE_ZEXTH, which is a mistake.
> 
> gcc/ChangeLog:
> 
> * config/riscv/riscv.cc (riscv_macro_fusion_pair_p): Fix condition.
Thanks!  As soon as this patch finishes regression testing I'll push it 
to the trunk.

I'm curious, how did you find this?  Did you see a case where fusible 
ops weren't being kept together or did you find it just by code examination?

Jeff
Jeff Law Dec. 20, 2023, 5:36 p.m. UTC | #2 
On 12/20/23 01:39, Wang Pengcheng wrote:
> The condition is RISCV_FUSE_ZEXTH, which is a mistake.
> 
> gcc/ChangeLog:
> 
> * config/riscv/riscv.cc (riscv_macro_fusion_pair_p): Fix condition.
I've pushed this to the trunk.  Attached is the actual patch committed 
which also fixes formatting of that code.

Thanks again!
jeff
commit eef65d60a8bb2e9328fd9d2b6cd869618be4f08e
Author: Wang Pengcheng <wangpengcheng.pp@bytedance.com>
Date:   Wed Dec 20 10:32:59 2023 -0700

    [PATCH] RISC-V: Fix RISCV_FUSE_ZEXTWS fusion condition
    
    The condition is RISCV_FUSE_ZEXTH, which is a mistake.
    
    gcc/ChangeLog:
    
            * config/riscv/riscv.cc (riscv_macro_fusion_pair_p): Fix condition.

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index d9b45f17a1b..c6784a22127 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -8096,8 +8096,9 @@ riscv_macro_fusion_pair_p (rtx_insn *prev, rtx_insn *curr)
   if (!riscv_macro_fusion_p ())
     return false;
 
-  if (simple_sets_p && (riscv_fusion_enabled_p (RISCV_FUSE_ZEXTW) ||
-			riscv_fusion_enabled_p (RISCV_FUSE_ZEXTH)))
+  if (simple_sets_p
+      && (riscv_fusion_enabled_p (RISCV_FUSE_ZEXTW)
+	  || riscv_fusion_enabled_p (RISCV_FUSE_ZEXTWS)))
     {
       /* We are trying to match the following:
 	   prev (slli) == (set (reg:DI rD)
Wang Pengcheng Dec. 21, 2023, 3:30 a.m. UTC | #3 
On 2023/12/21 1:08, Jeff Law wrote:
>
>
> On 12/20/23 01:39, Wang Pengcheng wrote:
>> From: wangpc
>>
>> The condition is RISCV_FUSE_ZEXTH, which is a mistake.
>>
>> gcc/ChangeLog:
>>
>> * config/riscv/riscv.cc (riscv_macro_fusion_pair_p): Fix condition.
> Thanks!  As soon as this patch finishes regression testing I'll push
> it to the trunk.
>
> I'm curious, how did you find this?  Did you see a case where fusible
> ops weren't being kept together or did you find it just by code
> examination?
>
> Jeff

Yeah, I just found it when I tried to understand the original fusion
implementation commit. :-)

And thanks a lot for formatting and merging the patch! I'm sorry that I
almost forgot the process of contribution to GCC as the last time I did
it is about two years ago.
Jeff Law Dec. 21, 2023, 4:07 a.m. UTC | #4 
On 12/20/23 20:30, Wang Pengcheng wrote:

> 
> Yeah, I just found it when I tried to understand the original fusion
> implementation commit. :-)
Ah.  If you have any questions, don't hesitate to reach out.  While I 
didn't do the original implementation (that was Philipp T. and his 
team), the basics are pretty straightforward and my team has made some 
minor additions to the cases supported.

> 
> And thanks a lot for formatting and merging the patch! I'm sorry that I
> almost forgot the process of contribution to GCC as the last time I did
> it is about two years ago.
No problem.  The goofy formatting slipped through my review when I 
pushed Philipp's code a little while ago.  Figured there wasn't enough 
changing here to warrant a distinct patch for the formatting fix.

Jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 8ae65760b6e..87a62119b01 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -8089,7 +8089,7 @@  riscv_macro_fusion_pair_p (rtx_insn *prev, rtx_insn
*curr)
return false;

if (simple_sets_p && (riscv_fusion_enabled_p (RISCV_FUSE_ZEXTW) ||
- riscv_fusion_enabled_p (RISCV_FUSE_ZEXTH)))
+ riscv_fusion_enabled_p (RISCV_FUSE_ZEXTWS)))
{
/* We are trying to match the following: