This is an archive of the discontinued LLVM Phabricator instance.

Reorder stack up-adjustment and register copies
ClosedPublic

Authored by Baltoli on Feb 25 2023, 2:34 PM.

Details

Summary

Context

This diff reorders the stack up-adjustment and return value copying phases of
machine-ir generation on Aarch64. Doing so prevents a bug observed for fastcc
calls with >8 arguments, where the up-adjustment required from making that call
is placed in the wrong place relative to spill and reloading code.

See: https://github.com/llvm/llvm-project/issues/60972 for full issue reproduction and context.

Changes

The code change is small and consists only of moving one stanza of code-generation to occur earlier than another; no change is made to the substance of that code. Doing so entails a change to several GISel tests for AArch64; I verified by hand that all of these were simple reorderings and that no changes to instruction bodies were required to get the tests to pass.

 Review

This is my first patch to LLVM of any kind and I'm sure that I will probably have missed something in the process. Some questions I have:

  • Where is best to add a regression test for the issue I linked to above?
  • Are there other related places I should have changed similar code, that my test didn't trigger?

If the approach or changes made in this PR are manifestly wrong, please feel free to reject them quickly! Thanks for your time spent addressing this diff.

Diff Detail

Event Timeline

Baltoli created this revision.Feb 25 2023, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2023, 2:34 PM
Baltoli requested review of this revision.Feb 25 2023, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2023, 2:34 PM
Baltoli edited the summary of this revision. (Show Details)Feb 25 2023, 2:35 PM
Baltoli edited the summary of this revision. (Show Details)
Baltoli edited the summary of this revision. (Show Details)Feb 25 2023, 2:40 PM
Baltoli added reviewers: kristof.beyls, hiraditya.

The failing unit tests here don't look (from my naive perspective) like they relate to the changes I've made, but I'm happy to be corrected if they are.

Thanks for the fix. Could you also add a test specific to the bug you're seeing as well? The test case from the GitHub issue is fine.

Baltoli updated this revision to Diff 500702.Feb 27 2023, 1:33 AM

Add regression test to original changes

Baltoli added a comment.EditedFeb 27 2023, 1:36 AM

@aemerson I've pulled in the original code from my Github issue and generated a test case from it using the LLC test update script; please let me know if I should be doing anything differently. I have inspected the new test case and verified that the generated code is as expected (i.e. the stack-up adjustment follows the call, and does not get between the spill and reload).

I have also tested this on the (maximal) original example identified in our language, and the patch allows the relevant code to execute successfully. Our interpreter makes extensive use of fastcc / tail called code which further reassures me that these codegen changes are OK.

Thanks for your review!

aemerson accepted this revision.Feb 27 2023, 9:53 AM

Thanks, LGTM.

This revision is now accepted and ready to land.Feb 27 2023, 9:53 AM

Thanks very much @aemerson - I don't have commit access so someone will need to do that for me.