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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

Will prepapre an update.



================
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.
----------------
sanjoy wrote:
> s/alias/aliases/
ok


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


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


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


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:351
+        // Alias analysis is not available, may alias.
+        if (!AA) {
+            Suitable = SR_Unsuitable;
----------------
sanjoy wrote:
> 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.
we can be in a good shape even if AA is not available. Say if other instruction is a spill. See the previous check: PSV != null and PSV->mayAlias is false. So it is not good to move AA check earlier. WRT AA is not null I will double check your idea.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:358
+                MMO1->getAAInfo()),
+            MemoryLocation(MMO2->getValue(), MemoryLocation::UnknownSize,
+                MMO2->getAAInfo()));
----------------
sanjoy wrote:
> What about multiple memoperands on the same machine instruction?
Not sure it is possible but we can iterate over mem operands.


https://reviews.llvm.org/D30331