diff mbox

[ARM] armv8 linux toolchain asan testcase fail due to stl missing conditional code

Message ID 560E7F46.7000003@arm.com
State New
Headers show

Commit Message

Kyrylo Tkachov Oct. 2, 2015, 12:57 p.m. UTC 
On 01/10/15 21:21, Christophe Lyon wrote:
> On 1 October 2015 at 11:10, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>> 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.
>>
> Hi Kyrill,

Hi Christophe,

>
> The backport in 4.9 causes build failures in libatomic when GCC is
> configured as:
> --with-cpu=cortex-a57
> --with-fpu=crypto-neon-fp-armv8
> --with-mode=arm
> --target=arm-none-linux-gnueabihf
>
> For instance when building store_1_.lo:
> /tmp/6529147_22.tmpdir/cceUjViw.s:36: Error: bad instruction `stlneb r1,[r0]'
>
> when building load_1_.lo:
> /tmp/6529147_22.tmpdir/cchhKmHw.s:37: Error: bad instruction `ldaneb r0,[r0]'

Huh, sorry for that.
I did bootstrap 4.9 configured --with-arch=armv8-a, don't know why the failure didn't flare up.

I did reproduce the bad code with a custom testcase.
This patch fixes it and brings the code in line with GCC 5 and trunk.

Bootstrapped and tested on the 4.9 branch.
Committed under the fixes-the-build rule to the 4.9 branch with r228389.

Thanks,
Kyrill

2015-10-02  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * sync.md (atomic_load<mode>): Fix output modifier for lda.
     (atomic_store<mode>): Likewise for stl.

> Christophe.
>
>> 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\";
>>>>>>>>>>>>>>>>>>>               }
>>>>>>>>>>>>>>>>>>>             )
>>>>>>>>>>>>>>>>>>>
diff mbox

Patch

diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md
index 747fc7e..25ed926 100644
--- a/gcc/config/arm/sync.md
+++ b/gcc/config/arm/sync.md
@@ -79,7 +79,7 @@ 
         || model == MEMMODEL_RELEASE)
       return \"ldr%(<sync_sfx>%)\\t%0, %1\";
     else
-      return \"lda%(<sync_sfx>%)\\t%0, %1\";
+      return \"lda<sync_sfx>%?\\t%0, %1\";
   }
   [(set_attr "predicable" "yes")
    (set_attr "predicable_short_it" "no")])
@@ -98,7 +98,7 @@ 
         || model == MEMMODEL_ACQUIRE)
       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")])