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

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