Re: [PATCH][AArch64] Implement ALU_BRANCH fusion

Re: [PATCH][AArch64] Implement ALU_BRANCH fusion

[Wilco Dijkstra]

Hi,

I think the patch isn't quite complete yet. You will also need changes in
generic code. Currently sched_macro_fuse_insns() does:

  if (any_condjump_p (insn))
    {
      unsigned int condreg1, condreg2;
      rtx cc_reg_1;
      targetm.fixed_condition_code_regs (&condreg1, &condreg2);
      cc_reg_1 = gen_rtx_REG (CCmode, condreg1);
      prev = prev_nonnote_nondebug_insn (insn);
      if (!reg_referenced_p (cc_reg_1, PATTERN (insn))
          || !prev
          || !modified_in_p (cc_reg_1, prev))
        return;
    }

Ie. it explicitly looks for a flag-setting ALU instruction whose condition is
used by a conditional branch, so none of the cases in your patch can match.

Note this code also excludes all CBZ type branches as fusion candidates,
is that intended too?

Wilco

Re: [PATCH][AArch64] Implement ALU_BRANCH fusion

[Andrew Pinski]

On Wed, Mar 15, 2017 at 8:20 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> Hi,
>
> I think the patch isn't quite complete yet. You will also need changes in
> generic code. Currently sched_macro_fuse_insns() does:
>
>   if (any_condjump_p (insn))
>     {
>       unsigned int condreg1, condreg2;
>       rtx cc_reg_1;
>       targetm.fixed_condition_code_regs (&condreg1, &condreg2);
>       cc_reg_1 = gen_rtx_REG (CCmode, condreg1);
>       prev = prev_nonnote_nondebug_insn (insn);
>       if (!reg_referenced_p (cc_reg_1, PATTERN (insn))
>           || !prev
>           || !modified_in_p (cc_reg_1, prev))
>         return;
>     }
>
> Ie. it explicitly looks for a flag-setting ALU instruction whose condition is
> used by a conditional branch, so none of the cases in your patch can match.
>
> Note this code also excludes all CBZ type branches as fusion candidates,
> is that intended too?

It is not intended that way; I did not even notice it after the
previous changes to make the macro_fusion more generic.  I wonder how
this code ever worked for the folks before we started to touch it :).

Naveen,
  Basically the idea is to push the check for CC usage into the target
macros (macro_fusion_pair_p in i386.c and aarch64.c are the only usage
of compare/branch fusion) instead of keeping it in the general code.

Also in aarch64.c's macro fusion you need check that the branch
instruction uses the same register as the other instruction sets like
the other code in this area.

Thanks,
Andrew Pinski



>
> Wilco

Re: [PATCH][AArch64] Implement ALU_BRANCH fusion

[Hurugalawadi, Naveen]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

Hi,

Thanks for the review and suggestions.

> I think the patch isn't quite complete yet. You will also need changes in
> generic code. Currently sched_macro_fuse_insns() does:

Modified the sched_macro_fuse_insns() as required.

> Basically the idea is to push the check for CC usage into target macros

Done. Pushed the check into target macros.

The modifications were generic and and quite different from ALU+BRANCH
fusion; a separate patch is posted with the above 2 modifications at:-
https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01368.html

> Also in aarch64.c's macro fusion you need check that the branch
>> instruction uses the same register 

Added to check that same registers are used in ALU and Branch instruction.

Bootstrapped and Regression tested on AArch64.

Please review the patch and let us know if its okay?

Thanks,
Naveen

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: alu-branch-4.patch --]
[-- Type: text/x-patch; name="alu-branch-4.patch", Size: 2359 bytes --]

diff --git a/gcc/config/aarch64/aarch64-fusion-pairs.def b/gcc/config/aarch64/aarch64-fusion-pairs.def
index f0e6dbc..300cd00 100644
--- a/gcc/config/aarch64/aarch64-fusion-pairs.def
+++ b/gcc/config/aarch64/aarch64-fusion-pairs.def
@@ -34,5 +34,6 @@ AARCH64_FUSION_PAIR ("movk+movk", MOVK_MOVK)
 AARCH64_FUSION_PAIR ("adrp+ldr", ADRP_LDR)
 AARCH64_FUSION_PAIR ("cmp+branch", CMP_BRANCH)
 AARCH64_FUSION_PAIR ("aes+aesmc", AES_AESMC)
+AARCH64_FUSION_PAIR ("alu+branch", ALU_BRANCH)
 
 #undef AARCH64_FUSION_PAIR
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 4f769a4..31bc5f4 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -792,7 +792,8 @@ static const struct tune_params thunderx2t99_tunings =
   &generic_approx_modes,
   4, /* memmov_cost.  */
   4, /* issue_rate.  */
-  (AARCH64_FUSE_CMP_BRANCH | AARCH64_FUSE_AES_AESMC), /* fusible_ops  */
+  (AARCH64_FUSE_CMP_BRANCH | AARCH64_FUSE_AES_AESMC
+   | AARCH64_FUSE_ALU_BRANCH), /* fusible_ops  */
   16,	/* function_align.  */
   8,	/* jump_align.  */
   16,	/* loop_align.  */
@@ -13981,6 +13982,50 @@ aarch_macro_fusion_pair_p (rtx_insn *prev, rtx_insn *curr)
         return true;
     }
 
+  if (aarch64_fusion_enabled_p (AARCH64_FUSE_ALU_BRANCH)
+      && any_condjump_p (curr))
+    {
+      /* We're trying to match:
+	  prev (alu_insn) == (set (r0) plus ((r0) (r1/imm)))
+	  curr (cbz) ==  (set (pc) (if_then_else (eq/ne) (r0)
+							 (const_int 0))
+						 (label_ref ("SYM"))
+						 (pc))  */
+
+      if (SET_DEST (curr_set) != (pc_rtx)
+	  || GET_CODE (SET_SRC (curr_set)) != IF_THEN_ELSE
+	  || ! REG_P (XEXP (XEXP (SET_SRC (curr_set), 0), 0))
+	  || ! REG_P (SET_DEST (prev_set))
+	  || REGNO (SET_DEST (prev_set))
+	     != REGNO (XEXP (XEXP (SET_SRC (curr_set), 0), 0)))
+	return false;
+
+      /* Fuse ALU operations followed by conditional branch instruction.  */
+      switch (get_attr_type (prev))
+	{
+	case TYPE_ALU_IMM:
+	case TYPE_ALU_SREG:
+	case TYPE_ADC_REG:
+	case TYPE_ADC_IMM:
+	case TYPE_ADCS_REG:
+	case TYPE_ADCS_IMM:
+	case TYPE_LOGIC_REG:
+	case TYPE_LOGIC_IMM:
+	case TYPE_CSEL:
+	case TYPE_ADR:
+	case TYPE_MOV_IMM:
+	case TYPE_SHIFT_REG:
+	case TYPE_SHIFT_IMM:
+	case TYPE_BFM:
+	case TYPE_RBIT:
+	case TYPE_REV:
+	case TYPE_EXTEND:
+	  return true;
+
+	default:;
+	}
+    }
+
   return false;
 }
 

Re: [PING][PATCH][AArch64] Implement ALU_BRANCH fusion

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01369.html

Thanks,
Naveen



    

Re: [PING][PATCH][AArch64] Implement ALU_BRANCH fusion

[Wilco Dijkstra]

Hi Naveen,

> https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01369.html

Same comment for this part, we want to return true if we match:

+      if (SET_DEST (curr_set) != (pc_rtx)
+	  || GET_CODE (SET_SRC (curr_set)) != IF_THEN_ELSE
+	  || ! REG_P (XEXP (XEXP (SET_SRC (curr_set), 0), 0))
+	  || ! REG_P (SET_DEST (prev_set))
+	  || REGNO (SET_DEST (prev_set))
+	     != REGNO (XEXP (XEXP (SET_SRC (curr_set), 0), 0)))
+	return false;

Note writing these complex conditions using positive logic makes them much
more readable - if you have to negate use !(X && Y && Z) rather than !X || !Y || !Z.

Wilco



        

Re: [PING][PATCH][AArch64] Implement ALU_BRANCH fusion

[Hurugalawadi, Naveen]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

Hi Wilco,

>> Same comment for this part, we want to return true if we match:

Thanks for pointing out about the confusion.

>> Note writing these complex conditions using positive logic makes them much
>> more readable - if you have to negate use !(X && Y && Z) rather than
>> !X || !Y || !Z.

Modified the code as required.

Bootstrapped and Regression tested on AArch64 and X86_64.
Please review the patch and let us know if its okay?

Thanks,
Naveen         

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: alu-branch-5.patch --]
[-- Type: text/x-diff; name="alu-branch-5.patch", Size: 2365 bytes --]

diff --git a/gcc/config/aarch64/aarch64-fusion-pairs.def b/gcc/config/aarch64/aarch64-fusion-pairs.def
index f0e6dbc..300cd00 100644
--- a/gcc/config/aarch64/aarch64-fusion-pairs.def
+++ b/gcc/config/aarch64/aarch64-fusion-pairs.def
@@ -34,5 +34,6 @@ AARCH64_FUSION_PAIR ("movk+movk", MOVK_MOVK)
 AARCH64_FUSION_PAIR ("adrp+ldr", ADRP_LDR)
 AARCH64_FUSION_PAIR ("cmp+branch", CMP_BRANCH)
 AARCH64_FUSION_PAIR ("aes+aesmc", AES_AESMC)
+AARCH64_FUSION_PAIR ("alu+branch", ALU_BRANCH)
 
 #undef AARCH64_FUSION_PAIR
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 2e385c4..1a63ad0 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -792,7 +792,8 @@ static const struct tune_params thunderx2t99_tunings =
   &generic_approx_modes,
   4, /* memmov_cost.  */
   4, /* issue_rate.  */
-  (AARCH64_FUSE_CMP_BRANCH | AARCH64_FUSE_AES_AESMC), /* fusible_ops  */
+  (AARCH64_FUSE_CMP_BRANCH | AARCH64_FUSE_AES_AESMC
+   | AARCH64_FUSE_ALU_BRANCH), /* fusible_ops  */
   16,	/* function_align.  */
   8,	/* jump_align.  */
   16,	/* loop_align.  */
@@ -13982,6 +13992,49 @@ aarch_macro_fusion_pair_p (rtx_insn *prev, rtx_insn *curr)
         return true;
     }
 
+  if (aarch64_fusion_enabled_p (AARCH64_FUSE_ALU_BRANCH)
+      && any_condjump_p (curr))
+    {
+      /* We're trying to match:
+	  prev (alu_insn) == (set (r0) plus ((r0) (r1/imm)))
+	  curr (cbz) ==  (set (pc) (if_then_else (eq/ne) (r0)
+							 (const_int 0))
+						 (label_ref ("SYM"))
+						 (pc))  */
+      if (! (SET_DEST (curr_set) == (pc_rtx)
+	     && GET_CODE (SET_SRC (curr_set)) == IF_THEN_ELSE
+	     && REG_P (XEXP (XEXP (SET_SRC (curr_set), 0), 0))
+	     && REG_P (SET_DEST (prev_set))
+	     && REGNO (SET_DEST (prev_set))
+		== REGNO (XEXP (XEXP (SET_SRC (curr_set), 0), 0))))
+	return true;
+
+      /* Fuse ALU operations followed by conditional branch instruction.  */
+      switch (get_attr_type (prev))
+	{
+	case TYPE_ALU_IMM:
+	case TYPE_ALU_SREG:
+	case TYPE_ADC_REG:
+	case TYPE_ADC_IMM:
+	case TYPE_ADCS_REG:
+	case TYPE_ADCS_IMM:
+	case TYPE_LOGIC_REG:
+	case TYPE_LOGIC_IMM:
+	case TYPE_CSEL:
+	case TYPE_ADR:
+	case TYPE_MOV_IMM:
+	case TYPE_SHIFT_REG:
+	case TYPE_SHIFT_IMM:
+	case TYPE_BFM:
+	case TYPE_RBIT:
+	case TYPE_REV:
+	case TYPE_EXTEND:
+	  return true;
+
+	default:;
+	}
+    }
+
   return false;
 }
 

Re: [PING][PATCH][AArch64] Implement ALU_BRANCH fusion

[Wilco Dijkstra]

Hi Naveen,

This version has the same issue of claiming that all instructions should
be fused except for the cases that can be fused. You should only return
true if there is a match, not if there is not a match.

Cheers,
Wilco
           

Re: [PING][PATCH][AArch64] Implement ALU_BRANCH fusion

[Hurugalawadi, Naveen]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

Hi Wilco,

>> You should only return true if there is a match, not if there is
>> not a match.

Done.

Bootstrapped and Regression tested on AArch64 and X86_64.
Please review the patch and let us know if its okay?

Thanks,
Naveen
               

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: alu-branch-6.patch --]
[-- Type: text/x-patch; name="alu-branch-6.patch", Size: 2421 bytes --]

diff --git a/gcc/config/aarch64/aarch64-fusion-pairs.def b/gcc/config/aarch64/aarch64-fusion-pairs.def
index f0e6dbc..300cd00 100644
--- a/gcc/config/aarch64/aarch64-fusion-pairs.def
+++ b/gcc/config/aarch64/aarch64-fusion-pairs.def
@@ -34,5 +34,6 @@ AARCH64_FUSION_PAIR ("movk+movk", MOVK_MOVK)
 AARCH64_FUSION_PAIR ("adrp+ldr", ADRP_LDR)
 AARCH64_FUSION_PAIR ("cmp+branch", CMP_BRANCH)
 AARCH64_FUSION_PAIR ("aes+aesmc", AES_AESMC)
+AARCH64_FUSION_PAIR ("alu+branch", ALU_BRANCH)
 
 #undef AARCH64_FUSION_PAIR
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 1e58e9d..d3b66f2 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -792,7 +792,8 @@ static const struct tune_params thunderx2t99_tunings =
   &generic_approx_modes,
   4, /* memmov_cost.  */
   4, /* issue_rate.  */
-  (AARCH64_FUSE_CMP_BRANCH | AARCH64_FUSE_AES_AESMC), /* fusible_ops  */
+  (AARCH64_FUSE_CMP_BRANCH | AARCH64_FUSE_AES_AESMC
+   | AARCH64_FUSE_ALU_BRANCH), /* fusible_ops  */
   16,	/* function_align.  */
   8,	/* jump_align.  */
   16,	/* loop_align.  */
@@ -14031,6 +14032,49 @@ aarch_macro_fusion_pair_p (rtx_insn *prev, rtx_insn *curr)
         return true;
     }
 
+  if (aarch64_fusion_enabled_p (AARCH64_FUSE_ALU_BRANCH)
+      && any_condjump_p (curr))
+    {
+      /* We're trying to match:
+	  prev (alu_insn) == (set (r0) plus ((r0) (r1/imm)))
+	  curr (cbz) ==  (set (pc) (if_then_else (eq/ne) (r0)
+							 (const_int 0))
+						 (label_ref ("SYM"))
+						 (pc))  */
+      if (SET_DEST (curr_set) == (pc_rtx)
+	  && GET_CODE (SET_SRC (curr_set)) == IF_THEN_ELSE
+	  && REG_P (XEXP (XEXP (SET_SRC (curr_set), 0), 0))
+	  && REG_P (SET_DEST (prev_set))
+	  && REGNO (SET_DEST (prev_set))
+	     == REGNO (XEXP (XEXP (SET_SRC (curr_set), 0), 0)))
+	{
+	  /* Fuse ALU operations followed by conditional branch instruction.  */
+	  switch (get_attr_type (prev))
+	    {
+	    case TYPE_ALU_IMM:
+	    case TYPE_ALU_SREG:
+	    case TYPE_ADC_REG:
+	    case TYPE_ADC_IMM:
+	    case TYPE_ADCS_REG:
+	    case TYPE_ADCS_IMM:
+	    case TYPE_LOGIC_REG:
+	    case TYPE_LOGIC_IMM:
+	    case TYPE_CSEL:
+	    case TYPE_ADR:
+	    case TYPE_MOV_IMM:
+	    case TYPE_SHIFT_REG:
+	    case TYPE_SHIFT_IMM:
+	    case TYPE_BFM:
+	    case TYPE_RBIT:
+	    case TYPE_REV:
+	    case TYPE_EXTEND:
+	      return true;
+
+	    default:;
+	    }
+	}
+    }
+
   return false;
 }
 

Re: [PING] [PATCH] [AArch64] Implement ALU_BRANCH fusion

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01333.html

Thanks,
Naveen
                   

Re: [PING 2] [PATCH] [AArch64] Implement ALU_BRANCH fusion

[Hurugalawadi, Naveen]

Hi,  

Please consider this as a personal reminder to review the patch
at following link and let me know your comments on the same.  

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01333.html

Thanks,
Naveen
                       

Re: [PING 2] [PATCH] [AArch64] Implement ALU_BRANCH fusion

[Wilco Dijkstra]

Hurugalawadi, Naveen <Naveen.Hurugalawadi@cavium.com> wrote:
>
> Please consider this as a personal reminder to review the patch
> at following link and let me know your comments on the same.  
>
> https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01333.html

Looks good to me.

Wilco
                           

Re: [PING 3] [PATCH] [AArch64] Implement ALU_BRANCH fusion

[Hurugalawadi, Naveen]

Hi Wilco,

>> That looks good to me now.

Thanks for the review and your okay for the patch.

Please consider this as a personal reminder to review the patch
at following link and let me know if its okay to commit?

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01333.html

Thanks,
Naveen

Re: [PING][PATCH][AArch64] Implement ALU_BRANCH fusion

[James Greenhalgh]

On Thu, Apr 27, 2017 at 05:07:26AM +0000, Hurugalawadi, Naveen wrote:
> Hi Wilco,
> 
> >> You should only return true if there is a match, not if there is
> >> not a match.
> 
> Done.
> 
> Bootstrapped and Regression tested on AArch64 and X86_64.
> Please review the patch and let us know if its okay?

OK.

Thanks,
James

> 
> Thanks,
> Naveen
>