Page MenuHomePhabricator

[AIX] TOC pseudo expansion for 64bit large + 64bit small + 32bit large modes
ClosedPublic

Authored by Xiangling_L on Oct 2 2019, 8:21 AM.

Details

Summary

On AIX, for toc-referenced globals,

  1. under large code model, we generate:

[32 bit]
LWZtocL(@sym, ADDIStocHA(%r2, @sym))
[64 bit]
LDtocL(@sym, ADDIStocHA8(%x2, @sym)).

  1. under small code model, we generate:

[32 bit]
LWZtoc @op1, %r2
[64 bit]
LDtoc @op1, %x2

In this patch, we provide support for peudo ops including ADDIStocHA8, ADDIStocHA, LWZtocL, LDtoc, LDtocL for AIX('LWZtoc' is supported by an earlier patch already), lowering them from MIR to assembly.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Rebase on latest master to pick up 'isDarwin' related patch

sfertile added inline comments.Oct 6 2019, 7:06 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
80

SRE is only used in an assert, which will introduce an unused local warning when asserts are disabled.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
516

Should we try to include 'TOC' in the name since this is specifically for mapping the TOC pseudos symbol ref operand into an MCSymbol?

736

isDarwin --> IsDarwin. (There are a couple places in EmitInstruction that will need to be updated similarly)

Xiangling_L marked 3 inline comments as done.

Move variable 'SRE' into assertion & rename function name to 'getMCSymbolForTOCPseudoMO'

sfertile added inline comments.Oct 8 2019, 9:32 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
755

Very minor nit: We can slightly simplify if you drop the `!' and reverse the true/false sides.

829

Was the ';' after 'block-address' meant to be a comma?

838

Minor nit: We can fold away the if:

bool GlobalToc = MO.isGlobal() && Subtarget->isGVIndirectSymbol(MO.getGlobal());

880

Instead of using an 'if' statement that will be empty when assertions are disabled, can we fold this condition into the assert? ie

LLVM_DEBUG(
  assert((!MO.isGlobal() || Subtarget->isGVIndirectSymbol(MO.getGlobal())) && ...
909–910

Ditto on folding away the if.

llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll
1–5

Please add '-verify-machine-instr` and an mcpu option to each llc invocation.

llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll
3

ditto.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
77

Add "an" before "addis".

80

Do not use dyn_cast for its Boolean value. Use isa.

81

Same comment regarding "an".

82

Add "if it is an expression at all" to the end of the sentence.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
923

There is quite a bit of noise in this patch from these NFC changes. Please split them out.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
3172 ↗(On Diff #223652)

Why the whitespace change on this line? The surrounding defs have spaces around the colon.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
829

Add "or" before "is a block address".
Add "the" before "large code model".
Add a comma after "Otherwise".

jasonliu added inline comments.Oct 8 2019, 10:53 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
908

nit: MOSymbol could move down a bit to be closer to where it get used.

Also I would want to add const to this MOSymbol. But it seems that a lot of similar MOSymbols in this file do not have const with them, only add const for this one here would create inconsistency. Not sure if it's worth to have a NFC patch to add const for all the MOSymbol in this file first.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
3171 ↗(On Diff #223420)

Curious about what the effect is for adding hasSideEffects and isReMaterializable.
We already have ADDIStocHA before for the other targets, but they did not require hasSideEffects = 0, and isReMaterializable = 1.
So what is special about the AIX target that we need to add them? Or is it simply an omission before and it's actually needed for the other targets as well?
I try to remove this line here and no test case would fail. If we need those, should we add test case for it?

llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll
1–5

Do we want to add -verify-machineinstrs to every llc invocation?

Xiangling_L marked 14 inline comments as done.Oct 8 2019, 2:10 PM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
908

As Sean mentioned to me, Stefan is doing this NFC patch about MOSymbol, so I think I can put this up there: https://github.ibm.com/compiler/llvm-project/pull/672.

923

Thanks for mentioning this, will do.

Xiangling_L marked 10 inline comments as done.Oct 9 2019, 7:38 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.td
3171 ↗(On Diff #223420)

Some of my rational of adding these two properties are as follows, and please feel free to raise your further concerns:

  1. For the instruction property hasSideEffects, it's used to indicate "does the instruction have side effects that are not captured by any operands of the instruction or other flags". And it's unset by default, the TableGen will infer its value from the instruction pattern when possible. One rationale I used to add this property is that since ADDIStocHA and ADDIStocHA8 [using under 64-bit mode] have the same instruction pattern, it should be safe to explicitly specify this hasSideEffects=0 for ADDIStocHA. And also considering ADDIStocHA itself, what it does is to load a value from TOC, it does have no side effect like implicit function call, volatile variable read etc, so I set this property to be 0.

2.For the instruction property isReMaterializable = 1, meaning this instruction has no side effects and requires no operands that aren't always available. The only allowed uses are constants and unallocatable physical registers so that the instructions result is independent of the place in the function.
Since it loads value from TOC, and TOC register is always reserved on PPC target, so I think it's safe to set it as 1. Plus, the same reason as above, I would prefer sync the behavior of ADDIStocHA and ADDIStocHA8`.

llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll
1–5

Thank you for reminding me of this, will do.

1–5

Yes, I will add it and cpu level for testcases as well.

Xiangling_L marked 3 inline comments as done.

Address 2nd round comments;
split out variable name NFC patch;
update testcases;

Notes:
A NFC patch about getMCSymbolForTOCPseudoMO will be splited out from this patch, and after that patch is landed, I will rebase this lowering patch onto it, then post following variable name NFC patch and 'hasSideEffects & isRematerializable' NFC patch.

Rebase on latest master after the PPCAsmPrinter cleanup NFC patch landed.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
757

Similar cases below have two more spaces of indentation.

847

See comment below re: the !.

890

I believe @sfertile made a comment about avoiding the ! in the condition in cases like these.

llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll
27

This does not follow. REG3 apparently holds the address of the operand for the load, so REG4 holds the address of the target of the store. We are loading the value to REG4 though, so its value will be clobbered before we get to the store.

llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll
40

That the ordering and interleaving of the logical operations involved differ between the various cases seem to indicate that the test is already too complicated. Please reduce the test to use a single memory operand (e.g., store a constant or return the value read).

Xiangling_L marked 7 inline comments as done.Oct 11 2019, 11:39 AM
Xiangling_L added inline comments.
llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll
40

@sfertile I guess your original purpose of creating this testcase is to test if load from TOC works for both load and store?

llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll
27

Sorry, it was my mistake, it should be [[REG5:[0-9]+]]

Xiangling_L marked 2 inline comments as done.

correct code format & the testcase

hubert.reinterpretcast marked an inline comment as done.Oct 11 2019, 11:47 AM
hubert.reinterpretcast added inline comments.
llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll
40

If that is indeed the intent, then the goal can be achieved with more tests that are simpler.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
928–929

Two more spaces here.

Xiangling_L marked 2 inline comments as done.Oct 11 2019, 6:02 PM
Xiangling_L added inline comments.
llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll
40

Thanks, I will update the testcase

Update testcases: split into simpler ones

hubert.reinterpretcast marked an inline comment as done.

LGTM with minor changes.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
82

Indentation:

assert(isa<MCSymbolRefExpr>(MI->getOperand(2).getExpr()) &&
       "The third operand of an addis instruction should be a symbol "
       "reference expression if it is an expression at all.");
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
736

The space after assert seems odd. clang-format removes this space.

768

We know IsDarwin is false here. For code readability purposes, I am okay with this line as-is because the Darwin code is supposedly going away.

823

Same comment about the space.

863

Same comment about the space.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
333

This patch adds ADDIStocHA to two switches with a different ordering relative to ADDIStoHA8 in each switch.

llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll
15

This is a function entry point label, so

SMALL-LABEL: .test_load:{{$}}

is appropriate.

20

Same comment re: function entry label.

33–42

Same comment re: function entry label.

38

Same comment re: function entry label.

llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll
16

Same comment re: function entry label.

18

Just a note:
Currently, the front-end seems to not generate signext or zeroext into the IR for 64-bit AIX. It seems the default behaviour matches zeroext (meaning the return type is unsigned).

21

Same comment re: function entry label.

34

Same comment re: function entry label.

39

Same comment re: function entry label.

This revision is now accepted and ready to land.Oct 11 2019, 8:30 PM
llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll
18

We discussed this today, and we want to explicitly add the zeroext in (or signext if that is was is intended). @Xiangling_L, please add this change on the commit as well. Thanks.

Xiangling_L marked 15 inline comments as done.

Minor changes & add zeroext to 64bit testcase

llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll
16

Does this pass without a colon? Check also for the cases below.

29

i32 zeroext %0

Xiangling_L marked 3 inline comments as done.

update the 64 bit testcase

LGTM. Thanks.

Thank you for your reviews.

llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll
16

Thanks for mentioning this, it happended to pass because it matched .test_load in function descriptor. I will update it to function entry point label.

This revision was automatically updated to reflect the committed changes.

This revision breaks builds with assertions disabled because IsPPC64 (introduced on line 539 of PPCAsmPrinter.cpp is unused when assertions are turned off.