Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 16 January 2015 at 11:54, Marcus Shawcroft
marcus.shawcr...@gmail.com wrote:
On 15 January 2015 at 18:18, Richard Henderson r...@redhat.com wrote:
On 12/15/2014 12:41 AM, Zhenqiang Chen wrote:
+(define_expand cmpmode
+ [(set (match_operand 0 cc_register )
+(match_operator:CC 1 aarch64_comparison_operator
+ [(match_operand:GPI 2 register_operand )
+ (match_operand:GPI 3 aarch64_plus_operand )]))]
+
+
+ operands[1] = gen_rtx_fmt_ee (COMPARE, SELECT_CC_MODE (GET_CODE
(operands[1]),
+ operands[2],
+ operands[3]),
+ operands[2], operands[3]);
+
+)
Use { } not for the C portion.
Otherwise ok.
Jiong... this is fine with me. /Marcus
Jiong,
I have noticed regressions on aarch64 after this patch:
See:
http://abe.tcwglab.linaro.org/logs/validations/cross-validation/gcc/trunk/219723/report-build-info.html
Passed now fails [PASS = FAIL]:
gcc.target/aarch64/test_frame_1.c scan-assembler-times ldr\tx30,
\\[sp\\], [0-9]+ 3
gcc.target/aarch64/test_frame_2.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
gcc.target/aarch64/test_frame_4.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
gcc.target/aarch64/test_frame_6.c scan-assembler-times ldr\tx30,
\\[sp\\], [0-9]+ 3
gcc.target/aarch64/test_frame_7.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
Is this expected?
Thanks,
Christophe.
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On Sun, Jan 18, 2015 at 11:58 AM, Christophe Lyon
christophe.l...@linaro.org wrote:
On 16 January 2015 at 11:54, Marcus Shawcroft
marcus.shawcr...@gmail.com wrote:
On 15 January 2015 at 18:18, Richard Henderson r...@redhat.com wrote:
On 12/15/2014 12:41 AM, Zhenqiang Chen wrote:
+(define_expand cmpmode
+ [(set (match_operand 0 cc_register )
+(match_operator:CC 1 aarch64_comparison_operator
+ [(match_operand:GPI 2 register_operand )
+ (match_operand:GPI 3 aarch64_plus_operand )]))]
+
+
+ operands[1] = gen_rtx_fmt_ee (COMPARE, SELECT_CC_MODE (GET_CODE
(operands[1]),
+ operands[2],
+ operands[3]),
+ operands[2], operands[3]);
+
+)
Use { } not for the C portion.
Otherwise ok.
Jiong... this is fine with me. /Marcus
Jiong,
I have noticed regressions on aarch64 after this patch:
See:
http://abe.tcwglab.linaro.org/logs/validations/cross-validation/gcc/trunk/219723/report-build-info.html
Passed now fails [PASS = FAIL]:
gcc.target/aarch64/test_frame_1.c scan-assembler-times ldr\tx30,
\\[sp\\], [0-9]+ 3
gcc.target/aarch64/test_frame_2.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
gcc.target/aarch64/test_frame_4.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
gcc.target/aarch64/test_frame_6.c scan-assembler-times ldr\tx30,
\\[sp\\], [0-9]+ 3
gcc.target/aarch64/test_frame_7.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
Is this expected?
Yes and now you just have to revert the revert of my patch to fix those.
Thanks,
Andrew
Thanks,
Christophe.
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 18 January 2015 at 21:22, Andrew Pinski pins...@gmail.com wrote:
On Sun, Jan 18, 2015 at 11:58 AM, Christophe Lyon
christophe.l...@linaro.org wrote:
On 16 January 2015 at 11:54, Marcus Shawcroft
marcus.shawcr...@gmail.com wrote:
On 15 January 2015 at 18:18, Richard Henderson r...@redhat.com wrote:
On 12/15/2014 12:41 AM, Zhenqiang Chen wrote:
+(define_expand cmpmode
+ [(set (match_operand 0 cc_register )
+(match_operator:CC 1 aarch64_comparison_operator
+ [(match_operand:GPI 2 register_operand )
+ (match_operand:GPI 3 aarch64_plus_operand )]))]
+
+
+ operands[1] = gen_rtx_fmt_ee (COMPARE, SELECT_CC_MODE (GET_CODE
(operands[1]),
+ operands[2],
+ operands[3]),
+ operands[2], operands[3]);
+
+)
Use { } not for the C portion.
Otherwise ok.
Jiong... this is fine with me. /Marcus
Jiong,
I have noticed regressions on aarch64 after this patch:
See:
http://abe.tcwglab.linaro.org/logs/validations/cross-validation/gcc/trunk/219723/report-build-info.html
Passed now fails [PASS = FAIL]:
gcc.target/aarch64/test_frame_1.c scan-assembler-times ldr\tx30,
\\[sp\\], [0-9]+ 3
gcc.target/aarch64/test_frame_2.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
gcc.target/aarch64/test_frame_4.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
gcc.target/aarch64/test_frame_6.c scan-assembler-times ldr\tx30,
\\[sp\\], [0-9]+ 3
gcc.target/aarch64/test_frame_7.c scan-assembler-times ldp\tx19,
x30, \\[sp\\], [0-9]+ 2
Is this expected?
Yes and now you just have to revert the revert of my patch to fix those.
Thanks for the clarification/confirmation, I thought I had seen
something like that but I was confused.
Thanks,
Andrew
Thanks,
Christophe.
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 15 January 2015 at 18:18, Richard Henderson r...@redhat.com wrote:
On 12/15/2014 12:41 AM, Zhenqiang Chen wrote:
+(define_expand cmpmode
+ [(set (match_operand 0 cc_register )
+(match_operator:CC 1 aarch64_comparison_operator
+ [(match_operand:GPI 2 register_operand )
+ (match_operand:GPI 3 aarch64_plus_operand )]))]
+
+
+ operands[1] = gen_rtx_fmt_ee (COMPARE, SELECT_CC_MODE (GET_CODE
(operands[1]),
+ operands[2],
+ operands[3]),
+ operands[2], operands[3]);
+
+)
Use { } not for the C portion.
Otherwise ok.
Jiong... this is fine with me. /Marcus
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 12/15/2014 12:41 AM, Zhenqiang Chen wrote:
+(define_expand cmpmode
+ [(set (match_operand 0 cc_register )
+(match_operator:CC 1 aarch64_comparison_operator
+ [(match_operand:GPI 2 register_operand )
+ (match_operand:GPI 3 aarch64_plus_operand )]))]
+
+
+ operands[1] = gen_rtx_fmt_ee (COMPARE, SELECT_CC_MODE (GET_CODE
(operands[1]),
+ operands[2],
+ operands[3]),
+ operands[2], operands[3]);
+
+)
Use { } not for the C portion.
Otherwise ok.
r~
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 15/12/14 08:41, Zhenqiang Chen wrote:
-Original Message-
From: Richard Henderson [mailto:r...@redhat.com]
Sent: Saturday, December 13, 2014 3:26 AM
To: Zhenqiang Chen
Cc: Marcus Shawcroft; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
- tree lhs = gimple_assign_lhs (g);
enum machine_mode mode = TYPE_MODE (TREE_TYPE (lhs));
rtx target = gen_reg_rtx (mode);
+
+ start_sequence ();
tmp = emit_cstore (target, icode, NE, cc_mode, cc_mode,
0, tmp, const0_rtx, 1, mode);
if (tmp)
- return tmp;
+ {
+ rtx seq = get_insns ();
+ end_sequence ();
+ emit_insn (prep_seq);
+ emit_insn (gen_seq);
+ emit_insn (seq);
+ return tmp;
+ }
+ end_sequence ();
Given that you're already doing delete_insns_since (last) at the end of this
function, I don't think you need a new sequence around the emit_cstore.
Just
emit_insn (prep_seq);
emit_insn (gen_seq);
tmp = emit_cstore (...);
if (tmp)
return tmp;
Updated.
+ int unsignedp = code == LTU || code == LEU || code == GTU || code
+ == GEU;
You don't need to examine the code, you can look at the argument:
TYPE_UNSIGNED (TREE_TYPE (treeop0))
Updated.
+ op0 = prepare_operand (icode, op0, 2, op_mode, cmp_mode,
+ unsignedp);
+ op1 = prepare_operand (icode, op1, 3, op_mode, cmp_mode,
+ unsignedp); if (!op0 || !op1)
+{
+ end_sequence ();
+ return NULL_RTX;
+}
+ *prep_seq = get_insns ();
+ end_sequence ();
+
+ cmp = gen_rtx_fmt_ee ((enum rtx_code) code, cmp_mode, op0, op1);
+ target = gen_rtx_REG (CCmode, CC_REGNUM);
+
+ create_output_operand (ops[0], target, CCmode);
+ create_fixed_operand (ops[1], cmp); create_fixed_operand (ops[2],
+ op0); create_fixed_operand (ops[3], op1);
Hmm. With so many fixed operands, I think you may be better off not
creating the cmpmode expander in the first place. Just inline the
SELECT_CC_MODE and everything right here.
In the patch, I use prepare_operand (icode, op0, 2, ...) to do the operand MODE
conversion (from HI/QI to SI), which needs a cmpmode expander. Without it, I
have to add additional codes to do the conversion (as it in previous patch, which
leads to PR64015).
Ping~,
verified this patch will pass speck2k6 build without ICE anymore.
Thanks!
-Zhenqiang
RE: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
-Original Message-
From: Richard Henderson [mailto:r...@redhat.com]
Sent: Saturday, December 13, 2014 3:26 AM
To: Zhenqiang Chen
Cc: Marcus Shawcroft; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
- tree lhs = gimple_assign_lhs (g);
enum machine_mode mode = TYPE_MODE (TREE_TYPE (lhs));
rtx target = gen_reg_rtx (mode);
+
+ start_sequence ();
tmp = emit_cstore (target, icode, NE, cc_mode, cc_mode,
0, tmp, const0_rtx, 1, mode);
if (tmp)
- return tmp;
+ {
+ rtx seq = get_insns ();
+ end_sequence ();
+ emit_insn (prep_seq);
+ emit_insn (gen_seq);
+ emit_insn (seq);
+ return tmp;
+ }
+ end_sequence ();
Given that you're already doing delete_insns_since (last) at the end of this
function, I don't think you need a new sequence around the emit_cstore.
Just
emit_insn (prep_seq);
emit_insn (gen_seq);
tmp = emit_cstore (...);
if (tmp)
return tmp;
Updated.
+ int unsignedp = code == LTU || code == LEU || code == GTU || code
+ == GEU;
You don't need to examine the code, you can look at the argument:
TYPE_UNSIGNED (TREE_TYPE (treeop0))
Updated.
+ op0 = prepare_operand (icode, op0, 2, op_mode, cmp_mode,
+ unsignedp);
+ op1 = prepare_operand (icode, op1, 3, op_mode, cmp_mode,
+ unsignedp); if (!op0 || !op1)
+{
+ end_sequence ();
+ return NULL_RTX;
+}
+ *prep_seq = get_insns ();
+ end_sequence ();
+
+ cmp = gen_rtx_fmt_ee ((enum rtx_code) code, cmp_mode, op0, op1);
+ target = gen_rtx_REG (CCmode, CC_REGNUM);
+
+ create_output_operand (ops[0], target, CCmode);
+ create_fixed_operand (ops[1], cmp); create_fixed_operand (ops[2],
+ op0); create_fixed_operand (ops[3], op1);
Hmm. With so many fixed operands, I think you may be better off not
creating the cmpmode expander in the first place. Just inline the
SELECT_CC_MODE and everything right here.
In the patch, I use prepare_operand (icode, op0, 2, ...) to do the operand MODE
conversion (from HI/QI to SI), which needs a cmpmode expander. Without it, I
have to add additional codes to do the conversion (as it in previous patch,
which leads to PR64015).
Thanks!
-Zhenqiang
gen-ccmp-v2.patch
Description: Binary data
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
- tree lhs = gimple_assign_lhs (g);
enum machine_mode mode = TYPE_MODE (TREE_TYPE (lhs));
rtx target = gen_reg_rtx (mode);
+
+ start_sequence ();
tmp = emit_cstore (target, icode, NE, cc_mode, cc_mode,
0, tmp, const0_rtx, 1, mode);
if (tmp)
- return tmp;
+ {
+ rtx seq = get_insns ();
+ end_sequence ();
+ emit_insn (prep_seq);
+ emit_insn (gen_seq);
+ emit_insn (seq);
+ return tmp;
+ }
+ end_sequence ();
Given that you're already doing delete_insns_since (last)
at the end of this function, I don't think you need a new
sequence around the emit_cstore. Just
emit_insn (prep_seq);
emit_insn (gen_seq);
tmp = emit_cstore (...);
if (tmp)
return tmp;
+ int unsignedp = code == LTU || code == LEU || code == GTU || code == GEU;
You don't need to examine the code, you can look at the argument:
TYPE_UNSIGNED (TREE_TYPE (treeop0))
+ op0 = prepare_operand (icode, op0, 2, op_mode, cmp_mode, unsignedp);
+ op1 = prepare_operand (icode, op1, 3, op_mode, cmp_mode, unsignedp);
+ if (!op0 || !op1)
+{
+ end_sequence ();
+ return NULL_RTX;
+}
+ *prep_seq = get_insns ();
+ end_sequence ();
+
+ cmp = gen_rtx_fmt_ee ((enum rtx_code) code, cmp_mode, op0, op1);
+ target = gen_rtx_REG (CCmode, CC_REGNUM);
+
+ create_output_operand (ops[0], target, CCmode);
+ create_fixed_operand (ops[1], cmp);
+ create_fixed_operand (ops[2], op0);
+ create_fixed_operand (ops[3], op1);
Hmm. With so many fixed operands, I think you may be better off not
creating the cmpmode expander in the first place. Just inline the
SELECT_CC_MODE and everything right here.
r~
RE: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
-Original Message-
From: Richard Henderson [mailto:r...@redhat.com]
Sent: Tuesday, November 25, 2014 5:25 PM
To: Zhenqiang Chen
Cc: Marcus Shawcroft; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 11/25/2014 09:41 AM, Zhenqiang Chen wrote:
I want to confirm with you two things before I rework it.
(1) expand_insn needs an optab_handler as input. Do I need to define a
ccmp_optab with different mode support in optabs.def?
No, look again: expand_insn needs an enum insn_code as input. Since this is
the backend, you can use any icode name you like, which means that you can
use CODE_FOR_ccmp_and etc directly.
(2) To make sure later operands not clobber CC, all operands are expanded
before ccmp-first in current implementation. If taking tree/gimple as input,
what's your preferred logic to guarantee CC not clobbered?
Hmm. Perhaps the target hook will need to output two sequences, each of
which will be concatenated while looping around the calls to gen_ccmp_next.
The first sequence will be operand preparation and the second sequence will
be ccmp generation.
Something like
bool
aarch64_gen_ccmp_start(rtx *prep_seq, rtx *gen_seq,
int cmp_code, int bit_code,
tree op0, tree op1) {
bool success;
start_sequence ();
// Widen and expand operands
*prep_seq = get_insns ();
end_sequence ();
start_sequence ();
// Generate the first compare
*gen_seq = get_insns ();
end_sequence ();
return success;
}
bool
aarch64_gen_ccmp_next(rtx *prep_seq, rtx *gen_seq,
rtx prev, int cmp_code, int bit_code,
tree op0, tree op1) {
bool success;
push_to_sequence (*prep_seq);
// Widen and expand operands
*prep_seq = get_insns ();
end_sequence ();
push_to_sequence (*gen_seq);
// Generate the next ccmp
*gen_seq = get_insns ();
end_sequence ();
return success;
}
If there are ever any failures, the middle-end can simply discard the
sequences. If everything succeeds, it simply calls emit_insn on both
sequences.
Thanks for the comments. The updated patch is attached.
Note: Function aarch64_code_to_ccmode is the same as it before reverting.
ChangeLog:
2014-12-12 Zhenqiang Chen zhenqiang.c...@arm.com
* ccmp.c (expand_ccmp_next): New function.
(expand_ccmp_expr_1, expand_ccmp_expr): Handle operand insn sequence
and compare insn sequence.
* config/aarch64/aarch64.c (aarch64_code_to_ccmode,
aarch64_gen_ccmp_first, aarch64_gen_ccmp_next): New functions.
(TARGET_GEN_CCMP_FIRST, TARGET_GEN_CCMP_NEXT): New MICRO.
* config/aarch64/aarch64.md (*ccmp_and): Changed to ccmp_andmode.
(*ccmp_ior): Changed to ccmp_iormode.
(cmpmode): New pattern.
* doc/tm.texi (TARGET_GEN_CCMP_FIRST, TARGET_GEN_CCMP_NEXT): Update
parameters.
* target.def (gen_ccmp_first, gen_ccmp_next): Update parameters.
testsuite/ChangeLog:
2014-12-12 Zhenqiang Chen zhenqiang.c...@arm.com
* gcc.dg/pr64015.c: New test.
gen-ccmp.patch
Description: Binary data
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 11/26/2014 10:23 AM, Zhenqiang Chen wrote: /* Check to make sure CC is not clobbered since prepare_operand might generates copy or mode convertion insns, although no test shows such insns clobber CC. */ And what do you do if a clobber does happen? With TER enabled, there's every possibility that it might be. Does such change align with your comments? Not really, no. r~
RE: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
-Original Message-
From: Richard Henderson [mailto:r...@redhat.com]
Sent: Tuesday, November 25, 2014 5:25 PM
To: Zhenqiang Chen
Cc: Marcus Shawcroft; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 11/25/2014 09:41 AM, Zhenqiang Chen wrote:
I want to confirm with you two things before I rework it.
(1) expand_insn needs an optab_handler as input. Do I need to define a
ccmp_optab with different mode support in optabs.def?
No, look again: expand_insn needs an enum insn_code as input. Since this is
the backend, you can use any icode name you like, which means that you can
use CODE_FOR_ccmp_and etc directly.
(2) To make sure later operands not clobber CC, all operands are expanded
before ccmp-first in current implementation. If taking tree/gimple as input,
what's your preferred logic to guarantee CC not clobbered?
Hmm. Perhaps the target hook will need to output two sequences, each of
which will be concatenated while looping around the calls to gen_ccmp_next.
The first sequence will be operand preparation and the second sequence will
be ccmp generation.
Something like
bool
aarch64_gen_ccmp_start(rtx *prep_seq, rtx *gen_seq,
int cmp_code, int bit_code,
tree op0, tree op1) {
bool success;
start_sequence ();
// Widen and expand operands
*prep_seq = get_insns ();
end_sequence ();
start_sequence ();
// Generate the first compare
*gen_seq = get_insns ();
end_sequence ();
return success;
}
bool
aarch64_gen_ccmp_next(rtx *prep_seq, rtx *gen_seq,
rtx prev, int cmp_code, int bit_code,
tree op0, tree op1) {
bool success;
push_to_sequence (*prep_seq);
// Widen and expand operands
*prep_seq = get_insns ();
end_sequence ();
push_to_sequence (*gen_seq);
// Generate the next ccmp
*gen_seq = get_insns ();
end_sequence ();
return success;
}
If there are ever any failures, the middle-end can simply discard the
sequences. If everything succeeds, it simply calls emit_insn on both
sequences.
When there are more than one ccmps, it will be complexity to maintain the
un-emitted sequences in a recursive algorithm. E.g.
CC0 = CMP (a, b);
CC1 = CCMP (NE (CC0, 0), CMP (e, f));
...
CCn = CCMP (NE (CCn-1, 0), CMP (...));
I read the codes to expand cstoresi and cbranchsi. It just uses normal
expand_operands. So I think we can keep expand_operands in middle-end.
For the start one, we do not need worry about it since its last step should
compare to set CC. And it is easy to delete the insns when fail.
static rtx
aarch64_gen_ccmp_first (int code, rtx op0, rtx op1)
{
... // init the vars
op0 = prepare_operand (icode, op0, 2, op_mode, cmp_mode, unsignedp);
op1 = prepare_operand (icode, op1, 3, op_mode, cmp_mode, unsignedp);
if (!op0 || !op1)
return NULL_RTX;
cmp = gen_rtx_fmt_ee ((enum rtx_code) code, cmp_mode, op0, op1);
target = gen_rtx_REG (CCmode, CC_REGNUM);
create_output_operand (ops[0], target, CCmode);
create_fixed_operand (ops[1], cmp);
create_fixed_operand (ops[2], op0);
create_fixed_operand (ops[3], op1);
if (!maybe_expand_insn (icode, 4, ops))
return NULL_RTX;
return gen_rtx_REG (cc_mode, CC_REGNUM);
}
aarch64_gen_ccmp_next (rtx prev, int cmp_code, rtx op0, rtx op1, int bit_code)
{
... // init the vars
op0 = prepare_operand (icode, op0, 2, op_mode, cmp_mode, unsignedp);
op1 = prepare_operand (icode, op1, 3, op_mode, cmp_mode, unsignedp);
if (!op0 || !op1)
return NULL_RTX;
/* Check to make sure CC is not clobbered since prepare_operand might
generates copy or mode convertion insns, although no test shows
such insns clobber CC. */
...
cmp1 = gen_rtx_fmt_ee ((enum rtx_code) cmp_code, cmp_mode, op0, op1);
cmp0 = gen_rtx_fmt_ee (NE, cmp_mode, prev, const0_rtx);
target = gen_rtx_REG (cc_mode, CC_REGNUM);
create_fixed_operand (ops[0], prev);
create_fixed_operand (ops[1], target);
create_fixed_operand (ops[2], op0);
create_fixed_operand (ops[3], op1);
create_fixed_operand (ops[4], cmp0);
create_fixed_operand (ops[5], cmp1);
if (!maybe_expand_insn (icode, 6, ops))
return NULL_RTX;
return target;
}
Does such change align with your comments?
Thanks!
-Zhenqiang
RE: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
-Original Message- From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- ow...@gcc.gnu.org] On Behalf Of Richard Henderson Sent: Monday, November 24, 2014 4:57 PM To: Zhenqiang Chen; gcc-patches@gcc.gnu.org Cc: Marcus Shawcroft Subject: Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015) On 11/24/2014 06:11 AM, Zhenqiang Chen wrote: Expand pass always uses sign-extend to represent constant value. For the case in the patch, a 8-bit unsigned value 252 is represented as -4, which pass the ccmn check. After mode conversion, -4 becomes 252, which leads to mismatch. This sort of thing is why I suggested from the beginning that expansion happen directly from trees instead of sort-of re-expanding from rtl. I think you're better off fixing this properly than hacking around it here. Thanks for the comments. Here was your previous comments: We could avoid that by using struct expand_operand, create_input_operand et al, then expand_insn. That does require that the target hooks be given trees rather than rtl as input. I want to confirm with you two things before I rework it. (1) expand_insn needs an optab_handler as input. Do I need to define a ccmp_optab with different mode support in optabs.def? (2) To make sure later operands not clobber CC, all operands are expanded before ccmp-first in current implementation. If taking tree/gimple as input, what's your preferred logic to guarantee CC not clobbered? Thanks! -Zhenqiang
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 11/25/2014 09:41 AM, Zhenqiang Chen wrote:
I want to confirm with you two things before I rework it.
(1) expand_insn needs an optab_handler as input. Do I need to define a
ccmp_optab with different mode support in optabs.def?
No, look again: expand_insn needs an enum insn_code as input. Since this is
the backend, you can use any icode name you like, which means that you can use
CODE_FOR_ccmp_and etc directly.
(2) To make sure later operands not clobber CC, all operands are expanded
before ccmp-first in current implementation. If taking tree/gimple as input,
what's your preferred logic to guarantee CC not clobbered?
Hmm. Perhaps the target hook will need to output two sequences, each of which
will be concatenated while looping around the calls to gen_ccmp_next. The
first sequence will be operand preparation and the second sequence will be ccmp
generation.
Something like
bool
aarch64_gen_ccmp_start(rtx *prep_seq, rtx *gen_seq,
int cmp_code, int bit_code,
tree op0, tree op1)
{
bool success;
start_sequence ();
// Widen and expand operands
*prep_seq = get_insns ();
end_sequence ();
start_sequence ();
// Generate the first compare
*gen_seq = get_insns ();
end_sequence ();
return success;
}
bool
aarch64_gen_ccmp_next(rtx *prep_seq, rtx *gen_seq,
rtx prev, int cmp_code, int bit_code,
tree op0, tree op1)
{
bool success;
push_to_sequence (*prep_seq);
// Widen and expand operands
*prep_seq = get_insns ();
end_sequence ();
push_to_sequence (*gen_seq);
// Generate the next ccmp
*gen_seq = get_insns ();
end_sequence ();
return success;
}
If there are ever any failures, the middle-end can simply discard the
sequences. If everything succeeds, it simply calls emit_insn on both sequences.
r~
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On 11/24/2014 06:11 AM, Zhenqiang Chen wrote: Expand pass always uses sign-extend to represent constant value. For the case in the patch, a 8-bit unsigned value 252 is represented as -4, which pass the ccmn check. After mode conversion, -4 becomes 252, which leads to mismatch. This sort of thing is why I suggested from the beginning that expansion happen directly from trees instead of sort-of re-expanding from rtl. I think you're better off fixing this properly than hacking around it here. r~
Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
On Sun, Nov 23, 2014 at 9:11 PM, Zhenqiang Chen zhenqiang.c...@arm.com wrote:
Hi,
Expand pass always uses sign-extend to represent constant value. For the
case in the patch, a 8-bit unsigned value 252 is represented as -4,
which pass the ccmn check. After mode conversion, -4 becomes 252, which
leads to mismatch.
The patch adds another operand check after mode conversion.
No make check regression with qemu.
OK for trunk?
Thanks!
-Zhenqiang
ChangeLog:
2014-11-24 Zhenqiang Chen zhenqiang.c...@arm.com
PR target/64015
* config/aarch64/aarch64.c (aarch64_gen_ccmp_first): Recheck operand
after mode conversion.
(aarch64_gen_ccmp_next): Likewise.
testsuite/ChangeLog:
2014-11-24 Zhenqiang Chen zhenqiang.c...@arm.com
* gcc.target/aarch64/pr64015.c: New test.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 1809513..203d095 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10311,7 +10311,10 @@ aarch64_gen_ccmp_first (int code, rtx op0, rtx op1)
if (!aarch64_plus_operand (op1, GET_MODE (op1)))
op1 = force_reg (mode, op1);
- if (!aarch64_convert_mode (op0, op1, unsignedp))
+ if (!aarch64_convert_mode (op0, op1, unsignedp)
+/* Some negative value might be transformed into a positive one.
+ So need recheck here. */
+ || !aarch64_plus_operand (op1, GET_MODE (op1)))
return NULL_RTX;
Shouldn't we force it to a reg here instead?
mode = aarch64_code_to_ccmode ((enum rtx_code) code);
@@ -10344,7 +10347,10 @@ aarch64_gen_ccmp_next (rtx prev, int cmp_code, rtx
op0, rtx op1, int bit_code)
|| !aarch64_ccmp_operand (op1, GET_MODE (op1)))
return NULL_RTX;
- if (!aarch64_convert_mode (op0, op1, unsignedp))
+ if (!aarch64_convert_mode (op0, op1, unsignedp)
+/* Some negative value might be transformed into a positive one.
+ So need recheck here. */
+ || !aarch64_ccmp_operand (op1, GET_MODE (op1)))
return NULL_RTX;
Also really the cost of forcing to a register is better really.
In the cases where we would not have forced to a register in a cmp
instruction, the constant would be one instruction and compared to the
cost of two cset and an and/or is better.
In the cases where we would have forced to a register for the cmp
instruction, two cost for doing the forcing is the same on both cases
but since we gaining from removing a cset and an and/or we are better.
mode = aarch64_code_to_ccmode ((enum rtx_code) cmp_code);
diff --git a/gcc/testsuite/gcc.target/aarch64/pr64015.c
b/gcc/testsuite/gcc.target/aarch64/pr64015.c
new file mode 100644
index 000..eeed665
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr64015.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options -O2 } */
+int
+test (unsigned short a, unsigned char b)
+{
+ return a 0xfff2 b 252;
+}
Since this testcase is generic (except for the -O2), it really should
go into gcc.c-torture/compile instead of remove the two dg-*
directives so it can be tested on more than AARCH64 and on more
optimization levels.
Thanks,
Andrew Pinski
