This addresses a FIXME in MachineInstr::mayAlias.
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 think the more interesting checks for PseudoValues are whether they can alias at all (i.e. PseudoSourceValue::isAliased/mayAlias), and if not they can only alias with themselves.
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:
This patch is performance neutral on Falkor/Kryo. This addresses a FIXME that I saw in the passing and attempted to fix as it saves compile time and catch corner cases that are currently not covered.
The changes to the AMDGPU tests need more explanation.
These lit tests were failing due to instruction scheduling changes, so I modified the tests to reflect the new code generation and made the AMDGPU tests more robust.
getValue() will not be null here as we check for getValue() is null and return early at line 1638
if (!MMOa->getValue() || !MMOb->getValue()) return true;
This is what I was trying to express in my previous comment.
This change is enhancing the alias checking in three different scenarios, one of which is fairly subtle:
- adding checking for non-Value MMOs
- adding simple overlap checking for two equal Value MMOs when no AA is available
- refining simple overlap checking for two equal Value MMOs when AA is available
Item 3 is the subtle one. The reason this is improving on the regular AA results is because of the way we're constructing the AA query for MMOs with a non-zero offset. For example, if we're asking about MMOa x[0:3] and MMOb x[4:7], we construct an AA query that instead asks about x[0:3] and x[0:7]. I assume this was done intentionally to avoid having to construct a new GEP for the non-zero offset case, but I don't know that for sure.
It seems like it might be reasonable to split off Item 1 from above into a separate patch and expand the cases it handles. Specifically, getting it to handle stack references on AArch64 when some are SP based and some are FP based would be interesting. You should be able to use the PseudoSourceValue::isAliased() to catch this. You should also be able to write MIR tests that confirm that this allows e.g. more load/store pairing on AArch64.
I also believe that this is true.
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.