[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.

Comments inline.



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:302
 ImplicitNullChecks::SuitabilityResult
+ImplicitNullChecks::AliasMemoryOp(MachineInstr &MI, MachineInstr *PrevMI) {
+  // If it is not memory access, skip the check.
----------------
Can we name this as a verb, like `areMemoryOpsAliased`?


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:319
+      if (const PseudoSourceValue *PSV = MMO2->getPseudoValue())
+        return PSV->mayAlias(MFI) ? SR_Unsuitable : SR_Suitable;
+      // Not a Value, conservatively alias.
----------------
This clause is a bit odd -- without this clause, the nested loop returns `SR_Suitable` only if all pairs of mem operands are NoAlias.  But this clause returns `SR_Suitable` even if one of `PrevMI` mem operands is a `PseudoSourceValue` with `!PSV->mayAlias(MFI)`.  Why do we have this difference?  If we have to, I'd prefer checking for `PseudoSourceValue` in a loop only over `PrevMI->memoperands()` outside this nested loop.

Also: can one of `MI.memoperands()` be a `PseudoSourceValue`?  If not, please add an assert.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:328
+                         MMO2->getAAInfo()));
+      if(AAResult != NoAlias)
+        return SR_Unsuitable;
----------------
Spacing is off here, but I'll run clang-format on the diff before pushing it so don't bother fixing it right now.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:364
+      // If we already found that it aliases then no need to continue.
+      // But we continue base pointer check as it can result in SR_impossible.
+      if (Suitable == SR_Suitable) {
----------------
s/`SR_impossible`/`SR_Impossible`/


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:366
+      if (Suitable == SR_Suitable) {
+        Suitable = AliasMemoryOp(MI, PrevMI);
+        if (Suitable == SR_Impossible)
----------------
This layering seems odd to me -- `AliasMemoryOp` should not return a `SuitabilityResult` IMO, since it has no idea on why it has been asked to compute aliasing between the two instructions.  It almost feels like it should return a tri-stated `AliasResult` with `AR_NoAlias`, `AR_MayAlias` and `AR_WillAliasEverything`.  That would be a bit more code, but I think the intent will be cleaner.


https://reviews.llvm.org/D30331