This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][WIP][RFC] Fix stack slot for argument type sizes not a multiple of 64 bits (Bug 49500)
AbandonedPublic

Authored by luismarques on Mar 22 2021, 4:06 AM.

Details

Summary

This is a fix for: https://bugs.llvm.org/show_bug.cgi?id=49500.

As it stands, this patch is basically just a copy of D97514. Originally I suspected that there might be a better way to solve it, but eventually I realized I had arrived at roughly the same approach. I'm not sure there's much point in using getRegisterTypeForCallingConv and getNumRegistersForCallingConv versus something more direct, but I've left that unchanged.

Diff Detail

Event Timeline

luismarques created this revision.Mar 22 2021, 4:06 AM
luismarques requested review of this revision.Mar 22 2021, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2021, 4:06 AM
luismarques edited reviewers, added: mundaym; removed: Michael.Mar 22 2021, 4:06 AM
luismarques edited the summary of this revision. (Show Details)Mar 22 2021, 4:09 AM
luismarques added inline comments.Mar 22 2021, 4:18 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6196–6197

Is OrigArgVT always going to be equal to Outs[i].ArgVT?

Does this happen to fix D95025? It's certainly in a similar area as my (WIP) fix that I never got round to publishing.

Does this happen to fix D95025? It's certainly in a similar area as my (WIP) fix that I never got round to publishing.

At first glance it looks like it does.

At first glance it looks like it does.

Excellent!

My fix was more direct: I went over ArgValue and all of the SDValue PartValue = OutVals[i + 1]; as in the loop later in this block, and summed up all of the required getStoreSizes of their VTs, and took the maximum required alignment of each. Then created a stack temporary of that summed size and max alignment. I wasn't sufficiently happy with it and since I didn't get much indication that this was an urgent fix I didn't put it up for discussion - sorry about that. I can share my patch with you if you're interested.

My fix was more direct: I went over ArgValue and all of the SDValue PartValue = OutVals[i + 1]; as in the loop later in this block, and summed up all of the required getStoreSizes of their VTs, and took the maximum required alignment of each. Then created a stack temporary of that summed size and max alignment. I wasn't sufficiently happy with it and since I didn't get much indication that this was an urgent fix I didn't put it up for discussion - sorry about that. I can share my patch with you if you're interested.

That sounds like it has the potential for being a better alternative than this one. Please post it for review. Thanks!

luismarques updated this revision to Diff 332295.EditedMar 22 2021, 8:24 AM
  • Precommitted the original test, to better show the difference caused by the patch.
  • Updated the existing tests. They seem to more broadly show the issue I noted about the excessive alignment/overly conservative stack size.
luismarques abandoned this revision.Apr 4 2021, 8:33 AM

Abandoned in favor of D99087. Thanks @frasercrmck!

luismarques edited the summary of this revision. (Show Details)

Fixing alignment before truly abandoning, in case it's useful for D97514.