[PATCH] D29400: [ImplicitNullCheck] Extend Implicit Null Check scope by using stores

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: docs/FaultMaps.rst:49
     uint32 : Reserved (expected to be 0)
     FunctionFaultInfo[NumFaultingPCs] {
+      uint32  : FaultKind
----------------
Please document the two possible values of this `FaultKind` field.


================
Comment at: include/llvm/CodeGen/FaultMaps.h:29
 public:
-  enum FaultKind { FaultingLoad = 1, FaultKindMax };
+  enum FaultKind { FaultingLoad = 1, FaultingStore, FaultKindMax };
 
----------------
I found the earlier classification system better, with a third `FaultingLoadStore` category.  Right now we don't have a place to put `addq (%rax), %rbx` type instructions, which both load and store.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:303
+  // First, if it is a store and we saw load before we bail out
+  // because we will no be able to re-order load-store without
+  // usage of memory aliasing analysis.
----------------
s/no be/not be/


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:304
+  // because we will no be able to re-order load-store without
+  // usage of memory aliasing analysis.
+  if (SeenLoad && MI.mayStore())
----------------
I'd s/usage of memory aliasing analysis/using alias analysis/


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:308
+
+  SeenLoad = SeenLoad || MI.mayLoad();
+
----------------
How about `SeenLoad |= MI.mayLoad()`?


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:310
+
+  // Without Memory Aliasing Analysis we cannot re-order store with anything.
+  // so if this instruction is not a candidate we should stop.
----------------
It is more idiomatic to say "alias analysis" instead of "Memory Aliasing Analysis".


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:312
+  // so if this instruction is not a candidate we should stop.
+  SuitabilityResult cont = MI.mayStore() ? SR_Impossible : SR_Unsuitable;
+
----------------
Please call this something more descriptive than `cont`.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:583
 
-    // Insert a faulting load where the conditional branch was originally.  We
+    // Insert a faulting instr where the conditional branch was originally. We
     // check earlier ensures that this bit of code motion is legal.  We do not
----------------
s/`instr`/`instruction`/


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:917
 
-  for (auto I = MI.operands_begin() + LoadOperandsBeginIdx,
-            E = MI.operands_end();
+  for (auto I = FaultingMI.operands_begin() + OperandsBeginIdx,
+            E = FaultingMI.operands_end();
----------------
Later on (in a separate change), perhaps we can rewrite this as:

```
for (auto &MO : make_range(FaultingMI.operands_begin() + OperandsBeginIdx, FaultingMI.operands_end())) {
  // Use MO instead of I
}
```



https://reviews.llvm.org/D29400