[Bug target/55139] New: __atomic store does not support __ATOMIC_HLE_RELEASE

[Bug target/55139] New: __atomic store does not support __ATOMIC_HLE_RELEASE

[andi-gcc at firstfloor dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55139

             Bug #: 55139
           Summary: __atomic store does not support __ATOMIC_HLE_RELEASE
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: andi-gcc@firstfloor.org


volatile int slock;

void unlock(void)
{
        int free_val = 1;
        __atomic_store(&slock, &free_val,
__ATOMIC_RELEASE|__ATOMIC_HLE_RELEASE);
}

spin.c: In function 'unlock':
spin.c:6:16: warning: invalid memory model argument 3 of '__atomic_store'
[-Winvalid-memory-model]
  __atomic_store(&slock, &free_val, __ATOMIC_RELEASE|__ATOMIC_HLE_RELEASE);


But XRELEASE MOV ... is allowed in TSX.

[Bug target/55139] __atomic store does not support __ATOMIC_HLE_RELEASE

[andi-gcc at firstfloor dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55139

--- Comment #1 from Andi Kleen <andi-gcc at firstfloor dot org> 2012-11-07 04:03:53 UTC ---
This is an interesting one. This is the gcc code:

enum memmodel
{
  MEMMODEL_RELAXED = 0,
  MEMMODEL_CONSUME = 1,
  MEMMODEL_ACQUIRE = 2,
  MEMMODEL_RELEASE = 3,
  MEMMODEL_ACQ_REL = 4,
  MEMMODEL_SEQ_CST = 5,
  MEMMODEL_LAST = 6
};
#define MEMMODEL_MASK ((1<<16)-1)

 enum memmodel model;

  model = get_memmodel (CALL_EXPR_ARG (exp, 2));
  if ((model & MEMMODEL_MASK) != MEMMODEL_RELAXED
      && (model & MEMMODEL_MASK) != MEMMODEL_SEQ_CST
      && (model & MEMMODEL_MASK) != MEMMODEL_RELEASE)
    {
      error ("invalid memory model for %<__atomic_store%>");
      return NULL_RTX;
    }

HLE_STORE is 1 << 16, so outside the enum range

But when looking at the assembler we see that the & MEMMODEL_MASK
gets optimized away, it just generates a direct sequence of 32bit cmps.  

This makes all the != fail, even though they should succeed

I presume the optimizer assumes nothing can be outside the enum.

I tried to expand the enum by adding

  MEMMODEL_ARCH1 =  1 << 16,
  MEMMODEL_ARCH2 =  1 << 17,
  MEMMODEL_ARCH3 =  1 << 18,
  MEMMODEL_ARCH4 =  1 << 19

But still doesn't work.

Questions:
- Is it legal for the optimizer to assume this?
- Why does extending the enum not help?

We could fix it by not using an enum here of course, but I wonder if this is an
underlying optimizer bug.

[Bug target/55139] __atomic store does not support __ATOMIC_HLE_RELEASE

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55139

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-07 14:30:49 UTC ---
What compiler compiled the code?  Older g++s would optimize it away, but newer
ones should have -fno-strict-enums by default.  That said, guess it would be
better to rewrite it to use int anyway.

[Bug target/55139] __atomic store does not support __ATOMIC_HLE_RELEASE

[andi-gcc at firstfloor dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55139

--- Comment #3 from Andi Kleen <andi-gcc at firstfloor dot org> 2012-11-07 14:45:17 UTC ---
I saw the problem both with bootstrapped and non bootstrapped (4.6 base)
compilers
I haven't checked if it's always the missing and, but it's likely

Ok I can change everything to int, but why did extending the enum not help?
I fear that I may hide some bug this way.

Also how to fix libstdc++? It seems to have a similar enum in its headers
(it doesn't support HLE yet, but that's another bug)

[Bug target/55139] __atomic store does not support __ATOMIC_HLE_RELEASE

[andi-gcc at firstfloor dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55139

--- Comment #4 from Andi Kleen <andi-gcc at firstfloor dot org> 2012-11-09 14:06:20 UTC ---
My earlier analysis was not correct. I was chasing the wrong warning.
Rather the problem is in c-common.c, where the atomic models are checked again.
I'm sending a patch for that.

[Bug target/55139] __atomic store does not support __ATOMIC_HLE_RELEASE

[ak at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55139

--- Comment #5 from ak at gcc dot gnu.org 2012-11-09 15:24:32 UTC ---
Author: ak
Date: Fri Nov  9 15:24:25 2012
New Revision: 193363

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193363
Log:
Handle target specific memory models in C frontend

get_atomic_generic_size would error out for
__atomic_store(...,__ATOMIC_HLE_RELEASE)

Just mask it out. All the memory orders are checked completely
in builtins.c anyways.

I'm not sure what that check is for, it could be removed in theory.

Passed bootstrap and test suite on x86-64

gcc/c-family/:
2012-11-09  Andi Kleen  <ak@linux.intel.com>

    PR 55139
    * c-common.c (get_atomic_generic_size): Mask with
        MEMMODEL_MASK

Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c

[Bug target/55139] __atomic store does not support __ATOMIC_HLE_RELEASE

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED
   Target Milestone|---                         |4.8.0

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Fixed a long time ago.