Page MenuHomePhabricator

Implement call lowering without parameters on AIX
ClosedPublic

Authored by jasonliu on May 15 2019, 7:50 AM.

Details

Summary

This patch implements call lowering for calls without parameters on AIX as initial support.

Diff Detail

Repository
rL LLVM

Event Timeline

jasonliu created this revision.May 15 2019, 7:50 AM
aheejin added inline comments.May 17 2019, 5:02 AM
llvm/lib/Target/TargetMachine.cpp
181 ↗(On Diff #199610)

No TT.isOSBinFormatWasm() anymore?

jasonliu marked an inline comment as done.May 17 2019, 7:11 AM
jasonliu added inline comments.
llvm/lib/Target/TargetMachine.cpp
181 ↗(On Diff #199610)

Thanks for catching this. I will add it back in.

jasonliu updated this revision to Diff 200502.Tue, May 21, 7:21 AM
sfertile added inline comments.Tue, May 21, 6:06 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6612 ↗(On Diff #200502)

minor nit: missing period.

6616 ↗(On Diff #200502)

We don't need !isTailCall in the condition here.

6624 ↗(On Diff #200502)

With this patch AIX is using IsEligibleForTailCallOptimization to determine if a call can be a tail-call. Is this safe because we expect getTargetMachine().Options.GuaranteedTailCallOpt to be false on AIX ? If thats the case then we should assert that here. Or should we have an llvm_unreachable here for unsupported tail-calls for now?

efriedma added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6595 ↗(On Diff #200502)

Please use report_fatal_error instead of llvm_unreachable for code that you expect is actually reachable.

hubert.reinterpretcast requested changes to this revision.Wed, May 22, 7:51 AM
hubert.reinterpretcast added inline comments.
llvm/lib/Target/PowerPC/PPCCallingConv.td
309 ↗(On Diff #200502)

Testing seems to confirm that r13 is a non-volatile as is stated in Table 5 of AIX Version 7.2: Assembler Language Reference under the 32-bit environment.

int main(void) {
  int ret, val = 55;
  __asm__(
      "mr 13,%1\n\t"
      "mr %0,13"
      : "=r"(ret)
      : "r"(val)
      : "r13"
      );
  return ret;
}
This revision now requires changes to proceed.Wed, May 22, 7:51 AM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
74 ↗(On Diff #200502)

Minor nit: We should group checks of "similar" properties together. So, AIX and Darwin together.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5157 ↗(On Diff #200502)

Minor: No comma for lists of less than three items.

Suggestion:

SVR4 or AIX ABIs.

5164 ↗(On Diff #200502)

The NOP may change to become some other NOP.

5167 ↗(On Diff #200502)

Minor nit: Missing space before !isPatchPoint.

llvm/lib/Target/PowerPC/PPCISelLowering.h
1120 ↗(On Diff #200502)

Minor nit: Indentation needs adjustment.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
1460 ↗(On Diff #200502)

Minor nit: Other blocks align to after the <.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6599 ↗(On Diff #200502)

This should say "reserved". The Darwin version uses pre-reserved; the prefix seems redundant.

6604 ↗(On Diff #200502)

s/its varargs/the callee is variadic/;

llvm/test/CodeGen/PowerPC/test_call_aix.ll
11 ↗(On Diff #200502)

Question: Where is the code that ensures quadword (16-byte) alignment of the stack?

jasonliu marked 2 inline comments as done.Thu, May 23, 8:26 AM
jasonliu added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6624 ↗(On Diff #200502)

getTargetMachine().Options.GuaranteedTailCallOpt could return true if -tailcallopt is passed in from the command line.
I will put a report_fatal_error here to indicate unsupported tail calls for now.

llvm/test/CodeGen/PowerPC/test_call_aix.ll
11 ↗(On Diff #200502)

I believe it's done in PPCFrameLowering::determineFrameLayout.

jasonliu updated this revision to Diff 200984.Thu, May 23, 8:34 AM

Address review comment round 2.

jasonliu marked 11 inline comments as done.Thu, May 23, 8:35 AM
jasonliu marked an inline comment as done.

One minor comment; otherwise, all of my comments have been addressed.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5157 ↗(On Diff #200984)

Add "the" before AIX to clarify binding of "64-bit" as applying only to SVR4.

jasonliu updated this revision to Diff 201016.Thu, May 23, 10:57 AM

Clarify the comments modified for AIX.

jasonliu marked an inline comment as done.Thu, May 23, 10:57 AM
sfertile accepted this revision.Fri, May 24, 9:43 AM

LGTM.

This revision is now accepted and ready to land.Fri, May 24, 9:43 AM
This revision was automatically updated to reflect the committed changes.