[Bug target/67281] New: HTM builtins aren't treated as compiler barriers on powerpc

[Bug target/67281] New: HTM builtins aren't treated as compiler barriers on powerpc

[tuliom at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

            Bug ID: 67281
           Summary: HTM builtins aren't treated as compiler barriers on
                    powerpc
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: tuliom at linux dot vnet.ibm.com
  Target Milestone: ---

Depending on the level of optimization, GCC moves a memory access outside a
transaction which breaks the atomicity of the transaction.

This is a small testcase that reproduces this behavior:

$ cat tbegin-barrier.c
long
foo (long dest, long src0, long src1, long tries)
{
  long i;
  for (i = 0; i < tries; i++)
    {
      __builtin_tbegin (0);
      dest = src0 + src1;
      __builtin_tend (0);
    }
  return dest;
}

compiling using: -O2 -S -mcpu=power8 tbegin-barrier.c
gives
foo:
        cmpdi 0,6,0
        blelr 0
        mtctr 6
        add 3,4,5
        .p2align 4,,15
.L3:
        tbegin. 0
        tend. 0
        bdnz .L3
        blr

[Bug target/67281] HTM builtins aren't treated as compiler barriers on powerpc

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There is no memory access in your testcase at all. Since the variables are all
local variables and nothing takes the address, it can be moved out of the loop
as no other thread can cause that to fail.

[Bug target/67281] HTM builtins aren't treated as compiler barriers on powerpc

[tuliom at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

--- Comment #2 from Tulio Magno Quites Machado Filho <tuliom at linux dot vnet.ibm.com> ---
Oooops.  My bad.
What about this one?

$ cat tbegin-barrier.c
long
foo (long dest, long *src0, long src1, long tries)
{
  long i;
  for (i = 0; i < tries; i++)
    {
      __builtin_tbegin (0);
      dest = *src0 + src1;
      __builtin_tend (0);
    }
  return dest;
}

If we compile it the same way:

foo:
        cmpdi 0,6,0
        blelr 0
        mtctr 6
        ld 3,0(4)
        .p2align 4,,15
.L3:
        tbegin. 0
        tend. 0
        bdnz .L3
        add 3,3,5
        blr

[Bug target/67281] HTM builtins aren't treated as compiler barriers on powerpc

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Since there are no stores, the load seems like it can be pulled out of the loop
too.

[Bug target/67281] HTM builtins aren't treated as compiler barriers on powerpc

[tuliom at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

--- Comment #4 from Tulio Magno Quites Machado Filho <tuliom at linux dot vnet.ibm.com> ---
(In reply to Andrew Pinski from comment #3)
> Since there are no stores, the load seems like it can be pulled out of the
> loop too.

I disagree with you.
If I use the value of dest to take a decision inside the transaction, I need
the memory barrier before the access to *src0.

Here's an example:

long
foo (long dest, long *src0, long src1, long tries)
{
  long i;
  for (i = 0; i < tries; i++)
    {
      __builtin_tbegin (0);
      dest = *src0 + src1;
      if (dest == 13)
        __builtin_tabort(0);
      __builtin_tend (0);
    }
  return dest;
}

In other words, if you access *src0 before the memory barrier, its value may
change when the memory barrier is created.  This is particularly useful if dest
says if a lock has been acquired by another thread or not.

For the reference, this has been found in glibc source code:
https://sourceware.org/ml/libc-alpha/2015-07/msg00986.html

[Bug target/67281] HTM builtins aren't treated as compiler barriers on powerpc

[torvald at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

torvald at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |torvald at gcc dot gnu.org

--- Comment #6 from torvald at gcc dot gnu.org ---
I think tbegin needs to have same semantics as a lock acquisition and the
compiler must not assume to know anything about tbegin's return value; tend
must have same semantics as a lock release.

See the libc-alpha discussion for why I think this is the case:
https://sourceware.org/ml/libc-alpha/2015-08/msg00963.html

Thus, I don't think a full compiler barrier is necessary, but if we don't have
something finer-grained to capture the semantics of a lock acquisition, then we
need the compiler barrier (GCC currently assumes atomics to be compiler
barriers AFAIK).  We should in any case agree on a semantics and document it in
the GCC sources.  Documenting that we need a full compiler barrier is not
correct in that it's not a necessary condition (even though it should be a
sufficient condition).

[Bug target/67281] HTM builtins aren't treated as compiler barriers on powerpc

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

--- Comment #7 from Peter Bergner <bergner at gcc dot gnu.org> ---
Author: bergner
Date: Thu Oct 15 16:38:47 2015
New Revision: 228846

URL: https://gcc.gnu.org/viewcvs?rev=228846&root=gcc&view=rev
Log:
        Backport from mainline
        2015-10-14  Peter Bergner  <bergner@vnet.ibm.com>
                    Torvald Riegel  <triegel@redhat.com>

        PR target/67281
        * config/rs6000/htm.md (UNSPEC_HTM_FENCE): New.
        (tabort, tabort<wd>c, tabort<wd>ci, tbegin, tcheck, tend,
        trechkpt, treclaim, tsr, ttest): Rename define_insns from this...
        (*tabort, *tabort<wd>c, *tabort<wd>ci, *tbegin, *tcheck, *tend,
        *trechkpt, *treclaim, *tsr, *ttest): ...to this.  Add memory barrier.
        (tabort, tabort<wd>c, tabort<wd>ci, tbegin, tcheck, tend,
        trechkpt, treclaim, tsr, ttest): New define_expands.
        * config/rs6000/rs6000-c.c (rs6000_target_modify_macros): Define
        __TM_FENCE__ for htm.
        * doc/extend.texi: Update documentation for htm builtins.

Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/config/rs6000/htm.md
    branches/gcc-5-branch/gcc/config/rs6000/rs6000-c.c
    branches/gcc-5-branch/gcc/doc/extend.texi

[Bug target/67281] HTM builtins aren't treated as compiler barriers on powerpc

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

--- Comment #8 from Peter Bergner <bergner at gcc dot gnu.org> ---
Author: bergner
Date: Thu Oct 15 16:40:14 2015
New Revision: 228847

URL: https://gcc.gnu.org/viewcvs?rev=228847&root=gcc&view=rev
Log:
        Backport from mainline
        2015-10-14  Peter Bergner  <bergner@vnet.ibm.com>
                    Torvald Riegel  <triegel@redhat.com>

        PR target/67281
        * config/rs6000/htm.md (UNSPEC_HTM_FENCE): New.
        (tabort, tabort<wd>c, tabort<wd>ci, tbegin, tcheck, tend,
        trechkpt, treclaim, tsr, ttest): Rename define_insns from this...
        (*tabort, *tabort<wd>c, *tabort<wd>ci, *tbegin, *tcheck, *tend,
        *trechkpt, *treclaim, *tsr, *ttest): ...to this.  Add memory barrier.
        (tabort, tabort<wd>c, tabort<wd>ci, tbegin, tcheck, tend,
        trechkpt, treclaim, tsr, ttest): New define_expands.
        * config/rs6000/rs6000-c.c (rs6000_target_modify_macros): Define
        __TM_FENCE__ for htm.
        * doc/extend.texi: Update documentation for htm builtins.

Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/config/rs6000/htm.md
    branches/gcc-4_9-branch/gcc/config/rs6000/rs6000-c.c
    branches/gcc-4_9-branch/gcc/doc/extend.texi

[Bug target/67281] HTM builtins aren't treated as compiler barriers on powerpc

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67281

Peter Bergner <bergner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |powerpc*-linux
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED
           Assignee|unassigned at gcc dot gnu.org      |bergner at gcc dot gnu.org
   Target Milestone|---                         |6.0

--- Comment #9 from Peter Bergner <bergner at gcc dot gnu.org> ---
Fixed in trunk and the FSF 5 and 4.9 branches.