[PATCH] [CodeGen] Introduce a FAULTING_LOAD_OP pseudo-op.

[Sanjoy Das]

================
Comment at: docs/FaultMaps.rst:20-26
@@ +19,9 @@
+
+For example, Java requires null checks on objects before they are read
+from or written to.  If the object is ``null`` then a
+``NullPointerException`` has to be thrown, interrupting normal
+execution.  In practice, however, dereferencing a ``null`` pointer is
+extremely rare in well-behaved Java programs, and typically the null
+check can be folded into a nearby memory operation that operates on
+the same memory location.
+
----------------
atrick wrote:
> Should this be an admonition?
I don't think I understand -- what should be an admonition?

================
Comment at: docs/FaultMaps.rst:52
@@ +51,3 @@
+      uint32  : FaultingPCOffset
+      uint32  : hnlderPCOffset
+    }
----------------
atrick wrote:
> "Handler"
Fixed.

================
Comment at: include/llvm/CodeGen/FaultMaps.h:2
@@ +1,3 @@
+//===------------------- FaultMaps.h - StackMaps ----------------*- C++ -*-===//
+
+//
----------------
pgavlin wrote:
> Extra blank line.
Fixed.

================
Comment at: include/llvm/CodeGen/FaultMaps.h:15
@@ +14,3 @@
+#include "llvm/ADT/DenseMap.h"
+
+#include <vector>
----------------
pgavlin wrote:
> Extra blank line.
This one was intentional, to visually separate the llvm/ includes from the stdlib includes. :)

================
Comment at: include/llvm/CodeGen/FaultMaps.h:54
@@ +53,3 @@
+
+  typedef std::vector<FaultInfo> FunctionFaultingInfo;
+  DenseMap<const MCSymbol *, FunctionFaultingInfo> FunctionInfos;
----------------
pgavlin wrote:
> Nit: s/FunctionFaultingInfo/FunctionFaultInfos/
Fixed.

================
Comment at: lib/CodeGen/FaultMaps.cpp:11
@@ +10,3 @@
+#include "llvm/CodeGen/FaultMaps.h"
+
+#include "llvm/CodeGen/AsmPrinter.h"
----------------
pgavlin wrote:
> Extra blank line.
That's intentional, to visually separate the header this file implements from the rest.

================
Comment at: lib/CodeGen/FaultMaps.cpp:75-86
@@ +74,14 @@
+
+  // We emit the functions sorted by name for easier testing.
+
+  std::vector<const MCSymbol *> FunctionNames;
+  FunctionNames.reserve(FunctionInfos.size());
+
+  for (const auto &FFI : FunctionInfos)
+    FunctionNames.push_back(FFI.first);
+
+  std::sort(FunctionNames.begin(), FunctionNames.end(),
+            [](const MCSymbol *L, const MCSymbol *R) {
+              return L->getName() < R->getName();
+            });
+
----------------
pgavlin wrote:
> What specifically about testing are you trying to make easier here? Are you just looking for determinism in the output order? If so, perhaps it would be better if `FunctionInfos` was a `std::map`, which should give you this property without the extra work.
> 
> FWIW, I think determinism here is useful beyond the scope of testing, so it would be nice to be able to provide it relatively cheaply.
> What specifically about testing are you trying to make easier here?

`FileCheck` based testing, I don't want tests to spuriously fail just because the order in which we emitted function infos changed.  I'll change the data structure to an `std::map` and remove the sorting step.

================
Comment at: lib/MC/MCObjectFileInfo.cpp:523
@@ -516,3 +522,3 @@
 
 void MCObjectFileInfo::InitCOFFMCObjectFileInfo(Triple T) {
   bool IsWoA = T.getArch() == Triple::arm || T.getArch() == Triple::thumb;
----------------
pgavlin wrote:
> Could you also add code to initialize `FaultMapSection` for COFF objects?
I don't know enough about COFF to confidently do that, and I don't have a stackmap example to copy from -- do you mind adding that bit of code once this change has landed?

http://reviews.llvm.org/D10197

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/