diff mbox series

[rs6000] PR target/89112 [8/9 Regression] fix bdnzt pattern for long branch case

Message ID 59261c98-2250-8cb7-7627-9c93a38a593c@linux.ibm.com
State New
Headers show
Series [rs6000] PR target/89112 [8/9 Regression] fix bdnzt pattern for long branch case | expand

Commit Message

Aaron Sawdey Feb. 2, 2019, 11:17 p.m. UTC 
I needed to introduce a local label in this pattern because output_cbranch put out a second instruction
in the long branch case. This fixes the issue but there are a couple ways this could be improved:

* output_cbranch() is passed the original insn and assumes from that that the branch is a long
branch. However this is incorrect because we are just branching to a local label we know is only
a few instructions away. If there is a way to fix this, an unnecessary branch could be eliminated.

* While the long branch case of this pattern needs to work, the real problem is that part of
the code emitted by the memcmp expansion is being treated as cold code and moved to the end of
the function. Ideally all of this code should stay together. I suspect I need to make some kind
of branch frequency notation for this to happen.

Regstrap passes on ppc64le power7/8/9, ok for trunk and backport to 8?

Thanks!

2019-02-02  Aaron Sawdey  <acsawdey@linux.ibm.com>

	* config/rs6000/rs6000.md (<bd>tf_<mode>): generate a local label
	for the long branch case.

Comments

Segher Boessenkool Feb. 3, 2019, 12:45 a.m. UTC | #1 
On Sat, Feb 02, 2019 at 05:17:31PM -0600, Aaron Sawdey wrote:
> I needed to introduce a local label in this pattern because output_cbranch put out a second instruction
> in the long branch case. This fixes the issue but there are a couple ways this could be improved:
> 
> * output_cbranch() is passed the original insn and assumes from that that the branch is a long
> branch. However this is incorrect because we are just branching to a local label we know is only
> a few instructions away. If there is a way to fix this, an unnecessary branch could be eliminated.

Maybe output_cbranch can be made aware of bdz{,t,f} and bdnz{,t,f}?

> * While the long branch case of this pattern needs to work, the real problem is that part of
> the code emitted by the memcmp expansion is being treated as cold code and moved to the end of
> the function. Ideally all of this code should stay together. I suspect I need to make some kind
> of branch frequency notation for this to happen.

You can emit a REG_BR_PROB note on the branches that need one?

> Regstrap passes on ppc64le power7/8/9, ok for trunk and backport to 8?

I pre-approved it in the PR, the messages crossed I think :-)

But, hrm.  Labels ".L<number>" are already used for something else, with
something else for <number>.  Please use a unique name?  Okay with that
change.  Thanks!


Segher


> 2019-02-02  Aaron Sawdey  <acsawdey@linux.ibm.com>
> 
> 	* config/rs6000/rs6000.md (<bd>tf_<mode>): generate a local label
> 	for the long branch case.
> 
> Index: gcc/config/rs6000/rs6000.md
> ===================================================================
> --- gcc/config/rs6000/rs6000.md	(revision 268403)
> +++ gcc/config/rs6000/rs6000.md	(working copy)
> @@ -12639,8 +12639,8 @@
>    else
>      {
>        static char seq[96];
> -      char *bcs = output_cbranch (operands[3], "$+8", 1, insn);
> -      sprintf(seq, "<bd_neg> $+12\;%s;b %%l0", bcs);
> +      char *bcs = output_cbranch (operands[3], ".L%=", 1, insn);
> +      sprintf(seq, "<bd_neg> .L%%=\;%s\;b %%l0\;.L%%=:", bcs);
>        return seq;
>      }
>  }
diff mbox series

Patch

Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 268403)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -12639,8 +12639,8 @@ 
   else
     {
       static char seq[96];
-      char *bcs = output_cbranch (operands[3], "$+8", 1, insn);
-      sprintf(seq, "<bd_neg> $+12\;%s;b %%l0", bcs);
+      char *bcs = output_cbranch (operands[3], ".L%=", 1, insn);
+      sprintf(seq, "<bd_neg> .L%%=\;%s\;b %%l0\;.L%%=:", bcs);
       return seq;
     }
 }