[PATCH] D36392: [ImplicitNullCheck] Fix the bug when dependent instruction accesses memory

[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 minor comments



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:315
+    AliasResult AR = areMemoryOpsAliased(MI, PrevMI);
+    if (AR != AR_NoAlias)
+      return AR;
----------------
IIUC this isn't optimal -- we can early return with `AR_MayAlias` when processing further would have given us `AR_WillAliasEverything`.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:322
+ImplicitNullChecks::AliasResult
+ImplicitNullChecks::areMemoryOpsAliased(MachineInstr &MI,
                                         MachineInstr *PrevMI) {
----------------
Minor, but I'd be consistent here and either take both as refs or pointers.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:343
+      if (const PseudoSourceValue *PSV2 = MMO2->getPseudoValue()) {
+        if (PSV1) {
+          // Special case, both are pseudo values.
----------------
There are too many `continue`s and `return`s here -- do you mind splitting out a `mayAlias(MachineMemOperand *, MachineMemOperand *)` helper?


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:369
+      }
+      // Both MMO1 and MMO2 has value represented by LLVM IR.
       llvm::AliasResult AAResult = AA->alias(
----------------
s/has value/have values/


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:470
+  // Verify that DependenceMI does not alias with any instructions seen so far.
+  if (DependenceMI->mayLoadOrStore())
+    if (areMemoryOpsAliased(*DependenceMI, { InstsSeenSoFar.begin(),
----------------
I vaguely remember that originally we did not allow the dependence MI to touch memory.  Do you know when this changed (or am I misremembering)?


https://reviews.llvm.org/D36392