[PATCH] D34181: MachineInstr: Reason locally about some memory objects before going to AA.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lib/CodeGen/MachineInstr.cpp:1664
+  int64_t WidthB = MMOb->getSize();
+  bool SameVal = MMOa->getValue() == MMOb->getValue();
+  if (!SameVal) {
----------------
gberry wrote:
> bmakam wrote:
> > gberry wrote:
> > > I don't think this is right since getValue() will return nullptr for any PseudoSourceValue, so SameVal will always be true for any two PseudoSourceValues, even if they are different.
> > I am not sure I fully understand your comment, getValue will not return nullptr here because of the earlier condition that returns true (i.e. mayAlias) if it was nullptr.
> I missed that check before.  Given that check is there, the code below is useless since an MMO can only have a Value or a PseudoValue, so it will never change SameVal.
> 
> I think there are three interesting cases (at least) here:
> - pseudo vs. pseudo: i believe these can only alias if they are equal (and even then an overlap check can find cases that don't alias)
> - pseudo vs. value: can never alias if pseudo is not mayAlias (perhaps a stronger check is possible)
> - value vs. value: do overlap checking if no AA is present, otherwise just let AA do it  
> I think there are three interesting cases (at least) here:
> 
>  - pseudo vs. pseudo: i believe these can only alias if they are equal (and even then an overlap check can find cases that don't alias)

I also believe that this is true.

>  * pseudo vs. value: can never alias if pseudo is not mayAlias (perhaps a stronger check is possible)

Yes, although I think you need to check `isAliased`, not `mayAlias`. What you care about is that the access to the PseudoSourceValue is accessing something that no IR value can point to (which is what isAliased checks) because you're checking against an IR pointer value.

I'd certainly like to see this as a follow-up patch.


https://reviews.llvm.org/D34181