This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add Support for indirect calls on AIX.
ClosedPublic

Authored by sfertile on Nov 26 2019, 9:20 AM.

Details

Summary

Extends the desciptor-based indirect call support for 32-bit codegen, and enables indirect calls for AIX.

In-depth Description:
In a function descriptor based ABI, a function pointer points at a descriptor structure as opposed to the function's entry point. The
descriptor takes the form of 3 pointers: 1 for the function's entry point, 1 for the TOC anchor of the module containing the function
definition, and 1 for the environment pointer.

struct FunctionDescriptor {
  void *EntryPoint;
  void *TOCAnchor;
  void *EnvironmentPointer;
};

An indirect call has several steps of loading the the information from the descriptor into the proper registers for setting up the call. Namely it has to:

  1. Save the caller's TOC pointer into the TOC save slot in the linkage area, and then load the callee's TOC pointer into the TOC register (GPR 2 on AIX).
  2. Load the function descriptor's entry point into the count register.
  3. Load the environment pointer into the environment pointer register (GPR 11 on AIX).
  4. Perform the call by branching on count register.
  5. Restore the caller's TOC pointer after returning from the indirect call.

A couple important caveats to the above:

  • There is no way to directly load a value from memory into the count register. Instead we populate the count register by loading the entry point address into a gpr and then moving the gpr to the count register.
  • The TOC restore has to come immediately after the branch on count register instruction (i.e., the 1st instruction executed after we return from the call). This is an implementation limitation. We could, in theory, schedule the restore elsewhere as long as no uses of the TOC pointer fall in between the call and the restore; however, to keep it simple, we insert a pseudo instruction that represents both the indirect branch instruction and the load instruction that restores the caller's TOC from the linkage area. As they flow through the compiler as a single pseudo instruction, nothing can be inserted between them and the caller's TOC is then valid at any use.

Diff Detail

Event Timeline

sfertile created this revision.Nov 26 2019, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2019, 9:20 AM
sfertile edited the summary of this revision. (Show Details)Nov 26 2019, 9:21 AM
sfertile edited the summary of this revision. (Show Details)Nov 26 2019, 9:23 AM
sfertile edited the summary of this revision. (Show Details)
sfertile updated this revision to Diff 231340.Nov 27 2019, 5:56 PM
  • Change targeted cpu in the test from pwr7 to pwr4.
  • Add testing of object file generation.
  • Change CHECK-NEXT directive to CHECK-DAG directive for instructions that can be scheduled differently.
  • Use a file check variable instead of hard-coded register for scratch register used to move the callee address into the count register.

@sfertile, the current diff is missing the tests.

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

Nit: Add "is" before "not" here and on the next change.

5256

Nit: Add "is" before "not". Add a period.

5288

Nit: Add a comma after "ABIs".

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

Nit: Capitalize "should". Add "the" before "target". Same for the other cases below.

381

Nit: Add "a" before "TOC".

sfertile updated this revision to Diff 232122.Dec 4 2019, 6:45 AM

Addressed comments and added back missing tests.

sfertile marked 5 inline comments as done.Dec 4 2019, 6:46 AM
llvm/test/CodeGen/PowerPC/aix_indirect_call.ll
34

Just noting here that the test does not actually enforce that r2 is saved before being written to. Similarly for the other tests.

35

Just noting here that the test does not actually enforce that the input register is populated before this instruction. Similarly for the other tests.

73

This only parameterizes the scratch register in the human readable form. Two of the hex digits are sensitive to the scratch register value. Similarly for the other test.

75

Same comment. r4 is hardcoded.

96

Just noting that the test does not enforce that the function pointer has been moved to the scratch register before it is clobbered. Similarly for the other tests.

133

Scratch register is not parameterized here and below.

hubert.reinterpretcast edited the summary of this revision. (Show Details)Dec 5 2019, 10:25 AM
ZarkoCA added inline comments.Dec 6 2019, 8:07 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5255

I'm getting confused by this assert. Will it assert if a function has a nest parameter and it's not AIX?
Can it be rewritten to be:

assert((hasNest && Subtarget.isAIXABI()) && "Nest parameter is not supported on AIX.");

Or am i misunderstanding something?

llvm/test/CodeGen/PowerPC/aix_indirect_call.ll
2

I forget whether PWR4 supports altivec,. Wondering if you need -mattr=-altivec here?

I remember there was a discussion about different altivec codegen behaviour when you specify any -mcpu vs leaving it.

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

Can it be rewritten to be:

assert((hasNest && Subtarget.isAIXABI()) && "Nest parameter is not supported on AIX.");

That formulation will trigger the assert unless it is AIX and hasNest is true.

ZarkoCA added inline comments.Dec 6 2019, 8:50 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5255

You're right, I blanked on the the way the assert works. I see now it's meant to enforce that a function doesn't have nest or if it does it's not on AIX. Please ignore my comment, Sean.

sfertile updated this revision to Diff 232650.Dec 6 2019, 2:29 PM
  • Removed all instruction encodings from the test, other then those that are expanded from the newly added pseudo instruction.
  • parameterized the scratch register used in the 64-bit asm.
sfertile marked 6 inline comments as done.Dec 6 2019, 3:14 PM

Just noting here that the test does not actually enforce that r2 is saved before being written to. Similarly for the other tests.

and

Just noting here that the test does not actually enforce that the input register is populated before this instruction. Similarly for the other tests.

The problem is I have 2 different pairs of instructions that are order dependent with respect to each other, and several other instructions that can be interspersed. I'm not sure if its best to use CHECK-NEXT and verify the order at the cost of becoming fragile to the changes in the scheduler, or if its best to use 'CHECK-DAG` and expect that lnt and bootstrap to fail in the case the scheduler ever violated the dependency. I'm not aware of a way to write the test that both verifies those dependencies and keeps the flexibility for the independent instructions.

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

I can Demorgan it if you prefer: !(hasNest && Subtarget.isAIXABI())).

llvm/test/CodeGen/PowerPC/aix_indirect_call.ll
2

I need the attribute or I hit an error in CallLowering_AIX. The check for the error is overly general (just checking if the subtarget supports altivec, and not for any particular use of the altivec functionality). I would like to remove the error but that would need to be done in a seperate patch from this one.

73

Good catch, I completely missed that. The only encoding I'm actually interested in testing are the encodings of the 2 instructions expanded from the new pseudo. I've removed the other encodings from the test.

ZarkoCA added inline comments.Dec 6 2019, 6:13 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5255

It's fine the way it is. I think it's more informative as to what triggers it in your original form.

llvm/test/CodeGen/PowerPC/aix_indirect_call.ll
2

I agree this is something we should discuss. This error to may need to be added to lot of testcases in the future and, in turn, potentially removed when altivec is supported. While also potentially having to rewrite the tests.

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

Nit: Indentation does not match clang-format:

const unsigned TOCSaveOffset =
    Subtarget.getFrameLowering()->getTOCSaveOffset();
7061

Nit: Indentation does not match clang-format:

Chain = DAG.getStore(
    Val.getValue(1), dl, Val, AddPtr,
    MachinePointerInfo::getStack(DAG.getMachineFunction(), TOCSaveOffset));
llvm/lib/Target/PowerPC/PPCInstrFormats.td
1532

Nit: Missing space before xo1.

1533

Please refer to XLForm_2_ext. There should be no parameter taken for bh.

1539

Refer to DForm_1. This should be bits<21> Addr.

1545

Nit: This comment is a label, not a sentence. Do not use a period, but do capitalize.

1552

Same nit about labels versus sentences.

1552

This is incorrect for D-form. The D field has 16 bits.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
1652

Should this be memri (accepting non-aligned operands)?

1653

Should this be iaddr (i.e., D-form)?

llvm/test/CodeGen/PowerPC/aix_indirect_call.ll
146

Seems like it might be less duplication if the grouping was --check-prefixes=ASMOBJ32,ASM32 and --check-prefixes=ASMOBJ32,OBJ32.

sfertile updated this revision to Diff 232932.Dec 9 2019, 1:40 PM
sfertile marked an inline comment as done.
  • Fixed formatting in several places.
  • Fixed the added Instruction format and Ins of the new pseudo op. I had implemented as if the load were a DS form instruction instead of a D form instruction.
  • Collapsed the asm and obj lit testing checks together for the instructions where we don't care about the encoding (and the checks end up being the same between asm and binary output)

Some minor comments; otherwise, I think this is fine, but I don't know if others believe there are still comments to be addressed.

llvm/lib/Target/PowerPC/PPCInstrFormats.td
1539

The naming still refers to the "DS" field for the DS-form instruction.

1545

BH comprises bits 19 and 20, not bits 16 through 18.

llvm/test/CodeGen/PowerPC/aix_indirect_call.ll
116

Should we have an OBJ32-LABEL as well?

sfertile updated this revision to Diff 233365.Dec 11 2019, 7:15 AM

Rebased, combined the ASM32 and OBJ32 checks that were the same into 1 ASMOBJ32 check label, moved comment on tabelgen InstrFormat member to the bh bits field.

sfertile marked 19 inline comments as done and an inline comment as not done.Dec 11 2019, 7:18 AM
sfertile added inline comments.
llvm/test/CodeGen/PowerPC/aix_indirect_call.ll
146

Good Call.

llvm/lib/Target/PowerPC/PPCInstrFormats.td
1545

Do we want a comment here to indicate that this is a "reserved" field?

llvm/test/CodeGen/PowerPC/aix_indirect_call.ll
19

Missing reference to CHECKOBJ.

Nit: It may be better to rename the test aix-indirect-call.ll to be consistent.

I think this patch looks good, I'll leave for @hubert.reinterpretcast to approve after his latest round comments.

Nit: It may be better to rename the test aix-indirect-call.ll to be consistent.

I agree.

I think this patch looks good, I'll leave for @hubert.reinterpretcast to approve after his latest round comments.

It seems there are only minor comments left that can be fixed on the commit. This patch LGTM.

This revision is now accepted and ready to land.Dec 11 2019, 1:00 PM

Committed as rG93faa237da8d. @sfertile, typo in the commit message: s/Differtential/Differential/;