Re: [PATCH] rs6000: Fix PR67344
On Tue, Aug 25, 2015 at 1:08 PM, Segher Boessenkool seg...@kernel.crashing.org wrote: The *andmode3_imm_dot_shifted pattern is a define_insn_and_split, like most dot patterns: if its output is not assigned cr0 but some other cr reg, it splits to a non-dot insn and a compare. Unfortunately that non-dot insn will clobber cr0 as well. We could add another clobber (with =X,x), but then that second alternative is never useful; instead, just remove that second alternative. Bootstrapped and tested on powerpc64-linux; is this okay for trunk? Segher 2015-08-25 Segher Boessenkool seg...@kernel.crashing.org PR target/67344 * config/rs6000/rs6000.md (*andmode3_imm_dot_shifted): Change to a define_insn, remove second alternative. Okay. Thanks, David
[PATCH] rs6000: Fix PR67344
The *andmode3_imm_dot_shifted pattern is a define_insn_and_split,
like most dot patterns: if its output is not assigned cr0 but some
other cr reg, it splits to a non-dot insn and a compare.
Unfortunately that non-dot insn will clobber cr0 as well. We could
add another clobber (with =X,x), but then that second alternative
is never useful; instead, just remove that second alternative.
Bootstrapped and tested on powerpc64-linux; is this okay for trunk?
Segher
2015-08-25 Segher Boessenkool seg...@kernel.crashing.org
PR target/67344
* config/rs6000/rs6000.md (*andmode3_imm_dot_shifted): Change to
a define_insn, remove second alternative.
---
gcc/config/rs6000/rs6000.md | 29 -
1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 527ad98..2138184 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -3037,15 +3037,15 @@ (define_insn_and_split *andmode3_imm_mask_dot2
(set_attr dot yes)
(set_attr length 4,8)])
-(define_insn_and_split *andmode3_imm_dot_shifted
- [(set (match_operand:CC 3 cc_reg_operand =x,?y)
+(define_insn *andmode3_imm_dot_shifted
+ [(set (match_operand:CC 3 cc_reg_operand =x)
(compare:CC
(and:GPR
- (lshiftrt:GPR (match_operand:GPR 1 gpc_reg_operand %r,r)
- (match_operand:SI 4 const_int_operand n,n))
- (match_operand:GPR 2 const_int_operand n,n))
+ (lshiftrt:GPR (match_operand:GPR 1 gpc_reg_operand %r)
+ (match_operand:SI 4 const_int_operand n))
+ (match_operand:GPR 2 const_int_operand n))
(const_int 0)))
- (clobber (match_scratch:GPR 0 =r,r))]
+ (clobber (match_scratch:GPR 0 =r))]
logical_const_operand (GEN_INT (UINTVAL (operands[2])
INTVAL (operands[4])),
DImode)
@@ -3054,23 +3054,10 @@ (define_insn_and_split *andmode3_imm_dot_shifted
rs6000_gen_cell_microcode
{
operands[2] = GEN_INT (UINTVAL (operands[2]) INTVAL (operands[4]));
- if (which_alternative == 0)
-return andi%e2. %0,%1,%u2;
- else
-return #;
+ return andi%e2. %0,%1,%u2;
}
- reload_completed cc_reg_not_cr0_operand (operands[3], CCmode)
- [(set (match_dup 0)
- (and:GPR (lshiftrt:GPR (match_dup 1)
- (match_dup 4))
-(match_dup 2)))
- (set (match_dup 3)
- (compare:CC (match_dup 0)
- (const_int 0)))]
-
[(set_attr type logical)
- (set_attr dot yes)
- (set_attr length 4,8)])
+ (set_attr dot yes)])
(define_insn andmode3_mask
--
1.8.1.4
Re: [PATCH] rs6000: Fix PR67344
On Wed, Aug 26, 2015 at 08:40:49AM +0930, Alan Modra wrote:
On Tue, Aug 25, 2015 at 10:08:54AM -0700, Segher Boessenkool wrote:
-(define_insn_and_split *andmode3_imm_dot_shifted
- [(set (match_operand:CC 3 cc_reg_operand =x,?y)
+(define_insn *andmode3_imm_dot_shifted
+ [(set (match_operand:CC 3 cc_reg_operand =x)
Is this really the best solution? The operand predicate allows any
cr, but the constraint only cr0. In the past we've seen this sort of
thing result in insn does not satisfy its constraints errors,
I don't think that can happen here. It is the same situation as
gpc_reg_operand with a b constraint.
and if
the operand is successfully reloaded you'll get slow mcrf insns.
What is slow about those? It's not mfcr :-)
Also note that prior compilers (at least as far back as 4.7) generated
it here, too (at -O2 etc. even).
At
-O1 the testcase generates:
andi. 8,3,0x16
mcrf 4,0
I started throwing together a patch yesterday, before you claimed the
bug. With this patch, I see what looks to be better code despite it
being larger:
li 9,22
and 9,3,9
cmpdi 4,9,0
The alternative that I was trying to avoid is
andi. 8,3,0x16
cmpdi 4,8,0
which doesn't have such a long dependency chain.
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 87abd6e..a9eea80 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -3060,17 +3060,18 @@
return #;
}
reload_completed cc_reg_not_cr0_operand (operands[3], CCmode)
- [(set (match_dup 0)
- (and:GPR (lshiftrt:GPR (match_dup 1)
-(match_dup 4))
- (match_dup 2)))
+ [(set (match_dup 0) (match_dup 2))
This won't work for 0xa000 etc.
+ (set (match_dup 0) (and:GPR (match_dup 1) (match_dup 0)))
(set (match_dup 3)
(compare:CC (match_dup 0)
(const_int 0)))]
-
+
+{
+ operands[2] = GEN_INT (UINTVAL (operands[2]) INTVAL (operands[4]));
+}
[(set_attr type logical)
(set_attr dot yes)
- (set_attr length 4,8)])
+ (set_attr length 4,12)])
Segher
Re: [PATCH] rs6000: Fix PR67344
On Tue, Aug 25, 2015 at 10:08:54AM -0700, Segher Boessenkool wrote:
-(define_insn_and_split *andmode3_imm_dot_shifted
- [(set (match_operand:CC 3 cc_reg_operand =x,?y)
+(define_insn *andmode3_imm_dot_shifted
+ [(set (match_operand:CC 3 cc_reg_operand =x)
Is this really the best solution? The operand predicate allows any
cr, but the constraint only cr0. In the past we've seen this sort of
thing result in insn does not satisfy its constraints errors, and if
the operand is successfully reloaded you'll get slow mcrf insns. At
-O1 the testcase generates:
andi. 8,3,0x16
mcrf 4,0
I started throwing together a patch yesterday, before you claimed the
bug. With this patch, I see what looks to be better code despite it
being larger:
li 9,22
and 9,3,9
cmpdi 4,9,0
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 87abd6e..a9eea80 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -3060,17 +3060,18 @@
return #;
}
reload_completed cc_reg_not_cr0_operand (operands[3], CCmode)
- [(set (match_dup 0)
- (and:GPR (lshiftrt:GPR (match_dup 1)
- (match_dup 4))
-(match_dup 2)))
+ [(set (match_dup 0) (match_dup 2))
+ (set (match_dup 0) (and:GPR (match_dup 1) (match_dup 0)))
(set (match_dup 3)
(compare:CC (match_dup 0)
(const_int 0)))]
-
+
+{
+ operands[2] = GEN_INT (UINTVAL (operands[2]) INTVAL (operands[4]));
+}
[(set_attr type logical)
(set_attr dot yes)
- (set_attr length 4,8)])
+ (set_attr length 4,12)])
(define_insn andmode3_mask
--
Alan Modra
Australia Development Lab, IBM
Re: [PATCH] rs6000: Fix PR67344
On Tue, Aug 25, 2015 at 07:09:48PM -0500, Segher Boessenkool wrote: On Wed, Aug 26, 2015 at 08:40:49AM +0930, Alan Modra wrote: On Tue, Aug 25, 2015 at 10:08:54AM -0700, Segher Boessenkool wrote: -(define_insn_and_split *andmode3_imm_dot_shifted - [(set (match_operand:CC 3 cc_reg_operand =x,?y) +(define_insn *andmode3_imm_dot_shifted + [(set (match_operand:CC 3 cc_reg_operand =x) Is this really the best solution? The operand predicate allows any cr, but the constraint only cr0. In the past we've seen this sort of thing result in insn does not satisfy its constraints errors, I don't think that can happen here. It is the same situation as gpc_reg_operand with a b constraint. Yeah, I'm not sure that it could happen. and if the operand is successfully reloaded you'll get slow mcrf insns. What is slow about those? It's not mfcr :-) mcrf has a 3 cycle latency on p8, I believe. Also note that prior compilers (at least as far back as 4.7) generated it here, too (at -O2 etc. even). At -O1 the testcase generates: andi. 8,3,0x16 mcrf 4,0 I started throwing together a patch yesterday, before you claimed the bug. With this patch, I see what looks to be better code despite it being larger: li 9,22 and 9,3,9 cmpdi 4,9,0 The alternative that I was trying to avoid is andi. 8,3,0x16 cmpdi 4,8,0 which doesn't have such a long dependency chain. But does destroy cr0. + [(set (match_dup 0) (match_dup 2)) This won't work for 0xa000 etc. Oops, yes, you're right. -- Alan Modra Australia Development Lab, IBM
