[PATCH] D29119: [ImplicitNullCheck] NFC isSuitableMemoryOp cleanup

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with very minor nits

I'll submit this patch on your behalf once you upload a new version with the nits fixed here on phabricator



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:161
 
+  enum SuitableResult {
+    SR_OK,
----------------
I'd call this enum `SuitabilityResult` instead.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:162
+  enum SuitableResult {
+    SR_OK,
+    SR_Continue,
----------------
I'd call these `SR_Suitable`, `SR_Unsuitable`, and `SR_Impossible` (especially since `SR_Continue` is somewhat ambiguous).


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:168
   /// Is \p MI a memory operation that can be used to implicitly null check the
   /// value in \p PointerReg?  \p PrevInsts is the set of instruction seen since
   /// the explicit null check on \p PointerReg.
----------------
This comment needs updated since we're no longer returning a boolean.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:308
   // loading from PointerReg, and there isn't some re-definition of PointerReg
   // between the compare and the load.
   for (auto *PrevMI : PrevInsts)
----------------
Add a short line here on why you're returning `SR_Impossible`.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:490
     MachineInstr *Dependence;
-    if (isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar) &&
+    SuitableResult SRResult = isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar);
+    if (SRResult == SR_Impossible)
----------------
`SRResult` does not expand out correctly (it sounds like you're trying to say "suitability result result").

How about the more llvm idiomatic `SuitableResult SR = ..`?


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:493
+      return false;
+    if ((SRResult == SR_OK) &&
         canHoistLoadInst(&MI, PointerReg, InstsSeenSoFar, NullSucc,
----------------
The braces around `SRResult == SR_OK` is unnecessary.


https://reviews.llvm.org/D29119