This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][AARCH64]combine "ubfiz" and "orr" with bfi when certain condition meets.
- From: Richard Henderson <rth at redhat dot com>
- To: Renlin Li <renlin dot li at arm dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Cc: Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>
- Date: Tue, 25 Feb 2014 09:04:09 -0800
- Subject: Re: [PATCH][AARCH64]combine "ubfiz" and "orr" with bfi when certain condition meets.
- Authentication-results: sourceware.org; auth=none
- References: <530CBD1B dot 30704 at arm dot com>
On 02/25/2014 07:56 AM, Renlin Li wrote:
> +(define_insn_and_split "*combine_bfi3<mode>"
> + [(set (match_operand:GPI 0 "register_operand" "=r")
> + (ior:GPI (and:GPI (match_operand:GPI 1 "register_operand" "0")
> + (match_operand 2 "const_int_operand" "n"))
> + (and:GPI (ashift:GPI (match_operand:GPI 3 "register_operand" "r")
> + (match_operand 4 "const_int_operand" "n"))
> + (match_operand 5 "const_int_operand" "n"))))]
> + "exact_log2 ((INTVAL (operands[5]) >> INTVAL (operands[4])) + 1) >= 0
> + && (INTVAL (operands[5]) & ((1 << INTVAL (operands[4])) - 1)) == 0
> + && (INTVAL (operands[2]) & INTVAL (operands[5])) == 0"
> + "#"
> + ""
> + [(set (match_dup 0)
> + (and:GPI (match_dup 1) (match_dup 6)))
> + (set (zero_extract:GPI (match_dup 0 )
> + (match_dup 5 )
> + (match_dup 4 ))
> + (match_dup 3 ))]
> + "{
Don't use quotes and braces. Just use braces.
Watch the extra space before close parenthesis, all over the place.
> + int tmp = (INTVAL (operands[5]) >> INTVAL (operands[4])) + 1;
> + operands[5] = GEN_INT (exact_log2 (tmp));
> +
> + enum machine_mode mode = GET_MODE (operands[0]);
You know from the pattern that "GET_MODE (operands[0])" is "<MODE>mode", a
compile-time constant.
> + operands[6] = can_create_pseudo_p () ? gen_reg_rtx (mode) : operands[0];
> + if (!aarch64_bitmask_imm (INTVAL (operands[2]), mode))
> + emit_move_insn (operands[6], operands[2]);
> + else
> + operands[6] = operands[2];
When aarch64_bitmask_imm is true, you're creating a pseudo that you don't use.
I don't see how operands[0] can be unconditionally overwritten. Why couldn't
it overlap with operands[1] or operands[3]?
Positive tests are easier to follow than negative tests, given a choice.
I'm thinking this should be more like
if (aarch64_bitmask_imm (INTVAL (operands[2]), mode))
operands[6] = operands[2];
else if (can_create_pseudo_p ())
{
operands[6] = gen_reg_rtx (mode);
emit_move_insn (operands[6], operands[2]);
}
else
FAIL;
Alternately, you could decline to handle non-aarch64_bitmask_imm constants by
using aarch64_logical_immediate as the predicate for operands[2]. Which would
make all this code go away.
> + }"
> + [(set_attr "type" "bfm")]
> +)
Surely "multiple" is better for a force-split insn.
r~