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

[Richard Earnshaw <Richard dot Earnshaw at foss dot arm dot com>]

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.

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\";
>>>>>>>>>>>>>           }
>>>>>>>>>>>>>         )
>>>>>>>>>>>>>
>