This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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 --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")])
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]