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

[Sanjoy Das]

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:100-102
@@ +99,5 @@
+
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    MachineFunctionPass::getAnalysisUsage(AU);
+  }
+
----------------
ab wrote:
> Is this necessary?
Nope, thanks for catching!

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:134
@@ +133,3 @@
+
+  typedef TargetInstrInfo::MachineBranchPredicate MachineBranchPredicate;
+
----------------
ab wrote:
> How about 'using TargetInstrInfo::MachineBranchPredicate;'?
That does not seem to compile.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:192-195
@@ +191,6 @@
+    if (MemOp->mayLoad() && BaseReg == PointerReg && Offset < PageSize) {
+      // FIXME: we use MachineInstr::mayLoad which isn't quite correct for
+      // predicated load instructions that don't fault when the predicate is
+      // false.  We should add a instructionAlwaysLoads() hook to
+      // TargetInstrInfo and use that instead of mayLoad.
+      WorkList.emplace_back(MemOp, MBP.ConditionDef, &MBB, NotNullSucc,
----------------
ab wrote:
> Would (MayLoad && !Predicable) be (conservatively) correct?
Should be, unless there are other ways `MayLoad` instructions can avoid actually doing the load.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:213
@@ +212,3 @@
+  unsigned NumDefs = LoadMI->getDesc().getNumDefs();
+  assert(NumDefs == 1 && "other cases unhandled!");
+
----------------
ab wrote:
> I'm not sure an assert is appropriate: I don't have an example but this sounds easy to trigger.  Should this be checked before even adding the NullCheck to the worklist?
Kept the assert for documentation, but added a check before adding to worklist.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:255
@@ +254,3 @@
+    TII->InsertBranch(*NC.CheckBlock, NC.NotNullSucc, nullptr,
+                      SmallVector<MachineOperand, 0>(), DL);
+
----------------
ab wrote:
> With r239553, you should be able to replace this with the IMO much more readable:
> 
>     /*Cond=*/None, DL);
Done.

http://reviews.llvm.org/D10201

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