Re: [Patch 1/3] ARM 64 bit atomic operations

[David Gilbert <david.gilbert@linaro.org>]

On 12 July 2011 22:07, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
> Hi Dave,

Hi Ramana,
  Thanks for the review.

> Could you split this further into a patch that deals with the
> case for disabling MCR memory barriers for Thumb1 so that it
> maybe backported to the release branches ? I have commented inline
> as well.

Sure.

> Could you also provide a proper changelog entry for this that will
> also help with review of the patch ?

Yep, no problem.

> I've not yet managed to fully review all the bits in this patch but
> here's some initial comments that should be looked at.
>
> On 1 July 2011 16:54, Dr. David Alan Gilbert <david.gilbert@linaro.org> wrote:
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
<snip>

>> +      if (is_di)
>> +        {
>> +          arm_output_asm_insn (emit, 0, operands, "it\teq");
>
> This should be guarded with a if (TARGET_THUMB2) - there's no point in
> accounting for the length of this instruction in the compiler and then
> have the assembler fold it away in ARM state.

OK; the length accounting seems pretty broken anyway; I think it assumes
all instructions are 4 bytes.

>> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
>> index c32ef1a..3fdd22f 100644
>> --- a/gcc/config/arm/arm.h
>> +++ b/gcc/config/arm/arm.h
>> @@ -282,7 +282,8 @@ extern void (*arm_lang_output_object_attributes_hook)(void);
>> -#define TARGET_HAVE_DMB_MCR    (arm_arch6k && ! TARGET_HAVE_DMB)
>> +#define TARGET_HAVE_DMB_MCR    (arm_arch6k && ! TARGET_HAVE_DMB \
>> +                                && ! TARGET_THUMB1)
>
> This hunk (TARGET_HAVE_DMB_MCR) should probably be backported to
> release branches because this is technically fixing an issue and
> hence should be a separate patch that can be looked at separately.

OK, will do.

>>  /* Nonzero if this chip implements a memory barrier instruction.  */
>>  #define TARGET_HAVE_MEMORY_BARRIER (TARGET_HAVE_DMB || TARGET_HAVE_DMB_MCR)
>> @@ -290,8 +291,12 @@ extern void (*arm_lang_output_object_attributes_hook)(void);
>
> sync.md changes -
>
>> (define_mode_iterator NARROW [QI HI])
>>+(define_mode_iterator QHSD [QI HI SI DI])
>>+(define_mode_iterator SIDI [SI DI])
>>+
>>+(define_mode_attr sync_predtab [(SI "TARGET_HAVE_LDREX && TARGET_HAVE_MEMORY_BARRIER")
>>+                              (QI "TARGET_HAVE_LDREXBH && TARGET_HAVE_MEMORY_BARRIER")
>>+                              (HI "TARGET_HAVE_LDREXBH && TARGET_HAVE_MEMORY_BARRIER")
>>+                              (DI "TARGET_HAVE_LDREXD && ARM_DOUBLEWORD_ALIGN && TARGET_HAVE_MEMORY_BARRIER")])
>>+
>
> Can we move all the iterators to iterators.md and then arrange
> includes to work automatically ? Minor nit - could you align the entries
> for QI, HI and DI with the start of the SI ?

Yes I can do - the only odd thing is I guess the sync_predtab is very
sync.md specific, does it really make sense for that
to be in iterators.md ?

>>+(define_mode_attr sync_atleastsi [(SI "SI")
>>+                                (DI "DI")
>>+                                (HI "SI")
>>+                                (QI "SI")])
>>
>
> I couldn't spot where this was being used. Can this be removed if not
> necessary ?

Ah - yes I think that's dead; it's a relic from an attempt to merge some of the
other narrow cases into the same iterator but it got way too messy.

>>-(define_insn "arm_sync_new_nandsi"
>>+(define_insn "arm_sync_new_<sync_optab><mode>"
>>   [(set (match_operand:SI 0 "s_register_operand" "=&r")
>>-        (unspec_volatile:SI [(not:SI (and:SI
>>-                               (match_operand:SI 1 "arm_sync_memory_operand" "+Q")
>>-                               (match_operand:SI 2 "s_register_operand" "r")))
>>-                          ]
>>-                          VUNSPEC_SYNC_NEW_OP))
>>+        (unspec_volatile:SI [(syncop:SI
>>+                             (zero_extend:SI
>>+                               (match_operand:NARROW 1 "arm_sync_memory_operand" "+Q"))
>>+                             (match_operand:SI 2 "s_register_operand" "r"))
>>+                          ]
>>+                          VUNSPEC_SYNC_NEW_OP))
>>    (set (match_dup 1)
>>-        (unspec_volatile:SI [(match_dup 1) (match_dup 2)]
>>-                          VUNSPEC_SYNC_NEW_OP))
>>+      (unspec_volatile:NARROW [(match_dup 1) (match_dup 2)]
>>+                              VUNSPEC_SYNC_NEW_OP))
>>    (clobber (reg:CC CC_REGNUM))
>>    (clobber (match_scratch:SI 3 "=&r"))]
>>-  "TARGET_HAVE_LDREX && TARGET_HAVE_MEMORY_BARRIER"
>>+  "TARGET_HAVE_LDREXBH && TARGET_HAVE_MEMORY_BARRIER"
>
> Can't this just use <sync_predtab> instead since the condition is identical
> for QImode and HImode from that mode attribute and in quite a few
> places below. ?

Hmm yes it can - I'd only been using predtab in the places where it was
varying on the mode; but as you say this can be converted as well.

>>@@ -461,19 +359,19 @@
>>         (unspec_volatile:SI
>>         [(not:SI
>>            (and:SI
>>-               (zero_extend:SI
>>-               (match_operand:NARROW 1 "arm_sync_memory_operand" "+Q"))
>>-               (match_operand:SI 2 "s_register_operand" "r")))
>>+             (zero_extend:SI
>>+               (match_operand:NARROW 1 "arm_sync_memory_operand" "+Q"))
>>+             (match_operand:SI 2 "s_register_operand" "r")))
>>         ] VUNSPEC_SYNC_NEW_OP))
>>    (set (match_dup 1)
>>         (unspec_volatile:NARROW [(match_dup 1) (match_dup 2)]
>>-                              VUNSPEC_SYNC_NEW_OP))
>>+                              VUNSPEC_SYNC_NEW_OP))
>>    (clobber (reg:CC CC_REGNUM))
>>    (clobber (match_scratch:SI 3 "=&r"))]
>>-  "TARGET_HAVE_LDREX && TARGET_HAVE_MEMORY_BARRIER"
>>+  "TARGET_HAVE_LDREXBH && TARGET_HAVE_MEMORY_BARRIER"
>
> Again here . Not sure which pattern this is though just looking at the patch.

Sure.

Thanks for reviewing it.

Dave