Page MenuHomePhabricator

[GlobalISel][X86] Support call ABI.
ClosedPublic

Authored by igorb on Jun 25 2017, 5:35 AM.

Details

Summary

Support call ABI. For now only Linux C and X86_64_SysV calling conventions supported. Variadic function not supported.

Diff Detail

Repository
rL LLVM

Event Timeline

igorb created this revision.Jun 25 2017, 5:35 AM
oren_ben_simhon added inline comments.Jul 2 2017, 3:56 AM
lib/Target/X86/X86CallLowering.cpp
300 ↗(On Diff #103865)

Since you do not modify OrigArg you can define it as const &

317 ↗(On Diff #103865)

Could you please add a test that checks the case where the callee is a register?

324 ↗(On Diff #103865)

typo arguments

327 ↗(On Diff #103865)

Please consider having a separate method for handling call results.

344 ↗(On Diff #103865)

Could you please document the immediate additions?

test/CodeGen/X86/GlobalISel/callingconv.ll
123 ↗(On Diff #103865)

Could you please add tests that exercise arguments and return values that are spitted (like structures)?

igorb updated this revision to Diff 105029.Jul 2 2017, 6:14 AM
igorb marked 4 inline comments as done.
  • Fix according to comments
  • rebase ToT
igorb added inline comments.Jul 3 2017, 11:55 AM
lib/Target/X86/X86CallLowering.cpp
317 ↗(On Diff #103865)

test_indirect_call cover this case

lib/Target/X86/X86CallLowering.cpp
77 ↗(On Diff #105029)

You don't realy need to pass the DataLayout.
You can get it from the MIRBuilder, something like this:

MachineFunction &MF = MIRBuilder.getMF();
auto &DL = MF.getDataLayout();

Same goes for STI.

84 ↗(On Diff #105029)

Consider calling DL.getPointerSizeOnBits(0) once and assign it to a variable

109 ↗(On Diff #105029)

Please don't submit commented code

122 ↗(On Diff #105029)

Since you don't handle variadic functions, an assert should be added before returning true

igorb updated this revision to Diff 105416.Jul 6 2017, 6:24 AM
  • Fix according to comments and rebase.
lib/Target/X86/X86CallLowering.cpp
122 ↗(On Diff #105029)

I need graceful fallback to DAGSEL in case something not supported yet. Eventually it should be supported.

lib/Target/X86/X86CallLowering.cpp
141 ↗(On Diff #105416)

I am not sure why you made this members as public but i believe that they should be private as no one outside the class scope should change them. It is especially important for non const members.

test/CodeGen/X86/GlobalISel/callingconv.ll
3 ↗(On Diff #105416)

Remove --check-prefix=X64_GISEL which is not needed.

123 ↗(On Diff #103865)

Where are the tests for returned/passed structures?

igorb updated this revision to Diff 106802.Jul 16 2017, 7:00 AM
igorb marked an inline comment as done.
  • Fix according to comments and rebase.
igorb added inline comments.Jul 16 2017, 7:01 AM
test/CodeGen/X86/GlobalISel/callingconv.ll
123 ↗(On Diff #103865)

struct not supported yet.
please see split_return_callee test. In this case <8 x i32> split to 2 xmm registers.
Struct/array will have similar implementations.

This revision is now accepted and ready to land.Jul 16 2017, 7:22 AM
igorb updated this revision to Diff 111870.Aug 20 2017, 2:07 AM
  • rebase.
This revision was automatically updated to reflect the committed changes.