Page MenuHomePhabricator

[GlobalISel][AArch64] Contract trivial same-size cross-bank copies into G_STOREs

Authored by paquette on Jul 19 2019, 3:29 PM.



Sometimes, you can end up with cross-bank copies between same-sized GPRs and FPRs, which feed into G_STOREs. When these copies feed only into stores, they aren't necessary; we can just store using the original register bank.

This provides some minor code size savings for some floating point SPEC benchmarks. (Around 0.2% for 453.povray and 450.soplex)

This issue doesn't seem to show up due to regbankselect or anything similar. So, this patch introduces an early select function, contractCrossBankCopyIntoStore which performs the contraction when possible. The selector then continues normally and selects the correct store opcode, eliminating needless copies along the way.

Diff Detail


Event Timeline

paquette created this revision.Jul 19 2019, 3:29 PM
aemerson added inline comments.Jul 19 2019, 3:42 PM
1170 ↗(On Diff #210915)

You mention 32 bits here but the example uses 64 bit types. Also, it doesn't matter whether we use 32 or 64 bit stores right?

1184 ↗(On Diff #210915)

This pattern can now be replaced with getOpcodeDef()?

1276 ↗(On Diff #210915)

You're returning false here because we want selection to continue, even though you changed the instruction. This is what the preISelLower() hook I added is for.

paquette marked 3 inline comments as done.Jul 19 2019, 3:54 PM
paquette added inline comments.
1170 ↗(On Diff #210915)

Right, it doesn't matter. I'll update the comment for consistency though.

1184 ↗(On Diff #210915)

It's not quite equivalent, since it looks through copies. I think that it would still work though, if I rejigger the rest of the function a bit.

1276 ↗(On Diff #210915)


paquette marked an inline comment as done.Jul 19 2019, 3:55 PM
paquette added inline comments.
1184 ↗(On Diff #210915)

Oh, wait, it looks for a specific opcode. I don't think that will work too well with copies.

aemerson accepted this revision.Jul 19 2019, 4:15 PM

Pre-emptive LGTM with the minor changes done.

1170 ↗(On Diff #210915)

Maybe replace the "storing 32 bits" with something like "storing a scalar".

1184 ↗(On Diff #210915)

You're right, it specifically skips copies so it's no good here.

This revision is now accepted and ready to land.Jul 19 2019, 4:15 PM
paquette updated this revision to Diff 210931.Jul 19 2019, 4:28 PM

Address review comments.

I switched the copy check over to walking through copies to find the def.

This is equivalent, but does prevent us from checking if all users are stores. This doesn't seem to cause any code size issues though.

paquette updated this revision to Diff 210932.Jul 19 2019, 4:29 PM

remove unused IR function from test

This revision was automatically updated to reflect the committed changes.
scw added a subscriber: scw.Feb 4 2020, 2:55 PM points to contractCrossBankCopyIntoStore() not handling G_UNMERGE_VALUES correctly.