This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] update PPCTTIImpl::supportsTailCallFor() check conditions
ClosedPublic

Authored by tingwang on Dec 19 2022, 8:44 PM.

Details

Summary

This patch reuse PPCTargetLowering::isEligibleForTCO() to check PPCTTIImpl::supportsTailCallFor().

Fixes #59315

Diff Detail

Event Timeline

tingwang created this revision.Dec 19 2022, 8:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2022, 8:44 PM
tingwang requested review of this revision.Dec 19 2022, 8:44 PM
tingwang updated this revision to Diff 486512.Jan 5 2023, 2:54 AM

Refresh and Ping.

hmm, another issue cause by inconsistent logic for the tail call check between the IR coro-split pass and the instruction selection pass. Should we make them be consistent to avoid more issues?
For example, can we refactor IsEligibleForTailCallOptimization_64SVR4 and IsEligibleForTailCallOptimization() and call them in the TTI function supportsTailCallFor? I see there is a SelectionDAG &DAG parameter for these two functions, but the DAG is used to get the Caller Function, I think it should be easy to change the SelectionDAG to a Function parameter?

And for mayBeEmittedAsTailCall(), maybe we should also add another patch to extend it for non 64BitELFABI targets?

llvm/test/Transforms/Coroutines/coro-split-musttail-ppc64le.ll
14

Why asserts is required? I don't see checks of debug logs?

Seems for patch that addresses github issues, you can add a comment Fixes #59315 in the description.

tingwang planned changes to this revision.Jan 12 2023, 5:30 PM

As suggested by Zheng, I'm looking for a solution that reuse existing functions in PPCISelLowering.

tingwang updated this revision to Diff 488970.Jan 13 2023, 5:39 AM
tingwang edited the summary of this revision. (Show Details)

(1) Use new API to do the check.
(2) Remove assert requirement.
(3) Fixed test case: CHECK-PCREL should not be tailcall.

tingwang marked an inline comment as done.Jan 13 2023, 5:40 AM
tingwang added inline comments.
llvm/test/Transforms/Coroutines/coro-split-musttail-ppc64le.ll
14

Good catch. Thank you!

tingwang marked an inline comment as done.Jan 13 2023, 6:03 AM

hmm, another issue cause by inconsistent logic for the tail call check between the IR coro-split pass and the instruction selection pass. Should we make them be consistent to avoid more issues?
For example, can we refactor IsEligibleForTailCallOptimization_64SVR4 and IsEligibleForTailCallOptimization() and call them in the TTI function supportsTailCallFor? I see there is a SelectionDAG &DAG parameter for these two functions, but the DAG is used to get the Caller Function, I think it should be easy to change the SelectionDAG to a Function parameter?

And for mayBeEmittedAsTailCall(), maybe we should also add another patch to extend it for non 64BitELFABI targets?

Thank you! I created D141673 to refactor the two functions as you suggested. Regarding mayBeEmittedAsTailCall(), it should be easy to extend, and will be addressed separately.

shchenz added inline comments.Feb 26 2023, 9:29 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5664 ↗(On Diff #488970)

Should this be on PPC target only or it should be a target independent check? shouldBeMustTail checks some target independent conditions.

llvm/test/Transforms/Coroutines/coro-split-musttail-ppc64le.ll
13

nit: ppc32-- and powerpc-- should both be for PPC arch (32 bit, BE), I think just one line is needed.

tingwang updated this revision to Diff 500719.Feb 27 2023, 3:00 AM

Update according to comments:
(1) Removed IsByValArg check which is redundant.
(2) Removed one test case scenario which is redundant.

tingwang marked an inline comment as done.Feb 27 2023, 3:00 AM
tingwang added inline comments.Feb 27 2023, 3:04 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5664 ↗(On Diff #488970)

Yeah, these checks are already covered by shouldBeMustTail, and could be redundant. I removed the check and updated the patch. Thank you!

shchenz accepted this revision as: shchenz.Feb 27 2023, 6:55 PM

LGTM with one nit.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5672 ↗(On Diff #500719)

we may need a NFC patch to remove the IsByValArg for isEligibleForTCO as all callers pass false to it, we should be able to check the byval parameter inside isEligibleForTCO.

This revision is now accepted and ready to land.Feb 27 2023, 6:55 PM
tingwang added inline comments.Feb 28 2023, 5:35 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5672 ↗(On Diff #500719)

Sure, will do. Thank you!

tingwang edited the summary of this revision. (Show Details)Feb 28 2023, 7:28 PM
This revision was landed with ongoing or failed builds.Feb 28 2023, 7:29 PM
This revision was automatically updated to reflect the committed changes.