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

Sanjoy Das via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 7 09:36:45 PDT 2017

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::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)?


More information about the llvm-commits mailing list