[PATCH] haifa-sched: Avoid the fusion priority of the fused insn to affect the subsequent insn sequence.

[PATCH] haifa-sched: Avoid the fusion priority of the fused insn to affect the subsequent insn sequence.

[Jin Ma]

When the insn 1 and 2, 3 and 4 can be fusioned, then there is the
following sequence:

;;    insn |
;;      1  | sp=sp-0x18
;;  +   2  | [sp+0x10]=ra
;;      3  | [sp+0x8]=s0
;;      4  | [sp+0x0]=s1

The fusion priority of the insn 2, 3, and 4 are the same. According to
the current algorithm, since abs(0x10-0x8)<abs(0x10-0x0), the insn 2
is followed by the insn 3. It is obviously unreasonable to do so.

Therefore, when we issue the insn 3 and 4, we should consider the fusion
priority of the insn 1 instead of the insn 2. And the final instruction
sequence is as follows:

;;    insn |
;;      1  | sp=sp-0x18
;;  +   2  | [sp+0x10]=ra
;;      4  | [sp+0x8]=s1
;;  +   3  | [sp+0x0]=s0

gcc/ChangeLog:

	* haifa-sched.cc (rank_for_schedule): Likewise.
---
 gcc/haifa-sched.cc | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/gcc/haifa-sched.cc b/gcc/haifa-sched.cc
index 1bc610f9a5f..0c8c2b18bd2 100644
--- a/gcc/haifa-sched.cc
+++ b/gcc/haifa-sched.cc
@@ -2640,12 +2640,18 @@ rank_for_schedule (const void *x, const void *y)
       int a = INSN_FUSION_PRIORITY (tmp);
       int b = INSN_FUSION_PRIORITY (tmp2);
       int last = -1;
+      rtx_insn *last_insn = last_nondebug_scheduled_insn;
 
-      if (last_nondebug_scheduled_insn
-	  && !NOTE_P (last_nondebug_scheduled_insn)
+      /* If last_insn can be fusioned with the previous insn, then we
+	 should use the previous insn fusion priority and priority.  */
+      if (last_insn && INSN_P (last_insn) && SCHED_GROUP_P (last_insn))
+	last_insn = prev_nonnote_nondebug_insn_bb (last_insn);
+
+      if (last_insn
+	  && !NOTE_P (last_insn)
 	  && BLOCK_FOR_INSN (tmp)
-	       == BLOCK_FOR_INSN (last_nondebug_scheduled_insn))
-	last = INSN_FUSION_PRIORITY (last_nondebug_scheduled_insn);
+	       == BLOCK_FOR_INSN (last_insn))
+	last = INSN_FUSION_PRIORITY (last_insn);
 
       if (a != last && b != last)
 	{
@@ -2662,9 +2668,9 @@ rank_for_schedule (const void *x, const void *y)
 	}
       else if (a == b)
 	{
-	  gcc_assert (last_nondebug_scheduled_insn
-		      && !NOTE_P (last_nondebug_scheduled_insn));
-	  last = INSN_PRIORITY (last_nondebug_scheduled_insn);
+	  gcc_assert (last_insn
+		      && !NOTE_P (last_insn));
+	  last = INSN_PRIORITY (last_insn);
 
 	  a = abs (INSN_PRIORITY (tmp) - last);
 	  b = abs (INSN_PRIORITY (tmp2) - last);
-- 
2.17.1

Re:[PATCH] haifa-sched: Avoid the fusion priority of the fused insn to affect the subsequent insn sequence.

[Jin Ma]

I am very sorry that I did not check the commit information carefully. The statement is somewhat inaccurate.

> When the insn 1 and 2, 3 and 4 can be fusioned, then there is the
> following sequence:
> 
> ;;    insn |
> ;;      1  | sp=sp-0x18
> ;;  +   2  | [sp+0x10]=ra
> ;;      3  | [sp+0x8]=s0
> ;;      4  | [sp+0x0]=s1

> The fusion priority of the insn 2, 3, and 4 are the same. According to
> the current algorithm, since abs(0x10-0x8)<abs(0x10-0x0), the insn 2
> is followed by the insn 3. It is obviously unreasonable to do so.
> 
> Therefore, when we issue the insn 3 and 4, we should consider the fusion
> priority of the insn 1 instead of the insn 2. And the final instruction
> sequence is as follows:

> ;;    insn |
> ;;      1  | sp=sp-0x18
> ;;  +   2  | [sp+0x10]=ra
> ;;      4  | [sp+0x8]=s1
> ;;  +   3  | [sp+0x0]=s0
> 
> gcc/ChangeLog:

>  * haifa-sched.cc (rank_for_schedule): Likewise.

When the insn 1 and 2, 4 and 3 can be fusioned, then there is the
following sequence:

;;    insn |
;;      1  | sp=sp-0x18
;;  +   2  | [sp+0x10]=ra
;;      3  | [sp+0x8]=s0
;;      4  | [sp+0x0]=s1

The fusion priority of the insn 2, 3, and 4 are the same. According to
the current algorithm, since abs(0x10-0x8)<abs(0x10-0x0), the insn 2
is followed by the insn 3. It is obviously unreasonable to do so.

Therefore, when we issue the insn 3 and 4, we should consider the fusion
priority of the insn 1 instead of the insn 2. And the final instruction
sequence is as follows:

;;    insn |
;;      1  | sp=sp-0x18
;;  +   2  | [sp+0x10]=ra
;;      4  | [sp+0x0]=s1
;;  +   3  | [sp+0x8]=s0

gcc/ChangeLog:

 * haifa-sched.cc (rank_for_schedule): Likewise.
---

Sorry again for adding trouble to the review.
Thanks
Jin

Re: [PATCH] haifa-sched: Avoid the fusion priority of the fused insn to affect the subsequent insn sequence.

[Jeff Law]

On 6/6/24 8:51 PM, Jin Ma wrote:
> 
> I am very sorry that I did not check the commit information carefully. The statement is somewhat inaccurate.
> 
>> When the insn 1 and 2, 3 and 4 can be fusioned, then there is the
>> following sequence:
>>
>> ;;    insn |
>> ;;      1  | sp=sp-0x18
>> ;;  +   2  | [sp+0x10]=ra
>> ;;      3  | [sp+0x8]=s0
>> ;;      4  | [sp+0x0]=s1
> 
>> The fusion priority of the insn 2, 3, and 4 are the same. According to
>> the current algorithm, since abs(0x10-0x8)<abs(0x10-0x0), the insn 2
>> is followed by the insn 3. It is obviously unreasonable to do so.
>>
>> Therefore, when we issue the insn 3 and 4, we should consider the fusion
>> priority of the insn 1 instead of the insn 2. And the final instruction
>> sequence is as follows:
> 
>> ;;    insn |
>> ;;      1  | sp=sp-0x18
>> ;;  +   2  | [sp+0x10]=ra
>> ;;      4  | [sp+0x8]=s1
>> ;;  +   3  | [sp+0x0]=s0
>>
>> gcc/ChangeLog:
> 
>>   * haifa-sched.cc (rank_for_schedule): Likewise.
> 
> When the insn 1 and 2, 4 and 3 can be fusioned, then there is the
> following sequence:
> 
> ;;    insn |
> ;;      1  | sp=sp-0x18
> ;;  +   2  | [sp+0x10]=ra
> ;;      3  | [sp+0x8]=s0
> ;;      4  | [sp+0x0]=s1
> 
> The fusion priority of the insn 2, 3, and 4 are the same. According to
> the current algorithm, since abs(0x10-0x8)<abs(0x10-0x0), the insn 2
> is followed by the insn 3. It is obviously unreasonable to do so.
> 
> Therefore, when we issue the insn 3 and 4, we should consider the fusion
> priority of the insn 1 instead of the insn 2. And the final instruction
> sequence is as follows:
> 
> ;;    insn |
> ;;      1  | sp=sp-0x18
> ;;  +   2  | [sp+0x10]=ra
> ;;      4  | [sp+0x0]=s1
> ;;  +   3  | [sp+0x8]=s0
> 
> gcc/ChangeLog:
> 
>   * haifa-sched.cc (rank_for_schedule): Likewise.
I'd really love to see a testcase here, particularly since I'm still 
having trouble understanding the code you're currently getting vs the 
code you want.

Furthermore, I think I need to understand the end motivation here.  I 
always think of fusion priority has bringing insns consecutive so that 
peephole pass can then squash two more more insns into a single insn. 
THe canonical case being load/store pairs.


If you're trying to generate pairs, then that's fine.  I just want to 
make sure I understand the goal.  And if you're trying to generate pairs 
what actually can be paired?  I must admit I don't have any notable 
experience with the thead core extensions.

If you're just trying to keep the instructions consecutive in the IL, 
then I don't think fusion priorities are a significant concern.  Much 
more important for that case is the fusion pair detection (which I think 
is about to get a lot more attention in the near future).

Jeff