use arm assembly bits only for gcc < 4.6 on ARM > 6

Bug #726529 reported by Jani Monoses on 2011-02-28
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
libreoffice (Ubuntu)
Undecided
Björn Michaelsen

Bug Description

Binary package hint: libreoffice

I think it makes more sense to just use gcc builtins for atomic operations and do not try to distinguish between arm flavours.
The compiler defaults depending on debian/ubuntu should be adequate. This would get rid of expliciti --with-arm-flavour configure option and the related patch.

Upstream master has this (not on 3.3) which seems to be a subset of the Ubuntu patch and it helps powerpc as well.
http://cgit.freedesktop.org/libreoffice/ure/commit/?id=788072cefdce8cb61d46549a7aede4c754d9fae3

After 3.4 the whole arm_optimization patch can be dropped.

I talked this through with doko: Long story short we still want the asm parts in master for backports, were we still might have gcc < 4.6.

Changed in libreoffice (Ubuntu):
status: New → In Progress
summary: - drop arm assembly bits from patch
+ use arm assembly bits only for gcc < 4.6 on ARM > 6
Jani Monoses (jani) wrote :

gcc 4.5 generates the same code as in the assembly - or is it just gcc-4.5 linaro on natty?
Even better it adds a dmb instruction which the asm block is missing, causing it to not be SMP safe.

Code generated by default gcc 4.5 on natty.

   a: f04f 0201 mov.w r2, #1
   e: f3bf 8f5f dmb sy
  12: e853 1f00 ldrex r1, [r3]
  16: 4411 add r1, r2
  18: e843 1000 strex r0, r1, [r3]
  1c: f090 0f00 teq r0, #0
  20: d1f7 bne.n 12 <a+0x12>

On 28.02.2011 17:49, Jani Monoses wrote:
> gcc 4.5 generates the same code as in the assembly - or is it just gcc-4.5 linaro on natty?

yes.

> Even better it adds a dmb instruction which the asm block is missing, causing it to not be SMP safe.
>
> Code generated by default gcc 4.5 on natty.
>
> a: f04f 0201 mov.w r2, #1
> e: f3bf 8f5f dmb sy
> 12: e853 1f00 ldrex r1, [r3]
> 16: 4411 add r1, r2
> 18: e843 1000 strex r0, r1, [r3]
> 1c: f090 0f00 teq r0, #0
> 20: d1f7 bne.n 12 <a+0x12>

what dmart explain on #linaro ;)

<dmart> doko_: I would expect __sync_{add,sub}_and_fetch() to be close in
performance to the inlined asm for v6/v7. __sync_*() also has the needed
barriers -- the libreoffice code I see there doesn't, so it might not always
work correctly in some contexts on true SMP... but maybe the barriers happen
elsewhere

Changed in libreoffice (Ubuntu):
assignee: nobody → Björn Michaelsen (bjoern-michaelsen)

Jani: If I understood doko's discussion correctly it we would still need it as 4.5 Linaro not 4.5 FSF.
@doko: Please confirm.

Other than that, the patch would not hurt either way as-is (because it generates the same code), right?

Jani Monoses (jani) wrote :

From what doko says indeed on non-Ubuntu (and thus non Linaro) gcc 4.5 does not generate the more optimal code.

In general I was confused by the fact that this is not natty specific. I was thinking lucid/maverick etc have their own packaging branches and the natty package does not carry legacy stuff.

In either case the missing dmb instruction makes this not SMP safe. I think the asm patches were originally written before there were SMP ARM boards available making it a non-issue back then.

@Jani: As the original code by Erics Bachard is not guaranteed to be SMP-safe, could you please post a patch with the linaro generated code, so we could completely replace the asm code in:
 http://cgit.freedesktop.org/libreoffice/ure/commit/?id=83f2c071758ae7d74669d992e272e50057b895ed
at upstream with it?

Jani Monoses (jani) wrote :

This is how the two routines need to be modified:

The code block needs to be wrapped by dmb (memory barrier instruction)

    register int nCount __asm__ ("r1");
    int nResult;

    __asm__ __volatile__ (
"dmb\n" <- here
"1: ldrex %0, [%3]\n"
" add %0, %0, #1\n"
" strex %1, %0, [%3]\n"
" teq %1, #0\n"
" bne 1b\n"
"dmb\n" <-here
        : "=&r" (nCount), "=&r" (nResult), "=m" (*pCount)
        : "r" (pCount)
        : "memory");

I also sent a patch against LibO in email

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libreoffice - 1:3.3.1-1ubuntu5

---------------
libreoffice (1:3.3.1-1ubuntu5) natty; urgency=low

  * completely disable the arm patch for natty (LP: #726529)
  * make libreoffice-gtk recommend either tango or human (LP: #726921)
  * added ppc fixes (LP: #727118)
  * enable lzma everywhere except armel, not the other way around
  * regenerate control file
 -- Bjoern Michaelsen <email address hidden> Tue, 08 Mar 2011 11:45:10 +0100

Changed in libreoffice (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers