On Tue, Apr 17, 2012 at 6:41 PM, Andi Kleen wrote:
>> I also have a question regarding AS compatibility. In case one built
>> GCC using AS with support of HLE then using this GCC on a machine with
>> old AS will cause fail because of usupported prefix. Can we support it
>
> I don't think that's a
> I also have a question regarding AS compatibility. In case one built
> GCC using AS with support of HLE then using this GCC on a machine with
> old AS will cause fail because of usupported prefix. Can we support it
I don't think that's a supported use case for gcc.
It also doesn't work with .cfi
>
> Any other inputs?
>
I would suggest to use "snprintf" b/gcc/config/i386/i386-c.c to avoid
possible buffer overrun.
I also have a question regarding AS compatibility. In case one built
GCC using AS with support of HLE then using this GCC on a machine with
old AS will cause fail because of usup
> No, just the bits; programmers would need to do
> __atomic_...(..., __ATOMIC_RELEASE | HLE_RELEASE);
> I believe this is what you had in one of your versions of the patch. My
> suggestions was not about doing something new but instead a
> suggestions/poll for a resolution of the discussion.
Oh
On Thu, 2012-04-12 at 18:13 +0400, Kirill Yukhin wrote:
> > I would suggest that we keep the HLE acq/rel bits independent of the
> > memory order bits. Both are independent on a conceptual level. And we
> > should add documentation that tells programmers that memory orders need
> > always be spec
> I would suggest that we keep the HLE acq/rel bits independent of the
> memory order bits. Both are independent on a conceptual level. And we
> should add documentation that tells programmers that memory orders need
> always be specified.
>
Sorry, I didn't get your point. You propose to separate
> Would that be an acceptable option for everyone? Andi, would proper
> documentation resolve your ease-of-use concerns?
Proper documentation is needed in any case, but I would strongly prefer an
inherently hard-to-misuse interface. I think Andrew's proposal is the best
for that so far.
-Andi
On Thu, 2012-04-12 at 08:21 -0400, Andrew MacLeod wrote:
> On 04/11/2012 06:38 PM, Torvald Riegel wrote:
> > On Wed, 2012-04-11 at 15:06 +0200, Andi Kleen wrote:
> >>
> > Perhaps HLE_ACQUIRE / HLE_RELEASE should be something like HLE_START /
> > HLE_END instead? Not particularly great names, but a
> > __ATOMIC_HLE_XACQ_CONSUME
> > __ATOMIC_HLE_XACQ_ACQUIRE
> > __ATOMIC_HLE_XACQ_ACQ_REL
> > __ATOMIC_HLE_XACQ_SEQ_CST
> >
> > __ATOMIC_HLE_XREL_RELEASE
> > __ATOMIC_HLE_XREL_ACQ_REL
> > __ATOMIC_HLE_XREL_SEQ_CST
> >
> > or whatever happens to be valid... Doesn't really scale to adding
> > mor
> What if another vendor shows up, perhaps on another architecture?
HLE code is currently usually x86 specific. e.g. for practical spin locks
you have to include a __builtin_ia32_pause() on lock locked to stop speculation,
otherwise the lock path will speculate too, which is very inefficient.
So
Hi Andrew,
> Does it make any sense to simply predefine the possible valid
> combinations with the HLE bit already set? it at least removes any
> possible invalid combinations and forces the programmer to consciously
> choose their memory model.
>
> ie,
> __ATOMIC_HLE_XACQ_CONSUME
> __ATOMIC_
On Thu, 2012-04-12 at 13:36 +0200, Andi Kleen wrote:
> On Thu, Apr 12, 2012 at 12:38:44AM +0200, Torvald Riegel wrote:
> > On Wed, 2012-04-11 at 15:06 +0200, Andi Kleen wrote:
> > > > Tests passing, bootstrap in progress.
> > > >
> > > > Comments?
> > >
> > > Do you really imply ACQUIRE/RELEASE w
On Thu, Apr 12, 2012 at 08:21:47AM -0400, Andrew MacLeod wrote:
> On 04/11/2012 06:38 PM, Torvald Riegel wrote:
> >On Wed, 2012-04-11 at 15:06 +0200, Andi Kleen wrote:
> >Perhaps HLE_ACQUIRE / HLE_RELEASE should be something like HLE_START /
> >HLE_END instead? Not particularly great names, but at
On 04/11/2012 06:38 PM, Torvald Riegel wrote:
On Wed, 2012-04-11 at 15:06 +0200, Andi Kleen wrote:
Perhaps HLE_ACQUIRE / HLE_RELEASE should be something like HLE_START /
HLE_END instead? Not particularly great names, but at least it avoids
overloading ACQUIRE/RELEASE and thus should make it c
On Thu, Apr 12, 2012 at 12:38:44AM +0200, Torvald Riegel wrote:
> On Wed, 2012-04-11 at 15:06 +0200, Andi Kleen wrote:
> > > Tests passing, bootstrap in progress.
> > >
> > > Comments?
> >
> > Do you really imply ACQUIRE/RELEASE with HLE_ACQUIRE/RELEASE now? I don't
> > see that in the code. I th
On Thu, Apr 12, 2012 at 01:37:24PM +0400, Kirill Yukhin wrote:
> Folks,
> Here is patch with removed implied atomic ACQUIRE/RELEASE. Could you
> please have a look?
+
Folks,
Here is patch with removed implied atomic ACQUIRE/RELEASE. Could you
please have a look?
ChangeLog entry:
2012-04-12 Kirill Yukhin
* builtins.c (get_memmodel): Remove check of upper bound.
(expand_builtin_atomic_compare_exchange): Mask memmodel values.
* config/i
> Perhaps HLE_ACQUIRE / HLE_RELEASE should be something like HLE_START /
> HLE_END instead? Not particularly great names, but at least it avoids
> overloading ACQUIRE/RELEASE and thus should make it clearer that you
> still need to specify a memory order.
>
IMHO, this is also not as good, since AC
On Wed, 2012-04-11 at 15:06 +0200, Andi Kleen wrote:
> > Tests passing, bootstrap in progress.
> >
> > Comments?
>
> Do you really imply ACQUIRE/RELEASE with HLE_ACQUIRE/RELEASE now? I don't
> see that in the code. I think that's really required, otherwise the optimizer
> will do the wrong thing
> > That's true. Actually I see the values are defined by the compiler
> > at compile time, so it would be possible to move all one up?
>
> No, that is IMHO not possible. They need to match the enum values that are
> part of libstdc++ ABI already, and not everybody is going to use the
> __ATOMIC_
On Wed, Apr 11, 2012 at 6:06 PM, Andi Kleen wrote:
> + static char buf[128], hle[16];
>
> The hle buffer does not need to be static.
> BTW I'm surprised there is no better way to do this in machine descriptions
> than to use static buffers.
Oh, there is. Since we are looking at the operands, we
On Wed, Apr 11, 2012 at 06:40:03PM +0200, Andi Kleen wrote:
> > But such a model isn't possible. The HLE bits are just some high bits
> > ored into the memory model enum. So, if you use
> > __ATOMIC_HLE_ACQUIRE, it is the same thing as
> > __ATOMIC_HLE_ACQUIRE | __ATOMIC_RELAXED and thus it is a
> But such a model isn't possible. The HLE bits are just some high bits
> ored into the memory model enum. So, if you use
> __ATOMIC_HLE_ACQUIRE, it is the same thing as
> __ATOMIC_HLE_ACQUIRE | __ATOMIC_RELAXED and thus it is a relaxed xacquire,
> not xacquire with default memory model.
> __atom
On Wed, Apr 11, 2012 at 06:18:56PM +0200, Andi Kleen wrote:
> > I actually think it is a bad idea to imply any memory model
> > from the HLE bits. If anything, we should warn for memmodel
> > + hle bit combinations that are unlikely to DTRT.
>
> This would be a warning with _RELAXED/_CONSUME, but
> I actually think it is a bad idea to imply any memory model
> from the HLE bits. If anything, we should warn for memmodel
> + hle bit combinations that are unlikely to DTRT.
This would be a warning with _RELAXED/_CONSUME, but there may be very
obscure situations where someone really wants that
On Wed, Apr 11, 2012 at 06:06:58PM +0200, Andi Kleen wrote:
> +static unsigned HOST_WIDE_INT
> +ix86_extend_hle_macro (unsigned HOST_WIDE_INT memmodel)
> +{
> + unsigned HOST_WIDE_INT result = memmodel;
> +
> + if (memmodel & IX86_HLE_ACQUIRE)
> +result |= MEMMODEL_ACQUIRE;
> +
> + if (memmo
On Wed, Apr 11, 2012 at 07:52:59PM +0400, Kirill Yukhin wrote:
> Yet another iteration :)
>
> >> > Do you really imply ACQUIRE/RELEASE with HLE_ACQUIRE/RELEASE now? I don't
> Sorry, Andi. Added. So, at the moment you can do smth like
> __atomic_compare_exchange_n (p, &oldv, newv, 0,
> __ATOMIC_H
Yet another iteration :)
>> > Do you really imply ACQUIRE/RELEASE with HLE_ACQUIRE/RELEASE now? I don't
Sorry, Andi. Added. So, at the moment you can do smth like
__atomic_compare_exchange_n (p, &oldv, newv, 0,
__ATOMIC_HLE_ACQUIRE, __ATOMIC_ACQUIRE);
And will get __ATOMIC_ACQUIRE model as well
On Wed, Apr 11, 2012 at 03:12:44PM +0200, Jakub Jelinek wrote:
> On Wed, Apr 11, 2012 at 03:06:35PM +0200, Andi Kleen wrote:
> > Do you really imply ACQUIRE/RELEASE with HLE_ACQUIRE/RELEASE now? I don't
> > see that in the code. I think that's really required, otherwise the
> > optimizer
> > will
On Wed, Apr 11, 2012 at 03:06:35PM +0200, Andi Kleen wrote:
> Do you really imply ACQUIRE/RELEASE with HLE_ACQUIRE/RELEASE now? I don't
> see that in the code. I think that's really required, otherwise the optimizer
> will do the wrong thing and move memory references outside the region.
IMHO the
> Tests passing, bootstrap in progress.
>
> Comments?
Do you really imply ACQUIRE/RELEASE with HLE_ACQUIRE/RELEASE now? I don't
see that in the code. I think that's really required, otherwise the optimizer
will do the wrong thing and move memory references outside the region.
I second Jakub in d
On Wed, Apr 11, 2012 at 12:51 PM, Jakub Jelinek wrote:
> What is TARGET_HLE good for? I thought the point of HLE prefixes
> is that they are silently ignored on older CPUs. So, HLE should be
> always enabled IMHO. If you don't use __ATOMIC_HLE_* bits in __atomic_*
> in your source, it won't be
Hi!
On Wed, Apr 11, 2012 at 02:35:38PM +0400, Kirill Yukhin wrote:
What is TARGET_HLE good for? I thought the point of HLE prefixes
is that they are silently ignored on older CPUs. So, HLE should be
always enabled IMHO. If you don't use __ATOMIC_HLE_* bits in __atomic_*
in your source, it won'
Folks,
Thanks a lot for inputs and suggestions!
Here is updated version of patch.
ChangeLog entry:
2012-04-11 Kirill Yukhin
* builtins.c (get_memmodel): Remove check of upper bound.
(expand_builtin_atomic_compare_exchange): Mask memmodel values.
* config/i386/cpuid.h (b
> If it is the case, can we generate HLE RELEASE/ACQUIRE
> prefix automatically for C++ atomics via -mhle command line option.
> Then you don't need to modify the source codes to enable HLE support.
No, for HLE someone needs to decide whether HLE is beneficial for a given
lock. There are cases wh
On Tue, Apr 10, 2012 at 9:34 AM, Andi Kleen wrote:
> "H.J. Lu" writes:
>
>>> So, to emit HLE prefix, it is possible to do:
>>> int
>>> foo2 (int *p, int oldv, int newv)
>>> {
>>> __atomic_compare_exchange_n (p, &oldv, newv, 0, __ATOMIC_ACQUIRE |
>>> __ATOMIC_USE_HLE, __ATOMIC_ACQUIRE);
>>> retu
> In this case, can we reverse this sentence and just emit "lock
> xacquire" for MEMMODEL_ACQUIRE and "lock xrelease" for
> MEMMODEL_RELEASE ? Do we need separate HLE_* defines or can we somehow
> recycle existing C++11 memmodel defines?
No you absolutely can't. Transactions are quite different fr
Hello!
> > This is wrong since HLE ACQUIRE/RELEASE has nothing to do with
> > C++ atomic acquire/release. You can have HLE RELEASE with C++
> > atomic acquire.
>
> It makes sense to combine the two. On x86 C++ atomic acquire/release
> means the compiler cannot move references outside. For HLE
> we
On Tue, Apr 10, 2012 at 4:20 PM, Jakub Jelinek wrote:
> On Tue, Apr 10, 2012 at 06:12:08PM +0400, Kirill Yukhin wrote:
>> Attached patch implements HLE support for __atomic_compare_exchange_n.
>
> The target hook is definitely not appropriate, just define it in
> ix86_target_macros in i386-c.c ins
"H.J. Lu" writes:
>> So, to emit HLE prefix, it is possible to do:
>> int
>> foo2 (int *p, int oldv, int newv)
>> {
>> __atomic_compare_exchange_n (p, &oldv, newv, 0, __ATOMIC_ACQUIRE |
>> __ATOMIC_USE_HLE, __ATOMIC_ACQUIRE);
>> return oldv;
>> }
>
> This is wrong since HLE ACQUIRE/RELEASE has
On Tue, Apr 10, 2012 at 07:42:53AM -0700, H.J. Lu wrote:
> > Attached patch implements HLE support for __atomic_compare_exchange_n.
> >
> > So, to emit HLE prefix, it is possible to do:
> > int
> > foo2 (int *p, int oldv, int newv)
> > {
> > __atomic_compare_exchange_n (p, &oldv, newv, 0, __ATOMIC
On Tue, Apr 10, 2012 at 7:12 AM, Kirill Yukhin wrote:
>>
>> Yeah. And you don't need to change the FEs in any way, all that is needed
>> is to change the middle-end/expansion (builtins.c - e.g. get_memmodel)
>> and the backend (plus predefine the macros in the backend).
>>
>> Jakub
>
> Hi
On Tue, Apr 10, 2012 at 06:12:08PM +0400, Kirill Yukhin wrote:
> Attached patch implements HLE support for __atomic_compare_exchange_n.
The target hook is definitely not appropriate, just define it in
ix86_target_macros in i386-c.c instead or so.
Jakub
>
> Yeah. And you don't need to change the FEs in any way, all that is needed
> is to change the middle-end/expansion (builtins.c - e.g. get_memmodel)
> and the backend (plus predefine the macros in the backend).
>
>Jakub
Hi Jakub,
Attached patch implements HLE support for __atomic_compar
On Thu, Mar 08, 2012 at 07:04:03AM -0800, H.J. Lu wrote:
> > I don't think, we need to change FE for that...
>
> Please note that __ATOMIC_HLE_XACQUIRE has nothing to do with
> __ATOMIC_ACQUIRE. You can have
>
> __ATOMIC_ACQUIRE | __ATOMIC_HLE_XRELEASE
Yeah. And you don't need to change the FE
On Thu, Mar 8, 2012 at 12:44 AM, Kirill Yukhin wrote:
> Hi HJ,
> I am working on that. Here's some clarification from Jakub:
>> I meant that e.g. instead of:
>> int
>> foo (int *p, int oldv, int newv)
>> {
>> __atomic_compare_exchange_n (p, &oldv, newv, 0, __ATOMIC_ACQUIRE,
>> __ATOMIC_ACQUIRE);
On Wed, Mar 7, 2012 at 3:10 AM, Jakub Jelinek wrote:
> On Wed, Mar 07, 2012 at 03:05:58PM +0400, Kirill Yukhin wrote:
>> Hello guys,
>> I am attaching initial patch which enables TSX's HLE [1] prefixes in
>> GCC. Since we have no official intrinsics declarations, I want to hear
>> your comments ab
On Wed, Mar 07, 2012 at 03:05:58PM +0400, Kirill Yukhin wrote:
> Hello guys,
> I am attaching initial patch which enables TSX's HLE [1] prefixes in
> GCC. Since we have no official intrinsics declarations, I want to hear
> your comments about the patch
I think this is a wrong approach. Instead we
Hello guys,
I am attaching initial patch which enables TSX's HLE [1] prefixes in
GCC. Since we have no official intrinsics declarations, I want to hear
your comments about the patch
Note, there is no option '-mhle' and no tests (I'll do that after)
[1] -
http://software.intel.com/en-us/blogs/201
49 matches
Mail list logo
