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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov marked 5 inline comments as done.
skatkov added inline comments.


================
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.
----------------
sanjoy wrote:
> 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.
Good catch, fixed.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:366
+      if (Suitable == SR_Suitable) {
+        Suitable = AliasMemoryOp(MI, PrevMI);
+        if (Suitable == SR_Impossible)
----------------
sanjoy wrote:
> 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.
Sounds reasonable to me.


https://reviews.llvm.org/D30331