On 12 July 2011 22:07, Ramana Radhakrishnan
<ramana.radhakrish...@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.gilb...@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
