[PATCH] Document HLE metadata

[Jeffrey Yasskin]

  You seem to have ignored most of my previous comments. Please try to avoid that.


================
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
----------------
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.

================
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
----------------
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.

================
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
----------------
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.

================
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
----------------
"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.

================
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.
----------------
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?

================
Comment at: docs/LangRef.rst:1323
@@ +1322,3 @@
+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*.
+
----------------
grammar: s/early/earlier/

================
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
----------------
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?

================
Comment at: docs/LangRef.rst:1298
@@ +1297,3 @@
+
+.. admonition:: Rationale
+
----------------
You've provided an overview here, not a rationale. A rationale would address why HLE hints are important, not just what they do.

================
Comment at: docs/LangRef.rst:1321
@@ +1320,3 @@
+execution started by a matching atomic instruction with an HLE *acquire* hint.
+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
----------------
s/global/globally/

Also, I think you mean that the changes are not globally visible, not that shared memory isn't globally visible.

================
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*.
----------------
spelling: s/sucessful/successful/

================
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*.
----------------
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.


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