[PATCH] Document HLE metadata

[Jeffrey Yasskin]

================
Comment at: docs/LangRef.rst:1298
@@ +1297,3 @@
+
+.. admonition:: Rationale
+
----------------
darkbuck wrote:
> Jeffrey Yasskin wrote:
> > You've provided an overview here, not a rationale. A rationale would address why HLE hints are important, not just what they do.
> I am little confused here. The importantness of HLE hints is that we could leverage hardware HLE support by simply hinting atomic insn. Is that the rationale to introduce HLE hint instead introduce bunch of new atomic primitives?
Both that (why do enough users want to use these instructions that it's worth providing IR support? Why is metadata appropriate rather than a large set of x86-specific intrinsics?) and the general question, "Why do users want to leverage hardware support for this?"

Also, I think rationales belong at the end of their section, not the beginning. See http://llvm.org/docs/LangRef.html#volatile.

================
Comment at: docs/LangRef.rst:1301
@@ +1300,3 @@
+  As lock primitives are built upon atomic instructions, HLE hints attached on
+  atomic instructions allow programmers to convert a locked-based critical
+  section into a transaction region to take advantage of hardware transactional
----------------
darkbuck wrote:
> Jeffrey Yasskin wrote:
> > s/locked-based/lock-based/
> > 
> > Do you happen to have a native English speaker you could get to proofread your LangRef patch before you send out iterations? I don't mind correcting grammar when the meaning's obvious, but there are a couple places I'm having to guess at your meaning, and this review might go faster if I didn't have to.
> as non-native speaker, I will try best, :)
Sorry, I was suggesting that having a second person at Intel go over the patch might help.

================
Comment at: docs/LangRef.rst:1309
@@ +1308,3 @@
+Atomic instructions (:ref:`cmpxchg <i_cmpxchg>`, :ref:`atomicrmw <i_atomicrmw>`,
+and :ref:`atomic store <i_store>`) may take an optional hint of Hardware Lock
+Elision (Speculative Lock Elision or Transactional Lock Elision) to start an
----------------
darkbuck wrote:
> Jeffrey Yasskin wrote:
> > "an optional hint of Hardware Lock Elision" still uses HLE as a magic word. I want you to write this section at an audience who hasn't worked at Intel, which means they don't already know what HLE means.
> I did list non-Intel but mostly equivalent terms like SLE (speculative lock elison), TLE (transactional lock elision). If you want to put an introduction of them, we'd better provide a reference link.
A reference link might help, but even with that, it likely won't be enough to say that this metadata "does lock elision". You need to say what it does in more precise terms than that.

================
Comment at: docs/LangRef.rst:1311
@@ +1310,3 @@
+Elision (Speculative Lock Elision or Transactional Lock Elision) to start an
+HLEd critical section by hinting an atomic instruction with HLE *acquire* hint
+and to end an end an HLEd critical section by hinting an atomic instruction
----------------
darkbuck wrote:
> Jeffrey Yasskin wrote:
> > Nit: Here I would probably write out the metadata IR representation. That is, "by hinting an atomic instruction with !hle.lock !{metadata !"acquire"}". That avoids saying "hint" twice.
> ok, let's put them like "by annotating .. with ... hint" to avoid put HLE hint details here.
Ok.

================
Comment at: docs/LangRef.rst:1315
@@ +1314,3 @@
+
+With an HLE *acquire* hint, an atomic instruction is executed speculatively on
+the specified atomic object. It also start the transactional execution until
----------------
darkbuck wrote:
> Jeffrey Yasskin wrote:
> > What does it mean to execute an atomic instruction speculatively?
> > 
> > I suspect HLE actually *omits* the atomic instruction speculatively, on the assumption that it will be set back to its original value before any other thread could see the change. Please be precise.
> sure, speculation here just means the write to that atomic object is omitted, or other behaviour is still the same, e.g. the current value of atomic object will be returned.
Please say that, and don't just hand-wave with the word "speculatively".

================
Comment at: docs/LangRef.rst:1316
@@ +1315,3 @@
+With an HLE *acquire* hint, an atomic instruction is executed speculatively on
+the specified atomic object. It also start the transactional execution until
+a matching atomic instruction with an HLE *release* hint.
----------------
darkbuck wrote:
> Jeffrey Yasskin wrote:
> > I think you mean "It also starts transactional execution, which continues until a matching atomic instruction with an HLE *release* hint."
> > 
> > However, I believe this sentence is incorrect: the transaction may stop before the instruction with the release hint (a syscall or IO operation would do it, beyond the obvious case of cache line invalidation, right?). Please describe the cases where the transaction might abort, or describe the meaning of the acquire and release hints without referring to an extra "transactional" state.
> > 
> > Also, what does "matching" mean?
> the transactional execution (vs. normal execution) itself means the code may roll back due to abort.
> 
> Do people not know "matching" if they use lock to synchronize threads?
An atomic instruction isn't a lock, so there's no automatic extension of the meaning of "matching". You have to write down how the arguments to a pair of atomic instructions cause them to match. You could say that the pointer argument to an annotated atomic refers to a lock, after which lock-related terminology would be usable, but I'd rather a description just in terms of the instructions, because then you don't have to explain which kind of lock.

================
Comment at: docs/LangRef.rst:1322
@@ +1321,3 @@
+It tries to commit the changes to shared memory, which is not global visible
+before a sucessful commit. If the commit fails, the execution will be restarted
+from the point where an early atomic instruction hinted with HLE *acquire*.
----------------
darkbuck wrote:
> Jeffrey Yasskin wrote:
> > Jeffrey Yasskin wrote:
> > > spelling: s/sucessful/successful/
> > Why would a commit fail? For example, in [Intel® Architecture Instruction Set Extensions Programming Reference] 8.3.3, it looks like if the release atomic sets the variable to a value other than what it had before the acquire, the commit might fail.
> do you mean the commit failing at the point of *release*? 8.3.3 only gives one possible reason (specific to x86 HLE impl). there're other reasons (on x86 or non-x86 targets) that a transaction may fails, e.g. lack of resources.
So say that.

================
Comment at: docs/LangRef.rst:1325
@@ +1324,3 @@
+
+It's expected that HLE hints are paired similar to lock/unlock usage in thread
+synchronization. However, improper use of HLE hints will not cause functional
----------------
darkbuck wrote:
> Jeffrey Yasskin wrote:
> > grammar: s/similar/similarly/
> > 
> > What does "similar" actually mean here? Can the hints be paired like a recursive mutex? What about like a reader/writer mutex?
> it could be nested but not recursive (personally, recurisve mutex is evil and encourags bad concurrency practice). read/writer lock is OK to be hinted. the only tricky case I am aware of is the ticket lock.
So say that.


http://llvm-reviews.chandlerc.com/D475