[PATCH] [CodeGen] Add a pass to fold null checks into nearby memory operations.

[Sanjoy Das]

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:45-48
@@ +44,6 @@
+
+static cl::opt<unsigned> PageSize("imp-null-check-page-size",
+                                  cl::desc("The page size of the target in "
+                                           "bytes"),
+                                  cl::init(4096));
+
----------------
atrick wrote:
> Huh... There's no platform API for this? It's  probably fine since this pass will only be used by people who know what they're doing. I'm just surprised.
There is `Process::getPageSize` but we need the page size for the target, not the host.  For a JIT they'll be the same, but I'd rather not bake that assumption in.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:201-202
@@ +200,4 @@
+/// faults.  The FAULTING_LOAD_OP instruction is inserted at the end of MBB.
+MachineInstr *ImplicitNullChecks::insertWrappedLoad(MachineInstr *LoadMI,
+                                                    MachineBasicBlock *MBB,
+                                                    MCSymbol *HandlerLabel) {
----------------
atrick wrote:
> Do we need to introduce a new term "WrappedLoad"? It initially confused me a bit. Why not just stick with FaultingLoad?
Fixed.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:217-218
@@ +216,4 @@
+
+  for (auto &MO : LoadMI->operands())
+    if (!(MO.isReg() && MO.isDef()))
+      MIB.addOperand(MO);
----------------
atrick wrote:
> MachineDesc.getNumDefs() does not include any implicit defs that may have been tacked onto the end of the load. However, I think MO.isDef() will still return true for those loads so you'll end up dropping them. Rather than checking for Defs you could just iterate over MachineInst.uses(). Then you'll naturally skip the defs that you want to skip. Yes, it's crazy that MachineOperand.uses() also includes implicit defs.
> 
Fixed.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:227-228
@@ +226,4 @@
+/// Rewrite the null checks in WorkList into implicit null checks.
+void ImplicitNullChecks::rewriteNullChecks(
+    SmallVectorImpl<ImplicitNullChecks::NullCheck> &WorkList) {
+  DebugLoc DL;
----------------
atrick wrote:
> I think the name WorkList is misleading, because once you have analyzed all blocks, the list never grows. It's just a plain old list of null check candidates. To make it clear that the list is immutable, rewriteNullChecks can take ArrayRef<NullCheck>.
> 
Fixed.

http://reviews.llvm.org/D10201

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