This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch] MIPS: Improve HI and QI atomic memory ops.
David Daney <ddaney@avtrex.com> writes:
> Richard Sandiford wrote:
>> I suppose we could avoid the mask for positive constants, but at this
>> stage, I think it's probably best to drop the special cases altogether.
>> We should be able to optimize away the constant ops later.
>
> I agree that we *should* be able to optimize away the constant ops, but
> given the current state of the compiler, we do not.
>
> We can emit smaller code if we make these optimizations when expanding
> the atomic operations. It seems you are recommending improving the RTL
> optimizers so that the code is folded away if possible.
OK. We're making things harder for gcc by doing the shift first,
and then ANDing with the shifted mask. It can optimise things
properly if we do the mask first, in the form of a zero extension.
I think that makes more sense anyway: both QImode and HImode
zero extensions are single ANDIs, so we might as well avoid the
register dependency (and follow-on restrictions on scheduling).
Using a zero_extend will also allow the compiler to optimise
cases where it can prove a variable is already masked, and to
use lhu and lbu when fetching the old and new values from memory.
>> (I wondered about saying that before, for the const0_rtx case.
>> But that was such a simple shortcut that I didn't mind much
>> either way. Now that we have other potential shortcuts,
>> it seems more consistent to take none.)
>>
>> So the patch is OK if you simply add:
>>
>> mips_emit_binary (AND, old_shifted, old_shifted, mask);
>> and:
>> mips_emit_binary (AND, new_shifted, new_shifted, mask);
>>
>> to the existing code, without any of the other mips.c stuff.
>
> The changes to mips.{md,h} are unused/unreachable without the const0_rtx
> test.
>
> It seems that if you only want the masking part required for correct
> code generation, I should abandon the rest of the patch.
After the above change, we still use the optimised loop for constants,
even without the special const0_rtx guards. However, I agree it makes
sense to keep them, given that the define_insn accepts them.
Sorry for the noise.
What do you think of the patch below, in combination with your other
changes?
Richard
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c 2008-05-05 22:25:11.000000000 +0100
+++ gcc/config/mips/mips.c 2008-05-07 08:58:46.000000000 +0100
@@ -5880,7 +5880,10 @@ mips_expand_synci_loop (rtx begin, rtx e
mips_expand_compare_and_swap_12 (rtx result, rtx mem, rtx oldval, rtx newval)
{
rtx orig_addr, memsi_addr, memsi, shift, shiftsi, unshifted_mask;
- rtx mask, inverted_mask, oldvalsi, old_shifted, newvalsi, new_shifted, res;
+ rtx unshifted_mask_reg, mask, inverted_mask, res;
+ enum machine_mode mode;
+
+ mode = GET_MODE (mem);
/* Compute the address of the containing SImode value. */
orig_addr = force_reg (Pmode, XEXP (mem, 0));
@@ -5896,8 +5899,7 @@ mips_expand_compare_and_swap_12 (rtx res
counting from the least significant byte. */
shift = mips_force_binary (Pmode, AND, orig_addr, GEN_INT (3));
if (TARGET_BIG_ENDIAN)
- mips_emit_binary (XOR, shift, shift,
- GEN_INT (GET_MODE (mem) == QImode ? 3 : 2));
+ mips_emit_binary (XOR, shift, shift, GEN_INT (mode == QImode ? 3 : 2));
/* Multiply by eight to convert the shift value from bytes to bits. */
mips_emit_binary (ASHIFT, shift, shift, GEN_INT (3));
@@ -5907,9 +5909,9 @@ mips_expand_compare_and_swap_12 (rtx res
shiftsi = force_reg (SImode, gen_lowpart (SImode, shift));
/* Set MASK to an inclusive mask of the QImode or HImode value. */
- unshifted_mask = GEN_INT (GET_MODE_MASK (GET_MODE (mem)));
- unshifted_mask = force_reg (SImode, unshifted_mask);
- mask = mips_force_binary (SImode, ASHIFT, unshifted_mask, shiftsi);
+ unshifted_mask = GEN_INT (GET_MODE_MASK (mode));
+ unshifted_mask_reg = force_reg (SImode, unshifted_mask);
+ mask = mips_force_binary (SImode, ASHIFT, unshifted_mask_reg, shiftsi);
/* Compute the equivalent exclusive mask. */
inverted_mask = gen_reg_rtx (SImode);
@@ -5917,12 +5919,20 @@ mips_expand_compare_and_swap_12 (rtx res
gen_rtx_NOT (SImode, mask)));
/* Shift the old value into place. */
- oldvalsi = force_reg (SImode, gen_lowpart (SImode, oldval));
- old_shifted = mips_force_binary (SImode, ASHIFT, oldvalsi, shiftsi);
+ if (oldval != const0_rtx)
+ {
+ oldval = convert_modes (SImode, mode, oldval, true);
+ oldval = force_reg (SImode, oldval);
+ oldval = mips_force_binary (SImode, ASHIFT, oldval, shiftsi);
+ }
/* Do the same for the new value. */
- newvalsi = force_reg (SImode, gen_lowpart (SImode, newval));
- new_shifted = mips_force_binary (SImode, ASHIFT, newvalsi, shiftsi);
+ if (newval != const0_rtx)
+ {
+ newval = convert_modes (SImode, mode, newval, true);
+ newval = force_reg (SImode, newval);
+ newval = mips_force_binary (SImode, ASHIFT, newval, shiftsi);
+ }
/* Do the SImode atomic access. */
res = gen_reg_rtx (SImode);