This is an archive of the discontinued LLVM Phabricator instance.

[MachineScheduler] Add support for clustering mem ops with FI base operands

Authored by thegameg on Nov 23 2018, 2:41 AM.



Before this patch, the following stores in merge_fail would fail to be merged, while they would get merged in merge_ok:

void use(unsigned long long *);
void merge_fail(unsigned key, unsigned index)
  unsigned long long args[8];
  args[0] = key;
  args[1] = index;
void merge_ok(unsigned long long *dst, unsigned a, unsigned b)
  dst[0] = a;
  dst[1] = b;

The reason is that getMemOpBaseImmOfs would return false for FI base operands.

This adds support for this.

Diff Detail


Event Timeline

thegameg created this revision.Nov 23 2018, 2:41 AM
niravd added inline comments.Nov 26 2018, 8:53 AM
1502 ↗(On Diff #175096)

nit: Remove the final else here

1159 ↗(On Diff #175096)

You need to do slightly more checking here as there may be operations that cross Frame index boundaries. SelectionDAGAddressAnalysis has a relevant example.

thegameg marked 2 inline comments as done.Nov 27 2018, 6:17 AM
thegameg added inline comments.
1159 ↗(On Diff #175096)

Please correct me if I'm wrong, but from what I see in SelectionDAGAddressAnalysis, it tries to match non-equal FIs. Are you suggesting that we should also support this here? Or that comparing the FIs is not enough?

niravd added inline comments.Nov 27 2018, 7:33 AM
1159 ↗(On Diff #175096)

Right. Comparing FIs is not enough because for accesses based on fixed object frames indices may operate on with memory in another fixed object frame index.

thegameg updated this revision to Diff 175517.Nov 27 2018, 10:09 AM
thegameg marked 2 inline comments as done.

Check for non equal FIs in shouldClusterMemOps. If they are both fixed objects, they can be used to access another slot, so check that the offsets match.

niravd accepted this revision.Nov 27 2018, 11:00 AM

LGTM modulo a small nit I missed on the last pass.

1500 ↗(On Diff #175517)

nit: Remove this else as well.

This revision is now accepted and ready to land.Nov 27 2018, 11:00 AM
This revision was automatically updated to reflect the committed changes.