[PATCH] D11044: [ImplicitNullChecks] Be smarter in picking the memory op.

[Sanjoy Das]

sanjoy added inline comments.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:212
@@ +211,3 @@
+
+  // Returns true if it is safe to reorder MI to before NotNullSucc.
+  auto IsSafeToReorder = [&](MachineInstr *MI) {
----------------
reames wrote:
> Naming wise, this is a bit confusing.  It's specifically whether the instruction is safe to reorder past the previously saved information.  At first, I had expected this predicate to by applied to each instruction skipped.
Makes sense, I'll try and come up with a better name tomorrow.

================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:255
@@ +254,3 @@
+
+    for (auto *MMO : MI->memoperands())
+      // Right now we don't want to worry about LLVM's memory model.
----------------
reames wrote:
> If one of these loads potentially overlap with the value we eventually load from, don't we end up with a faulting instruction not recorded in the side table?
> 
> (To say this differently, I think you need to account for aliasing in your loads.)
The faulting instruction is reordered to where the original branch was, so the control flow remains intact.  So if I start with

```
test reg, reg
jz throw_NPE
maybe_aliasing_load
candidate_load
```

I'll end up with

```
candidate_load ;; handler_pc = throw_NPE
maybe_aliasing_load
```

so `maybe_aliasing_load` won't execute if reg was null.

Does this answer your question?


http://reviews.llvm.org/D11044