Re: [GCC, ARM] armv8 linux toolchain asan testcase fail due to stl missing conditional code

[Kyrill Tkachov <kyrylo.tkachov@arm.com>]

On 30/09/15 17:39, Kyrill Tkachov wrote:
> On 09/06/15 09:17, Kyrill Tkachov wrote:
>> On 05/06/15 14:14, Kyrill Tkachov wrote:
>>> On 05/06/15 14:11, Richard Earnshaw wrote:
>>>> On 05/06/15 14:08, Kyrill Tkachov wrote:
>>>>> Hi Shiva,
>>>>>
>>>>> On 05/06/15 10:42, Shiva Chen wrote:
>>>>>> Hi, Kyrill
>>>>>>
>>>>>> I add the testcase as stl-cond.c.
>>>>>>
>>>>>> Could you help to check the testcase ?
>>>>>>
>>>>>> If it's OK, Could you help me to apply the patch ?
>>>>>>
>>>>> This looks ok to me.
>>>>> One nit on the testcase:
>>>>>
>>>>> diff --git a/gcc/testsuite/gcc.target/arm/stl-cond.c
>>>>> b/gcc/testsuite/gcc.target/arm/stl-cond.c
>>>>> new file mode 100755
>>>>> index 0000000..44c6249
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.target/arm/stl-cond.c
>>>>> @@ -0,0 +1,18 @@
>>>>> +/* { dg-do compile } */
>>>>> +/* { dg-require-effective-target arm_arch_v8a_ok } */
>>>>> +/* { dg-options "-O2" } */
>>>>>
>>>>> This should also have -marm as the problem exhibited itself in arm state.
>>>>> I'll commit this patch with this change in 24 hours on your behalf if no
>>>>> one
>>>>> objects.
>>>>>
>>>> Explicit use of -marm will break multi-lib testing.  I've forgotten the
>>>> correct hook, but there's most-likely something that will give you the
>>>> right behaviour, even if it means that thumb-only multi-lib testing
>>>> skips this test.
>>> So I think what we want is:
>>>
>>> dg-require-effective-target arm_arm_ok
>>>
>>> The comment in target-supports.exp is:
>>> # Return 1 if this is an ARM target where -marm causes ARM to be
>>> # used (not Thumb)
>>>
>> I've committed the attached patch to trunk on Shiva's behalf with r224269.
>> It gates the test on arm_arm_ok and adds -marm, like other similar tests.
>> The ChangeLog I used is below:
> I'd like to backport this to GCC 5 and 4.9
> The patch applies and tests cleanly on GCC 5.
> On 4.9 it needs some minor changes, which I'm attaching here.
> I've bootstrapped and tested this patch on 4.9 and the Shiva's
> original patch on GCC 5.
>
> 2015-09-30  Kyrylo Tkachov  <kyrylo.tkachov@arm>
>
>       Backport from mainline
>       2015-06-09  Shiva Chen  <shiva0217@gmail.com>
>
>       * sync.md (atomic_load<mode>): Add conditional code for lda/ldr
>       (atomic_store<mode>): Likewise.
>
> 2015-09-30  Kyrylo Tkachov  <kyrylo.tkachov@arm>
>
>       Backport from mainline
>       2015-06-09  Shiva Chen  <shiva0217@gmail.com>
>
>       * gcc.target/arm/stl-cond.c: New test.
>
>
> I'll commit them tomorrow.

I've now backported the patch to GCC 5 with r228322
and 4.9 with r228323.

Kyrill

> Thanks,
> Kyrill
>
>
>
>> 2015-06-09  Shiva Chen  <shiva0217@gmail.com>
>>
>>        * sync.md (atomic_load<mode>): Add conditional code for lda/ldr
>>        (atomic_store<mode>): Likewise.
>>
>> 2015-06-09  Shiva Chen  <shiva0217@gmail.com>
>>
>>        * gcc.target/arm/stl-cond.c: New test.
>>
>>
>> Thanks,
>> Kyrill
>>
>>> Kyrill
>>>
>>>
>>>> R.
>>>>
>>>>> Ramana, Richard, we need to backport it to GCC 5 as well, right?
>>>>>
>>>>> Thanks,
>>>>> Kyrill
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Shiva
>>>>>>
>>>>>> 2015-06-05 16:34 GMT+08:00 Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>:
>>>>>>> Hi Shiva,
>>>>>>>
>>>>>>> On 05/06/15 09:29, Shiva Chen wrote:
>>>>>>>> Hi, Kyrill
>>>>>>>>
>>>>>>>> I update the patch as Richard's suggestion.
>>>>>>>>
>>>>>>>> -      return \"str<sync_sfx>\t%1, %0\";
>>>>>>>> +      return \"str%(<sync_sfx>%)\t%1, %0\";
>>>>>>>>           else
>>>>>>>> -      return \"stl<sync_sfx>\t%1, %0\";
>>>>>>>> +      return \"stl<sync_sfx>%?\t%1, %0\";
>>>>>>>>         }
>>>>>>>> -)
>>>>>>>> +  [(set_attr "predicable" "yes")
>>>>>>>> +   (set_attr "predicable_short_it" "no")])
>>>>>>>> +  [(set_attr "predicable" "yes")
>>>>>>>> +   (set_attr "predicable_short_it" "no")])
>>>>>>>>
>>>>>>>>
>>>>>>>> Let me sum up.
>>>>>>>>
>>>>>>>> We add predicable attribute to allow gcc do if-conversion in
>>>>>>>> ce1/ce2/ce3 not only in final phase by final_prescan_insn finite state
>>>>>>>> machine.
>>>>>>>>
>>>>>>>> We set predicalble_short_it to "no" to restrict conditional code
>>>>>>>> generation on armv8 with thumb mode.
>>>>>>>>
>>>>>>>> However, we could use the flags -mno-restrict-it to force generating
>>>>>>>> conditional code on thumb mode.
>>>>>>>>
>>>>>>>> Therefore, we have to consider the assembly output format for strb
>>>>>>>> with condition code on arm/thumb mode.
>>>>>>>>
>>>>>>>> Because arm/thumb mode use different syntax for strb,
>>>>>>>> we output the assembly as str%(<sync_sfx>%)
>>>>>>>> which will put the condition code in the right place according to
>>>>>>>> TARGET_UNIFIED_ASM.
>>>>>>>>
>>>>>>>> Is there still missing something ?
>>>>>>> That's all correct, and well summarised :)
>>>>>>> The patch looks good to me, but please include the testcase
>>>>>>> (test.c from earlier) appropriately marked up for the testsuite.
>>>>>>> I think to the level of dg-assemble, just so we know everything is
>>>>>>> wired up properly.
>>>>>>>
>>>>>>> Thanks for dealing with this.
>>>>>>> Kyrill
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Shiva
>>>>>>>>
>>>>>>>> 2015-06-04 18:00 GMT+08:00 Kyrill Tkachov
>>>>>>>> <kyrylo.tkachov@foss.arm.com>:
>>>>>>>>> Hi Shiva,
>>>>>>>>>
>>>>>>>>> On 04/06/15 10:57, Shiva Chen wrote:
>>>>>>>>>> Hi, Kyrill
>>>>>>>>>>
>>>>>>>>>> Thanks for the tips of syntax.
>>>>>>>>>>
>>>>>>>>>> It seems that correct syntax for
>>>>>>>>>>
>>>>>>>>>> ldrb with condition code is ldreqb
>>>>>>>>>>
>>>>>>>>>> ldab with condition code is ldabeq
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So I modified the pattern as follow
>>>>>>>>>>
>>>>>>>>>>         {
>>>>>>>>>>           enum memmodel model = (enum memmodel) INTVAL (operands[2]);
>>>>>>>>>>           if (model == MEMMODEL_RELAXED
>>>>>>>>>>               || model == MEMMODEL_CONSUME
>>>>>>>>>>               || model == MEMMODEL_RELEASE)
>>>>>>>>>>             return \"ldr%?<sync_sfx>\\t%0, %1\";
>>>>>>>>>>           else
>>>>>>>>>>             return \"lda<sync_sfx>%?\\t%0, %1\";
>>>>>>>>>>         }
>>>>>>>>>>         [(set_attr "predicable" "yes")
>>>>>>>>>>          (set_attr "predicable_short_it" "no")])
>>>>>>>>>>
>>>>>>>>>> It seems we don't have to worry about thumb mode,
>>>>>>>>> I suggest you use Richard's suggestion from:
>>>>>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00384.html
>>>>>>>>> to write this in a clean way.
>>>>>>>>>
>>>>>>>>>> Because we already set "predicable" "yes" and predicable_short_it"
>>>>>>>>>> "no"
>>>>>>>>>> for the pattern.
>>>>>>>>> That's not quite true. The user may compile for armv8-a with
>>>>>>>>> -mno-restrict-it which will turn off this
>>>>>>>>> restriction for Thumb and allow the conditional execution of this.
>>>>>>>>> In any case, I think Richard's suggestion above should work.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Kyrill
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> The new patch could build gcc and run gcc regression test
>>>>>>>>>> successfully.
>>>>>>>>>>
>>>>>>>>>> Please correct me if I still missing something.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Shiva
>>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Richard Earnshaw [mailto:Richard.Earnshaw@foss.arm.com]
>>>>>>>>>> Sent: Thursday, June 04, 2015 4:42 PM
>>>>>>>>>> To: Kyrill Tkachov; Shiva Chen
>>>>>>>>>> Cc: Ramana Radhakrishnan; GCC Patches; nickc@redhat.com; Shiva Chen
>>>>>>>>>> Subject: Re: [GCC, ARM] armv8 linux toolchain asan testcase fail
>>>>>>>>>> due to
>>>>>>>>>> stl missing conditional code
>>>>>>>>>>
>>>>>>>>>> On 04/06/15 09:17, Kyrill Tkachov wrote:
>>>>>>>>>>> Hi Shiva,
>>>>>>>>>>>
>>>>>>>>>>> On 04/06/15 04:13, Shiva Chen wrote:
>>>>>>>>>>>> Hi, Ramana
>>>>>>>>>>>>
>>>>>>>>>>>> Currently, I work for Marvell and the company have copyright
>>>>>>>>>>>> assignment
>>>>>>>>>>>> on file.
>>>>>>>>>>>>
>>>>>>>>>>>> Hi, all
>>>>>>>>>>>>
>>>>>>>>>>>> After adding the attribute and rebuild gcc, I got the assembler
>>>>>>>>>>>> error
>>>>>>>>>>>> message
>>>>>>>>>>>>
>>>>>>>>>>>> load_n.s:39: Error: bad instruction `ldrbeq r0,[r0]'
>>>>>>>>>>>>
>>>>>>>>>>>> When i look into armv8 ISA document, it seems ldrb Encoding A1 have
>>>>>>>>>>>> conditional code field.
>>>>>>>>>>>>
>>>>>>>>>>>> Does it mean we should also patch assembler or I just miss
>>>>>>>>>>>> understanding something ?
>>>>>>>>>>>>
>>>>>>>>>>>> Following command use to generate load_n.s:
>>>>>>>>>>>>
>>>>>>>>>>>> /home/shivac/build-system-trunk/Release/build/armv8-marvell-linux-gnu
>>>>>>>>>>>>
>>>>>>>>>>>> eabihf-hard/gcc-final/./gcc/cc1 -fpreprocessed load_n.i -quiet
>>>>>>>>>>>> -dumpbase load_n.c -march=armv8-a -mfloat-abi=hard -mfpu=fp-armv8
>>>>>>>>>>>> -mtls-dialect=gnu -auxbase-strip .libs/load_1_.o -g3 -O2 -Wall
>>>>>>>>>>>> -Werror -version -fPIC -funwind-tables -o load_n.s
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> The test.c is a simple test case to reproduce missing conditional
>>>>>>>>>>>> code in mmap.c.
>>>>>>>>>>>>
>>>>>>>>>>>> Any suggestion ?
>>>>>>>>>>> I reproduced the assembler failure with your patch.
>>>>>>>>>>>
>>>>>>>>>>> The reason is that for arm mode we use divided syntax, where the
>>>>>>>>>>> condition field goes in a different place. So, while ldrbeq
>>>>>>>>>>> r0,[r0] is
>>>>>>>>>>> rejected, ldreqb r0, [r0] works.
>>>>>>>>>>> Since we always use divided syntax for arm mode, I think you'll need
>>>>>>>>>>> to put the condition field in the right place depending on arm or
>>>>>>>>>>> thumb
>>>>>>>>>>> mode.
>>>>>>>>>>> Ugh, this is becoming ugly :(
>>>>>>>>>>>
>>>>>>>>>> Use %(<suffix%) around the bit that changes for unified/divided
>>>>>>>>>> syntax.
>>>>>>>>>>        The compiler will then put the condition in the correct place.
>>>>>>>>>>
>>>>>>>>>> So:
>>>>>>>>>>
>>>>>>>>>> +      return \"str%(<sync_sfx>%)\t%1, %0\";
>>>>>>>>>>
>>>>>>>>>> R.
>>>>>>>>>>
>>>>>>>>>>> Kyrill
>>>>>>>>>>>
>>>>>>>>>>>> Shiva
>>>>>>>>>>>>
>>>>>>>>>>>> 2015-06-03 17:29 GMT+08:00 Shiva Chen <shiva0217@gmail.com>:
>>>>>>>>>>>>> Hi, Ramana
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'm not sure what copyright assignment means ?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Does it mean the patch have copyright assignment or not ?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I update the patch to add "predicable" and "predicable_short_it"
>>>>>>>>>>>>> attribute as suggestion.
>>>>>>>>>>>>>
>>>>>>>>>>>>> However, I don't have svn write access yet.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Shiva
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2015-06-03 16:36 GMT+08:00 Kyrill Tkachov
>>>>>>>>>>>>> <kyrylo.tkachov@arm.com>:
>>>>>>>>>>>>>> On 03/06/15 09:32, Ramana Radhakrishnan wrote:
>>>>>>>>>>>>>>>> This pattern is not predicable though, i.e. it doesn't have the
>>>>>>>>>>>>>>>> "predicable" attribute set to "yes".
>>>>>>>>>>>>>>>> Therefore the compiler should be trying to branch around here
>>>>>>>>>>>>>>>> rather than try to do a cond_exec.
>>>>>>>>>>>>>>>> Why does the generated code above look like it's converted to
>>>>>>>>>>>>>>>> conditional execution?
>>>>>>>>>>>>>>>> Could you produce a self-contained reduced testcase for this?
>>>>>>>>>>>>>>> CCFSM state machine in ARM state.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> arm.c (final_prescan_insn).
>>>>>>>>>>>>>> Ah ok.
>>>>>>>>>>>>>> This patch makes sense then.
>>>>>>>>>>>>>> As Ramana mentioned, please mark the pattern with "predicable"
>>>>>>>>>>>>>> and
>>>>>>>>>>>>>> also set the "predicable_short_it" attribute to "no" so that it
>>>>>>>>>>>>>> will not be conditionalised in Thumb2 mode or when
>>>>>>>>>>>>>> -mrestrict-it is
>>>>>>>>>>>>>> enabled.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Kyrill
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ramana
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> Kyrill
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> @@ -91,9 +91,9 @@
>>>>>>>>>>>>>>>>>              {
>>>>>>>>>>>>>>>>>                enum memmodel model = memmodel_from_int (INTVAL
>>>>>>>>>>>>>>>>> (operands[2]));
>>>>>>>>>>>>>>>>>                if (is_mm_relaxed (model) || is_mm_consume
>>>>>>>>>>>>>>>>> (model) ||
>>>>>>>>>>>>>>>>> is_mm_acquire (model))
>>>>>>>>>>>>>>>>> -      return \"str<sync_sfx>\t%1, %0\";
>>>>>>>>>>>>>>>>> +      return \"str<sync_sfx>%?\t%1, %0\";
>>>>>>>>>>>>>>>>>                else
>>>>>>>>>>>>>>>>> -      return \"stl<sync_sfx>\t%1, %0\";
>>>>>>>>>>>>>>>>> +      return \"stl<sync_sfx>%?\t%1, %0\";
>>>>>>>>>>>>>>>>>              }
>>>>>>>>>>>>>>>>>            )
>>>>>>>>>>>>>>>>>