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

[Ahmed Bougacha]

================
Comment at: include/llvm/CodeGen/Passes.h:553-554
@@ -552,1 +552,4 @@
 
+  // ImplicitNullChecks - This pass folds null pointer checks into nearby memory
+  // operations.
+  extern char &ImplicitNullChecksID;
----------------
triple slash?

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:46
@@ +45,3 @@
+static cl::opt<unsigned>
+PageSize("imp-null-check-page-size", cl::desc("the page size of the target in "
+                                              "bytes"),
----------------
"t -> "T

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:52
@@ +51,3 @@
+
+struct ImplicitNullChecks : public MachineFunctionPass {
+  static char ID;
----------------
Why not class?  For that matter, why not private for the non-override members?

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:72
@@ +71,3 @@
+
+    NullCheck() {
+      MemOperation = nullptr;
----------------
why not:

    NullCheck() : MemOperation(), ...

?

Also, CheckOperation is missing.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:115
@@ +114,3 @@
+
+  bool FoundWork = false;
+  SmallVector<NullCheck, 16> WorkList;
----------------
How about the more common name, "Changed"?  Also, this should be updated somewhere.  Instead, perhaps simply return !WorkList.empty(), or fold return true into the rewriteNullChecks block?

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:127-129
@@ +126,5 @@
+
+// Analyze MBB to check if its terminating branch can be turned into an implicit
+// null check.  If yes, append a description of the said null check to WorkList
+// and return true, else return false.
+bool ImplicitNullChecks::analyzeBlockForNullChecks(
----------------
JosephTremoulet wrote:
> Just curious: do you still intend to eventually have this check branch weights and only do the transformation when the null path is very cold?  And/or to add some other heuristics here?  Or is the plan now to be this aggressive here and rely entirely on patching to handle the cases where the null path is not very cold?
Why not triple-slash? (for the other methods as well)

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:139
@@ +138,3 @@
+  if (!(MBP.LHS.isReg() && MBP.RHS.isImm() && MBP.RHS.getImm() == 0 &&
+        (MBP.Predicate == CmpInst::ICMP_NE ||
+         MBP.Predicate == CmpInst::ICMP_EQ)))
----------------
This is what I feared with the AnalyzeBranchPredicate patch: I'm not comfortable with mentioning CmpInst here. A couple equally-icky alternatives:
- 'using CmpInst::Predicate' in MachineBranchPredicate
- duplicating it for the cases we're interested in, just NE/EQ ?

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:217-219
@@ +216,5 @@
+
+  auto MIB = BuildMI(MBB, DL, TII->get(TargetOpcode::FAULTING_LOAD_OP), DefReg);
+  MIB.addSym(HandlerLabel);
+  MIB.addImm(LoadMI->getOpcode());
+
----------------
Why not chaining BuildMI().addSym().addImm?

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:222
@@ +221,3 @@
+  for (auto &MO : LoadMI->operands())
+    if (!(MO.isReg() && MO.isDef()))
+      MIB.addOperand(MO);
----------------
Except for the first operand, does this ever happen?
If not, can you change the for() to ignore it instead, and turn this into an assert on LoadMI->getOperand(0) ?

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:253
@@ +252,3 @@
+    TII->InsertBranch(*NC.CheckBlock, NC.NotNullSucc, nullptr,
+                      SmallVector<MachineOperand, 0>(), DL);
+
----------------
I don't like this SmallVector<>() construct, but I don't have a better idea ;)

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:265
@@ +264,3 @@
+                      "Implicit null checks", false, false)
+INITIALIZE_AG_DEPENDENCY(AliasAnalysis)
+INITIALIZE_PASS_END(ImplicitNullChecks, "implicit-null-checks",
----------------
Is this actually used?

================
Comment at: lib/CodeGen/Passes.cpp:85-88
@@ -84,2 +84,6 @@
     cl::ZeroOrMore);
+static cl::opt<bool> EnableImplicitNullChecks(
+  "enable-implicit-null-checks",
+  cl::desc("Fold null checks into faulting memory operations"),
+  cl::init(false));
 
----------------
Can you move this elsewhere, say before PrintLSR?  I feel like it doesn't belong here.

http://reviews.llvm.org/D10201

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