[PATCH] [CodeGen] Introduce a FAULTING_LOAD_OP pseudo-op.
Sanjoy Das
sanjoy at playingwithpointers.com
Tue Jun 9 18:37:18 PDT 2015
================
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/
More information about the llvm-commits
mailing list