Now looking at the patch past the first hunk...
> +(define_insn "*iorbset_reg"
> + [(set (match_operand:SI 0 "register_operand" "+r")
> + (ior:SI (match_dup 0)
> + (match_operand 1 "const_int_operand" "Intso")))]
> + "satisfies_constraint_Intso (operands[1])"
> + "bset\\t%D1,%0"
> + [(set_attr "length" "3")]
> +)
While this and the new *xorbnot_reg and *andbclr_reg patterns are
correct, I question whether you really want to add them.
I think they should be folded in as alternatives in the parent
ior/xor/and patterns. Otherwise reload will not make the correct
choices. Note that they cannot be added to the version of the
ior/xor/and patterns that use the flags register.
> +(define_insn "*bset"
> + [(set (zero_extract:SI
> + (match_operand:QI 0 "rx_restricted_mem_operand" "+Q")
> + (const_int 1)
> + (match_operand 1 "rx_constbit_operand" "Uint03"))
> + (const_int 1))]
> + ""
> + "bset\\t%1,%0.B"
> + [(set_attr "length" "3")]
> +)
This and the *bclr pattern that you add seem to be redundant
with the existing *insv_imm pattern. Why did you add them?
> +(define_insn "*insv_mem_imm"
> + [(set (zero_extract:SI
> + (match_operand 0 "rx_restricted_mem_operand" "+Q")
> + (const_int 1)
> + (match_operand 1 "nonmemory_operand" "ri"))
> + (match_dup 0))]
> + ""
> +{
> + if (INTVAL (operands[2]) & 1)
> + return "bset\t%1, %0.B";
> + else
> + return "bclr\t%1, %0.B";
> +}
> + [(set_attr "length" "3")]
Huh? You reference operand 2, which does not exist.
And a similar comment applies: {bitclr_in_memory,bitset_in_memory}
are redundant with this insv_mem_imm pattern.
> /* We only handle single-bit inserts. */
> if (!CONST_INT_P (operands[1]) || INTVAL (operands[1]) != 1)
> FAIL;
>
> + if (GET_CODE (operands [0]) == MEM
> + && (!CONST_INT_P (operands[2])
> + || !CONST_INT_P (operands[3])
> + || (INTVAL (operands[2]) < 0 || INTVAL (operands[2]) > 7)))
> + FAIL;
> +
> + if (GET_CODE (operands [0]) == MEM
> + && satisfies_constraint_Intu1 (operands[1])
This check is redundant with the first test quoted above.
> + && satisfies_constraint_Uint03 (operands[2]))
> + {
> + if (satisfies_constraint_Intu0 (operands[3]))
Rather than referencing satisfies_constraint_* here, it would be
better to follow the form of the rest of the function. Code could
be shared then.
There is a problem here, however. The argument to the insv is an
SImode operand. The instruction takes a QImode operand. You
cannot just pretend the two are the same; you need to apply some
transform. The code as written is wrong for -mbig-endian-data.
Now, suppose you do transform SImode to QImode. Then I will question
whether this is efficient. Once you force the value to remain in
memory, and in a mode for which you have no arithmetic operations,
you cripple the rtl optimizers, and in particular, combine.
Have you tested this change with things other than microbenchmarks?
My suggestion for insv, and these sorts of patterns, is to merge them
so that you allow SImode values in registers or memory until after
reload is complete. And only then split the pattern, applying the
transform on the memory operand to create a QImode reference.
It's quite a bit more work than what you have here, which is why I
didn't do it myself when I rearranged all of this code in 2011.
r~
