[rs6000] Add memory barriers to tbegin, tend, etc.

On Fri, 2015-10-09 at 16:41 +0200, Torvald Riegel wrote:
> On Thu, 2015-09-03 at 16:58 -0500, Peter Bergner wrote:
>> +Note that the semantics of the above HTM builtins are required to mimic the
>> +locking semantics used for critical sections.  Builtins that are used to
>> +create a new transaction or restart a suspended transaction (specifically any
>> +builtin that changes the state from non-transactional to transactional)
> 
> Those builtins could be nested in transactions yet would need to provide
> the same semantics at least as far as the compiler is concerned, so
> maybe change "specifically" to "for example"?

Ah, good point.  I forgot about the nested example.  In that case, how about
we just drop the text within the ()'s instead?  I'm not sure we need it
given your suggested addition below.



> I would be a bit more specific just to avoid confusion about how the
> locks are implemented or what specific semantics they have.  What about
> adding this or something similar:
> 
> "Specifically, this must mimic lock semantics as specified by C++11, for
> example: Lock acquisition is as-if an execution of
> __atomic_exchange_n(&globallock,1,__ATOMIC_ACQUIRE) that returns 0, and
> lock release is as-if an execution of
> __atomic_store(&globallock,0,__ATOMIC_RELEASE), with globallock being an
> implicit implementation-defined lock used for all transactions."

Looks good to me.


>> The HTM instructions associated with
>> +with the builtins inherently provide the correct acquisition and release
>> +semantics required.
> 
> I would say "hardware barriers" or "hardware fences" instead of
> "semantics" to clarify that this is just one part.

Agreed, I'll change it to use "hardware barriers".



>> However, the compiler must be prohibited from moving
>> +loads and stores across the HTM instructions.  This has been accomplished
>> +by adding memory barriers to the associated HTM instructions.
> 
> I think it would be good to make the requirement tighter, so that we do
> not promise too much.  What about changing this to:
> 
> "However, the compiler must also be prohibited from moving loads and
> stores across the builtins in a way that would violate their semantics.
> This has been accomplished by adding memory barriers to the associated
> HTM instructions (which is a conservative approach to provide acquire
> and release semantics)."

Done.


> I haven't thought enough about txn suspend/resume/abort to make a
> proposal how their required semantics would be specified best.  Maybe we
> should call this out explicitly, or maybe the sentences above are
> sufficient because they refer to HTM instructions generally. 

Well we do mention that we need acquire semantics not just for instructions
that start transactions, but also for instructions that resume transactions.
Similar for release and end/suspend, so I guess I'd lean towards what we have
is sufficient.


> Maybe the second "HTM instructions" should be "builtins" too, I don't
> know.

Well the builtins expand into multiple hardware instructions and the
memory barriers are only attached to the HTM instructions and not the
other instructions that are generated by the builtins, so I think
HTM instructions is probably more correct here.  But if you think
builtins is better, I can change it.



>> Earlier versions
>> +of the compiler did not treat the HTM instructions as memory barriers.
>> +A @code{__TM_FENCE__} macro has been added, which can be used to determine
>> +whether the current compiler treats HTM instructions as memory barriers or not.
> 
> "builtins" instead of "HTM instructions"?

For the same reason as above, I think HTM instructions is probably more
correct here too...but can be convinced otherwise. :-)


Thanks for the review!  I've attached the changes to the documentation below.
Is this better?


Peter

On Fri, 2015-10-09 at 11:52 -0500, Peter Bergner wrote:
> On Fri, 2015-10-09 at 16:41 +0200, Torvald Riegel wrote:
> > On Thu, 2015-09-03 at 16:58 -0500, Peter Bergner wrote:
> >> +Note that the semantics of the above HTM builtins are required to mimic the
> >> +locking semantics used for critical sections.  Builtins that are used to
> >> +create a new transaction or restart a suspended transaction (specifically any
> >> +builtin that changes the state from non-transactional to transactional)
> > 
> > Those builtins could be nested in transactions yet would need to provide
> > the same semantics at least as far as the compiler is concerned, so
> > maybe change "specifically" to "for example"?
> 
> Ah, good point.  I forgot about the nested example.  In that case, how about
> we just drop the text within the ()'s instead?  I'm not sure we need it
> given your suggested addition below.

Sounds good to me.

> > I haven't thought enough about txn suspend/resume/abort to make a
> > proposal how their required semantics would be specified best.  Maybe we
> > should call this out explicitly, or maybe the sentences above are
> > sufficient because they refer to HTM instructions generally. 
> 
> Well we do mention that we need acquire semantics not just for instructions
> that start transactions, but also for instructions that resume transactions.
> Similar for release and end/suspend, so I guess I'd lean towards what we have
> is sufficient.

OK.

> 
> > Maybe the second "HTM instructions" should be "builtins" too, I don't
> > know.
> 
> Well the builtins expand into multiple hardware instructions and the
> memory barriers are only attached to the HTM instructions and not the
> other instructions that are generated by the builtins, so I think
> HTM instructions is probably more correct here.  But if you think
> builtins is better, I can change it.

No, it's exactly this aspect of the implementation that I didn't know
about, and thus wasn't sure.  Sounds all good.


> Thanks for the review!  I've attached the changes to the documentation below.
> Is this better?

Yes, thanks!

On Fri, 2015-10-09 at 22:19 +0200, Torvald Riegel wrote:
> On Fri, 2015-10-09 at 11:52 -0500, Peter Bergner wrote:
>> Thanks for the review!  I've attached the changes to the documentation below.
>> Is this better?
> 
> Yes, thanks!

Great, thanks!  This is committed a revision 228827.  I'm just starting
the testing of the backports to the release branches and will commit it
there too, assuming everything is clean.

Peter