This is an archive of the discontinued LLVM Phabricator instance.

[ARM] GlobalISel: Lower more than 4 arguments
ClosedPublic

Authored by rovka on Nov 29 2016, 2:19 AM.

Details

Summary

This adds support for lowering more than 4 arguments (although still i32 only).
It uses the handleAssignments / ValueHandler infrastructure extracted from
the AArch64 backend in D27045.

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 79523.Nov 29 2016, 2:19 AM
rovka retitled this revision from to [ARM] GlobalISel: Lower more than 4 arguments.
rovka updated this object.
rovka added reviewers: t.p.northover, qcolombet, ab.
rovka added subscribers: llvm-commits, rengolin.
t.p.northover added inline comments.Nov 30 2016, 9:59 AM
lib/Target/ARM/ARMCallLowering.cpp
51 ↗(On Diff #79523)

This is silently ignoring the signext/zeroext modifiers that are needed to correctly handle small types on AAPCS. It'd probably be better to fallback or assert if they're not being supported yet.

lib/Target/ARM/ARMInstructionSelector.cpp
84–86 ↗(On Diff #79523)

Is this selection stuff intentionally in the same patch? Seems like it's an orthogonal issue.

qcolombet added inline comments.Nov 30 2016, 12:48 PM
lib/Target/ARM/ARMCallLowering.cpp
51 ↗(On Diff #79523)

I would recommend to change the ValueHandler APIs to return boolean so we can indeed fallback while stuff is not supported.

rovka added inline comments.Dec 5 2016, 3:24 AM
lib/Target/ARM/ARMCallLowering.cpp
51 ↗(On Diff #79523)

Right now we're falling back before reaching this point, by returning false from ARMCallLowering::lowerReturn if the type is not strictly 32-bit wide (see line 86). I think assertions would be more appropriate in this case, because it's not too complicated to check all the value types up front before calling this. I'll add some throughout.

lib/Target/ARM/ARMInstructionSelector.cpp
84–86 ↗(On Diff #79523)

It's intentional, I wanted to have end-to-end support for more than 4 arguments, and that includes selecting loads from the stack.

rovka updated this revision to Diff 80284.Dec 5 2016, 9:58 AM
rovka updated this object.

Split out the part that was NFC with regard to D26677 (using ValueHandler for <= 4 args). This patch is now only concerned with lowering more than 4 arguments (i.e. loading from the stack). Hopefully it's easier to review now.

rovka updated this revision to Diff 81075.Dec 12 2016, 5:53 AM

Rebase for changes in D26677.

t.p.northover accepted this revision.Dec 16 2016, 11:10 AM
t.p.northover edited edge metadata.

Looks reasonable to me, modulo splitting it up slightly (which isn't even essential).

lib/Target/ARM/ARMInstructionSelector.cpp
84–86 ↗(On Diff #79523)

Yep, but while arg lowering depends on selection, the reverse isn't true. You could use and test these instructions without touching any CallLowering file. I think the changes are completely sound but the commit messages might be more coherent if you did it in two stages ("select XYZ" and "support more args") .

This revision is now accepted and ready to land.Dec 16 2016, 11:10 AM

Thanks for the review, I'll commit it in 2 rounds :)

This revision was automatically updated to reflect the committed changes.