[PATCH] D30331: [ImplicitNullCheck] Add alias analysis usage

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Some comments inline.



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:323
 
-  return SR_Suitable;
+    // Check whether the current memory access alias with previous one.
+    // If we already found that it aliases then no need to continue.
----------------
s/alias/aliases/


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:329
+    // If it is not memory access, skip the check.
+    if (!(PrevMI->mayStore() || PrevMI->mayLoad()))
+        continue;
----------------
Can we pull the body of this loop out into a static helper?


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:335
+        if (MI.memoperands_empty() || PrevMI->memoperands_empty()) {
+            Suitable = SR_Unsuitable;
+            continue;
----------------
Can we return `SR_Impossible` here if the `MachineInstr` that was the store also has no mem operands?


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:342
+        if (!MMO1->getValue()) {
+            Suitable = SR_Unsuitable;
+            continue;
----------------
The indents are off -- please use clang-format.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:351
+        // Alias analysis is not available, may alias.
+        if (!AA) {
+            Suitable = SR_Unsuitable;
----------------
Maybe move this check up to avoid wasting work if `AA` isn't available?

Btw, given the way you're getting `AA`, I'd expect it always be present since otherwise `getAnalysis<AAResultsWrapperPass>().getAAResults()` will have to be a null reference which is undefined behavior.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:358
+                MMO1->getAAInfo()),
+            MemoryLocation(MMO2->getValue(), MemoryLocation::UnknownSize,
+                MMO2->getAAInfo()));
----------------
What about multiple memoperands on the same machine instruction?


https://reviews.llvm.org/D30331