Page MenuHomePhabricator

[PowerPC] Refactor FinishCall [NFC]
ClosedPublic

Authored by sfertile on Nov 12 2019, 7:39 AM.

Details

Summary

Refactor FinishCall to be more easily understandable as a precursor to implementing indirect calls for AIX. The refactor tries to group similar code together at the cost of some code duplication. The high level overview of the refactor:

- Adds a number of helper functions for things like:
  * Determining if a call is indirect.
  * What the Opcode for a call is.
  * Transforming the callee for a direct function call.
  * Extracting the Chain operand from a CallSeqStart node.
  * Building the operands of the call.

- Adds helpers for building the indirect call DAG nodes (excluding the call instruction itself which is created in `FinishCall`).
- Removes PrepareCall, which has been subsumed by the helpers.
- rename 'InFlag' to 'Glue'.

- FinishCall has been refactored to:
  1) Set TOC pointer usage on the DAG for the TOC based subtargets.
  2) Calculate if a call is indirect.
  3) Determine the Opcode to use for the call instruction.
  4) Transform the Callee for direct calls, or build the DAG nodes for indirect calls.
  5) Buildup the call operands.
  6) Emit the call instruction.
  7) If needed, emit the callSeqEnd Node and finish lowering by calling `LowerCallResult`

A couple notes:

  • Sorry for the size of the patch, but it was difficult to find a more incremental approach that didn't need all the split up patches for the full context anyway.
  • I intend at least 1 follow on patch to cleanup up the handful of boolean flags we are passing between LowerCall and LowerCall_<Platform> and FinishCall.

Diff Detail

Event Timeline

sfertile created this revision.Nov 12 2019, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2019, 7:39 AM
sfertile edited the summary of this revision. (Show Details)Nov 12 2019, 7:39 AM

Overall, I think this refactor aids in readability and clarity of calling convention code PPCISelLowering.cpp. Even small changes like renaming InFlag to Glue go a long way in helping understanding.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5059

spelling nits:
abis -> ABIs
"callers TOC pointer" -> caller's TOC pointer
pseduo->pseudo
immeidatly ->immediately

5207

*environment

llvm/lib/Target/PowerPC/PPCSubtarget.h
356

I was looking into this, and does the SVR4ABI refer to both ELF v1 and ELF v2? Is that why you can't use isSVR4ABI but instead check that it's not ELFv2?

jasonliu added inline comments.Nov 15 2019, 12:27 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5232

Could this be a static function?

5242

If it's a direct call, pass ...

The new logic is much clearer, though it's hard to discern that the semantics remain correct because so much logic was redistributed. If a series of smaller commits were possible it would be easier to review.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5232

Did you intend buildCallOperands to be static?

5301

You've removed PrepareCall and should update the comment.

sfertile updated this revision to Diff 229862.Mon, Nov 18, 9:03 AM
sfertile marked 9 inline comments as done.
  • Various spelling and punctuation fixes.
  • made 'buildCallOperands' static
  • Added a comment in the 'useFunctionDescriptors' querry.
  • renamed 'buildIndirectCall' and 'buildDescriptorIndirectCall' by changing 'build' to 'prepare' --> The helpers build the DAGs, setting up the call, but don't insert the call instruction itself so 'prepare' is a better description then 'build'.

The new logic is much clearer, though it's hard to discern that the semantics remain correct because so much logic was redistributed. If a series of smaller commits were possible it would be easier to review.

I have to apologize about the size of the patch, its large and more disruptive then I initially hoped for. I attempted to break the patch down into a couple smaller ones but everything I cam up with either had new code that didn't really make sense in the context it was being added into (but did make sense once combined with the other changes) or the intermediate steps were decidedly worse then the starting point. I figured it was less work to review 1 large patch then it was to review several smaller ones in parallel (because they were all needed for context).

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5232

Yeah. updated.

5301

Good catch.

llvm/lib/Target/PowerPC/PPCSubtarget.h
356

SVR4ABI is all the ELF based ABIs: so powerpc32 targeting elf and both the v1 and v2 64-bit powerpc abis targeting elf. The only descriptor based elf abi is 64-bit ELF v1. If I used isSVR4ABI() I'd have to add in a isPPC64() as well to filter out the 32-bit ELF abi. I'll add a comment to make it easier for the reader to figure out why we are excluding the v2 abi.

lkail added a subscriber: lkail.Mon, Nov 18, 10:28 PM

Since this is a NFC patch, does it make sense to add NFC on patch title?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5039

It looks like !Subtarget.usesFunctionDescriptors() && !Subtarget.isELFv2ABI() eqauls to isDarwin() || is32ELFv1ABI(), but AFAIK Darwin is abondoned, and there is no 32bit ELFv1 ABI(Correct me if I am wrong), can we skip this query?

If the clang-format "TODO" in the patch description has been addressed, then the patch description should be updated.

Xiangling_L added inline comments.Tue, Nov 26, 1:55 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5078

Please fix the formatting issue here.

5155

Please fix the formatting issue from line 5145 - line 5149.

5259

I feel it's worth mentioning in the comment The address needs to go after the chain input but before the flag (or any other variadic arguments). for AddTOC like the original comment does, in case ppl mistakenly move this around in the future.

5287

Since there are a few spots using Subtarget.isPPC64() and Subtarget.isPPC64() ? MVT::i64 : MVT::i32), can we create a variable for each to shorten the code?

Xiangling_L added inline comments.Tue, Nov 26, 2:06 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5386

Just a question, how did you verify that Ins will never be empty?

sfertile marked 2 inline comments as done.Wed, Nov 27, 10:36 AM
sfertile added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5039

There is only a single 32-bit ELF ABI so we don't need to disambiguate it by calling it v1.The current condition is equivalent to isDarwin() || is32BitELFABI(). Targeting Darwin is deprecated, but we haven't started ripping out the Darwin code yet so condensing to just is32BitELFABI() is a bit premature in my opinion. I used the form above to try to highlight that the specific ABIs have features which are incompatable with a BLA but I think the comment here and the description in rL2111174 convey that accurately so we can switch to your suggested isDarwin() || is32BitELFABI() if you feel strongly.

5386

Ins can be empty. I found conditionally updating the Glue was confusing. It made me think we glue Ins to the Call_SEQEND node, but glue something else to the Call instruction if we have no ins. From your comment it seems you assumed similarly. (We already glue the Call instr to the Call_SEQEND node and IIUC correctly gluing another node to it wouldn't be valid .)

The reality is if we have no Ins then the array of return value CCValAssigns in LowerCallResult will be empty, and there is nothing else to glue into the chain, making updating the glue harmless.

sfertile updated this revision to Diff 231316.Wed, Nov 27, 1:21 PM
  • clang-formatted the patch.
  • Add back dropped comment (but reworded and expanded)
  • Added a local to replace several calls to Subtarget.isPPC64()
sfertile retitled this revision from [PowerPC] Refactor FinishCall to [PowerPC] Refactor FinishCall [NFC].Wed, Nov 27, 1:21 PM
sfertile edited the summary of this revision. (Show Details)
sfertile updated this revision to Diff 231333.Wed, Nov 27, 4:55 PM
sfertile marked 4 inline comments as done.
  • Accidentally dropped the useful part of the previous suggested comment when rewording. Added it back.
  • Added a local variable set to the MVT for a gpr, and replaced several conditional operator uses with it.
  • Reworded another comment that made it sound like plt stubs save multiple things to the stack.
sfertile marked 3 inline comments as done.Wed, Nov 27, 4:57 PM

Since this is a NFC patch, does it make sense to add NFC on patch title?

I missed that, updated.

If the clang-format "TODO" in the patch description has been addressed, then the patch description should be updated.

Done.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5078

Should be fixed now.

5155

Should be fixed now.

5259

Good point. I added it back but reworded it a bit and merged it with the other comment.

I couldn't apply your patch on latest master branch, do you mind updating it?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5039

Thank you for your clarification. I also think !Subtarget.usesFunctionDescriptors() && !Subtarget.isELFv2ABI() is better, it's more descriptive.

sfertile updated this revision to Diff 232004.Tue, Dec 3, 3:49 PM
sfertile marked an inline comment as done.

Rebased to pick up recent changes to PPCISelLowering.cpp

sfertile marked an inline comment as done.Tue, Dec 3, 3:51 PM
sfertile added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5067

Not sure why I dropped the extra part of the sentence here. I'll add it back.

ZarkoCA accepted this revision.Wed, Dec 4, 7:19 AM

Patch looks good to me.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5363

I don't think you wrote the original comment but maybe we can correct: "Expecting a" instead of "Expecting an"

This revision is now accepted and ready to land.Wed, Dec 4, 7:19 AM
Xiangling_L accepted this revision.Fri, Dec 6, 9:56 AM
This revision was automatically updated to reflect the committed changes.