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

Andrew Trick atrick at apple.com
Fri Jun 12 18:47:49 PDT 2015


Awesome. Nice job splitting the patches.

A few comments inline..


================
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));
+
----------------
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.

================
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) {
----------------
Do we need to introduce a new term "WrappedLoad"? It initially confused me a bit. Why not just stick with FaultingLoad?

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:217-218
@@ +216,4 @@
+
+  for (auto &MO : LoadMI->operands())
+    if (!(MO.isReg() && MO.isDef()))
+      MIB.addOperand(MO);
----------------
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.


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

http://reviews.llvm.org/D10201

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






More information about the llvm-commits mailing list