This is an archive of the discontinued LLVM Phabricator instance.

[RegAlloc] Trace sibling copies when checking for rematerializability during spill weight calculation (PR24139)
ClosedPublic

Authored by rob.lougher on Jul 31 2015, 11:49 AM.

Details

Summary

PR24139 ( https://llvm.org/bugs/show_bug.cgi?id=24139 ) contains an analysis of poor register allocation. One of the findings was that when calculating the spill weight, a rematerializable interval once split is no longer rematerializable. This is because the isRematerializable check in CalcSpillWeights.cpp does not follow the copies introduced by live range splitting (after splitting, the live interval register definition is a copy which is not rematerializable).

My initial fix used a hack which was sufficient for my experiments. This patch is a cleaned up version using a different approach. It, however, obtains the same results as my initial hack.

I have not included a test as I don't know how to test it! I could write a test that counted the number of spills but I feel that any such test would be too fragile in the long run. Advice welcome...

Diff Detail

Repository
rL LLVM

Event Timeline

rob.lougher retitled this revision from to [RegAlloc] Trace sibling copies when checking for rematerializability during spill weight calculation (PR24139).
rob.lougher updated this object.
rob.lougher added a reviewer: qcolombet.
rob.lougher added a subscriber: llvm-commits.
RKSimon added a subscriber: RKSimon.Aug 2 2015, 9:29 AM
qcolombet edited edge metadata.Aug 5 2015, 5:19 PM

Hi Rob,

I would have preferred that we use the LiveRangeEdit information to test whether or not the value is rematerializable. Indeed, we need to look through split only if we did split, i.e., we used LRE.

That being said, your fix is fine because the LRE rematerialization test is more general and it is likely that the spill cost resulting from it will be completely off.

Ultimately, I think it should be the spiller that should give the weight to spill a live range… but this is a clean-up for another day!

Anyway, thanks for catching and fixing it.

Could you add a test case using the related PR?

Cheers,
-Quentin

rob.lougher edited edge metadata.

Hi Quentin,

Thanks for the review. I have added a test based on the PR that checks we get the expected number of spills.

Thanks,
Rob.

qcolombet accepted this revision.Aug 7 2015, 3:43 PM
qcolombet edited edge metadata.

Thanks Rob!

Q.

This revision is now accepted and ready to land.Aug 7 2015, 3:43 PM
This revision was automatically updated to reflect the committed changes.