Page MenuHomePhabricator

MachineInstr: Reason locally about some memory objects before going to AA.
ClosedPublic

Authored by bmakam on Jun 13 2017, 3:44 PM.

Details

Summary

This addresses a FIXME in MachineInstr::mayAlias.

Diff Detail

Event Timeline

bmakam created this revision.Jun 13 2017, 3:44 PM
gberry added a subscriber: gberry.Jun 14 2017, 10:42 AM
gberry added inline comments.
lib/CodeGen/MachineInstr.cpp
1662

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.

1665

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.

bmakam added inline comments.Jun 14 2017, 3:33 PM
lib/CodeGen/MachineInstr.cpp
1662

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.

gberry added inline comments.Jun 15 2017, 8:36 AM
lib/CodeGen/MachineInstr.cpp
1662

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
bmakam updated this revision to Diff 102832.Jun 16 2017, 8:51 AM
bmakam edited reviewers, added: efriedma; removed: eli.friedman.

Address Geoff's comments.

efriedma edited edge metadata.

Any idea what effect this has on performance, if any?

The changes to the AMDGPU tests need more explanation.

lib/CodeGen/MachineInstr.cpp
1664

This looks weird. If getPseudoValue() is non-null, getValue() is null; therefore PSVa is always null here.

Any idea what effect this has on performance, if any?

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.

lib/CodeGen/MachineInstr.cpp
1664

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;
efriedma added inline comments.Jun 16 2017, 1:54 PM
lib/CodeGen/MachineInstr.cpp
1664

What I meant to say is "If getValue() is non-null, getPseudoValue() is null, therefore PSVa is always null here."

gberry added inline comments.Jun 16 2017, 1:58 PM
lib/CodeGen/MachineInstr.cpp
1664

This is what I was trying to express in my previous comment.
I also think these Pseduo value checks could be expanded to handle cases other than isConstant by checking the mayAlias/isAliased as I outlined above.

I see the problem now, a part of the patch that addressed Geoff's comments earlier was missing. I will update the correct patch for review shortly.

bmakam updated this revision to Diff 102888.Jun 16 2017, 3:14 PM

Uploading the correct patch. I was being conservative by checking for isConstant only, I can add more cases if you think they are more interesting.

arsenm added inline comments.Jun 20 2017, 11:45 AM
test/CodeGen/AMDGPU/load-global-i8.ll
370

R600 checks often break because the annoying clause marker syntax, so I'm not concerned about these

This change is enhancing the alias checking in three different scenarios, one of which is fairly subtle:

  1. adding checking for non-Value MMOs
  2. adding simple overlap checking for two equal Value MMOs when no AA is available
  3. 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.

hfinkel accepted this revision.Aug 5 2017, 3:06 PM

LGTM

lib/CodeGen/MachineInstr.cpp
1662

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.

This revision is now accepted and ready to land.Aug 5 2017, 3:06 PM
bmakam closed this revision.Aug 14 2017, 2:46 AM

Thanks for taking a look. Rebased and committed in r310825.