This is an archive of the discontinued LLVM Phabricator instance.

[M68k][GlobalISel] Implement lowerCall based on M68k calling convention
ClosedPublic

Authored by 0x59616e on Dec 16 2021, 5:59 AM.

Details

Summary

This patch implements CallLowering::lowerCall based on M68k calling convention and adds M68kOutgoingValueHandler and CallReturnHandler to handle argument passing and returned value.

Diff Detail

Event Timeline

0x59616e created this revision.Dec 16 2021, 5:59 AM
0x59616e requested review of this revision.Dec 16 2021, 5:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2021, 5:59 AM
0x59616e edited the summary of this revision. (Show Details)Dec 16 2021, 3:36 PM
0x59616e added reviewers: myhsu, ricky26.
0x59616e edited the summary of this revision. (Show Details)
myhsu added a comment.Dec 19 2021, 5:59 PM

Also there is a little problem: GlobalISel reject to combine G_LOAD followed by G_STORE into a move <ea>, <ea> unless there're consecutive in order. That is to say, if there is any instruction between them, GlobalISel will generate weird code like this:

Some of the move <ea>, <ea> definitions are actually missing IIRC. I'm tidying up all the MOVE instructions (to add those missing definitions, for instance) as part of the D115128 series and I think that will solve this problem.

myhsu added inline comments.Dec 19 2021, 7:22 PM
llvm/lib/Target/M68k/M68kInstrData.td
71 ↗(On Diff #394840)

is there any particular reason you made this change? Like, some restrictions imposed by GISel?

xgupta added a reviewer: sushmaunnibhavi.EditedDec 19 2021, 8:29 PM
xgupta added subscribers: sushmaunnibhavi, xgupta.

Adding @sushmaunnibhavi (who started GISel support in M68k) as a reviewer in case she still working in this area.

0x59616e added inline comments.Dec 19 2021, 9:28 PM
llvm/lib/Target/M68k/M68kInstrData.td
71 ↗(On Diff #394840)

Yes, GISel rejects the original pattern. It says "Dst operand has an unsupported type"

The SelectionDAG isn't affected. The MatcherTable is still the same after this change.

Also there is a little problem: GlobalISel reject to combine G_LOAD followed by G_STORE into a move <ea>, <ea> unless there're consecutive in order. That is to say, if there is any instruction between them, GlobalISel will generate weird code like this:

Some of the move <ea>, <ea> definitions are actually missing IIRC. I'm tidying up all the MOVE instructions (to add those missing definitions, for instance) as part of the D115128 series and I think that will solve this problem.

I got it. Thanks for your hard work! I won't touch that part until you've done :)

0x59616e updated this revision to Diff 399843.Jan 13 2022, 5:09 PM
0x59616e retitled this revision from [M68k][GlobalISel] Add basic function call and instruction selection support to [M68k][GlobalISel] Implement basic functionality of CallLowering.
0x59616e edited the summary of this revision. (Show Details)
0x59616e added a reviewer: arsenm.

update diff.

Since now I've discovered some problems in CallLowering and Legalizer, I want to defer the review of RegisterBankInfo & InstructionSelector. Doing that will also make reviewing simpler

0x59616e retitled this revision from [M68k][GlobalISel] Implement basic functionality of CallLowering to [M68k][GlobalISel] Extend the functionality of CallLowering.Jan 13 2022, 10:34 PM
0x59616e edited the summary of this revision. (Show Details)
0x59616e retitled this revision from [M68k][GlobalISel] Extend the functionality of CallLowering to [M68k][GlobalISel] Implement lowerCall based on M68k calling convention.Jan 14 2022, 7:33 PM
0x59616e edited the summary of this revision. (Show Details)
arsenm added inline comments.Jan 17 2022, 3:44 PM
llvm/lib/Target/M68k/GISel/M68kCallLowering.cpp
199

Using F.getCallingConv() is using the calling function's calling convention. You want Info.CallConv like above

215

Ditto

llvm/test/CodeGen/M68k/GlobalISel/irtranslator-call.ll
1

Don't need the 2>&1.

Also, probably should just use update_mir_test_checks

37

Value name in declaration list. I'm not sure why even parse this

41

These tests aren't comprehensive enough. You should add a wider variety of return value and outgoing argument types (particularly some pointers and aggregates). Maybe add some byval / sret too.

You're also testing a few different call instruction types which aren't covered (in particular indirect call and the isPositionIndependent switch).

0x59616e updated this revision to Diff 400807.EditedJan 18 2022, 4:04 AM
0x59616e marked 5 inline comments as done.

Address the issue pointed out by @arsenm. Thanks arsenm !

The following additional tests are included in this revision:

  • return value with i32 / i16 / i8 and sret
  • outgoing argument with i32 / i16 / i8 / pointer / array / struct
  • struct passed byval.
  • integer passed byval.
  • array passed byval.
  • indirect call
  • call with pic
0x59616e added inline comments.Jan 18 2022, 4:06 AM
llvm/lib/Target/M68k/GISel/M68kCallLowering.cpp
199

Done. Thanks :)

215

Done. Thanks :)

llvm/test/CodeGen/M68k/GlobalISel/irtranslator-call.ll
1

Done. Thanks :)

37

I added it accidentally. It should not have been there. I've removed it. Thanks :)

41

I've added a few more tests. Thanks for reviewing ;)

arsenm added inline comments.Feb 7 2022, 5:08 PM
llvm/lib/Target/M68k/GISel/M68kCallLowering.cpp
199

Same with F.isVarArg

215

Same with F.isVarArg

0x59616e updated this revision to Diff 406663.Feb 7 2022, 5:49 PM
0x59616e edited the summary of this revision. (Show Details)

Address feedback from @arsenm. Thanks !

0x59616e marked 2 inline comments as done.Feb 7 2022, 5:49 PM
0x59616e added inline comments.Feb 7 2022, 5:53 PM
llvm/lib/Target/M68k/GISel/M68kCallLowering.cpp
199

Done. Thanks !

215

Done. Thanks !

arsenm accepted this revision.Feb 7 2022, 6:13 PM
This revision is now accepted and ready to land.Feb 7 2022, 6:13 PM

Thanks @arsenm @myhsu !

Could someone commit this on behalf of me ?

name: Sheng
email: ox59616e@gmail.com

Thanks again ! Really appreciate your help ;)