Page MenuHomePhabricator

[AIX]Global Address Lowering
ClosedPublic

Authored by Xiangling_L on Jun 19 2019, 6:22 AM.

Details

Summary

This patch implements global address lowering for 32/64 bit with small/large code model.

1.For 32bit large code model on AIX, there are newly added pseudo opcode LWZtocL & ADDIStocHA32, the support of which on MC layer will be provided by future patches.

2.The default code model on AIX should be small code model

3.Since AIX does not have medium code model, "report_fatal_error" when users specify it.

Diff Detail

Repository
rL LLVM

Event Timeline

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

restructure the code

Xiangling_L marked an inline comment as done.Jul 9 2019, 6:47 PM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp
118 ↗(On Diff #208295)

Sorry I didn't make it clear enough. The thing was that after I added the assertion, there were around 200 LIT testcases failed at isPPC64() It's because by default we added the TOCRegDeps pass for each PPC target. That makes me start to think that maybe I shouldn't have added that check here, and maybe there does exist 32bit platform where X2 is needed to be reserved. For example,

[lib/Target/PowerPC/PPCFastISel.cpp]

unsigned PPCFastISel::PPCMaterializeFP(const ConstantFP *CFP, MVT VT) {
...
      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc), DestReg)
        .addConstantPoolIndex(Idx, 0, PPCII::MO_TOC_LO)
        .addReg(TmpReg)
        .addMemOperand(MMO);
...
}

It's adding PPCII::MO_TOC_LO target flag to constant pool index on both 32-bit&64-bit, for which, according to the function line 96: hasTOCLoReloc, we need to reserve X2.

And I feel that even if we are right that we shouldn't deal with 32bit medium/large code model anywhere, I am not sure if we want to clean those mess in this patch?

sfertile added inline comments.Jul 12 2019, 12:05 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5070 ↗(On Diff #208859)

minor nit: we should probably check for SVR4 explicitly instead of if the object file format is ELF, just to be pedantic.

5074 ↗(On Diff #208859)

nit: just check for isDarwin. Its really just an easy way to convey to the reader that Darwin code never reaches here. `
!isDarwin && (isELFABI || isAIXABI)` is tautological.

5093 ↗(On Diff #208859)

nit: Couldn't the first operand also be a constant pool index or a block address? Maybe we just say its the symbol-reference?

5119 ↗(On Diff #208859)

nit: combine the 2 ifs into a single condition.

5127 ↗(On Diff #208859)

Follows form previous comment: I think this would be more readable if we try to keep the nesting to a minimum, eg:

if (!isPPC64 && isELFABI) {
   ....
   return;
}

if (!isPPC64 && isAIX && CModel == CodeModel::Small) {
  ...
return;
}
5142 ↗(On Diff #208859)

Can we move this up either directly before or directly after the medium code model error for AIX?

llvm/lib/Target/PowerPC/PPCInstrInfo.td
3173 ↗(On Diff #208859)

I think the naming convention is for 32-bit instructions is to have no number appended to the name, and 64-bit instructions have '8' appended.

Xiangling_L marked 7 inline comments as done and an inline comment as not done.Jul 13 2019, 10:06 AM

minor changes of restructured code

restrict r0 usage for pseudo opcode ADDIStocHA32

sfertile added inline comments.Jul 16 2019, 2:01 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5072 ↗(On Diff #209932)

minor nit: do we need a local for this? As far as I can tell its used just once.

5116 ↗(On Diff #209932)

minor nit: independant --> independent

5118 ↗(On Diff #209932)

minor nit: codemodel --> code model

5136 ↗(On Diff #209932)

minor nit: I personally prefer having error handling (and the following code where we break from the case) before anything else. Not sure how others feel about this.

5070 ↗(On Diff #208859)

Sorry if my previous comment was unclear. I think the name of the local was fine, in fact i find it much more readable then isSVR4ABI, just we should query they subtarget for the ABI instead of checking the object file format.

const bool isELFABI = PPCSubTarget->isSVR4ABI();

5142 ↗(On Diff #208859)

I really think this is better at the top of the block , rather then moving the error checking down. You should be able to get rid of the replaceForMediumOrLargeCM lambda by handling this before AIX large code model.

llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp
118 ↗(On Diff #208295)

The thing was that after I added the assertion, there were around 200 LIT testcases failed at isPPC64() It's because by default we added the TOCRegDeps pass for each PPC target.

Ok. the failures are understandable then, we always add the pass so it runs but does nothing on 32-bit ELF targets. FWIW the 32-bit ELF abi supplement does have large code model examples, although using r31 as a GOT-pointer, and unless the 32-bit ELF linker does optimizations then there is no implicit dependence for the global register they do use.

It's adding PPCII::MO_TOC_LO target flag to constant pool index on both 32-bit&64-bit, for which, according to the function line 96: hasTOCLoReloc, we need to reserve X2.

That code will only be run for 64-bit targets. There is no comments explicitly stating that, but they get a temporary register from the PPC::G8RC_and_G8RC_NOX0RegClass register class which is 64-bit gpr register. 'PPC::X2' is likewise a 64-bit gpr, we couldn't add it as an operand on a 32-bit instruction.

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
251 ↗(On Diff #209932)

I suggest we take this chance to split this up. It gets longer, but I think much more readable. Something along the lines of:

if (JIT)
  return CodeModel::Small;

if (TT.isOSAIX())
  return CodeModel::Small;

assert(TT.isOSBinFormatELF() && "All remaining PPC OSs are ELF based.");

if (TT.isArch32())
  return CodeMode::Small;

assert(TT.isArch64() && "Unsupported  PPC architecture.");
return CodeModel::Medium;

I made the assumption we don't have to support Darwin here anymore. There may be 1 or 2 tests left that use a Darwin target with llvm-mc. If you encounter problems I'll look at changing the test to no longer need darwin if possible. I think we have reached a point where we should try to actively clean up the darwin code.

Xiangling_L marked 10 inline comments as done.Jul 17 2019, 9:05 AM

further restructure the code

Xiangling_L marked an inline comment as done.Jul 17 2019, 11:44 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
251 ↗(On Diff #209932)

Thanks, I updated it. And there is no Darwin LIT testcase failure currently.

sfertile marked an inline comment as done.Wed, Jul 24, 9:46 AM

A couple minor comments, I think we are almost there.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5079 ↗(On Diff #210376)

Would it be better to document that Tiny and Kernel code models are not supported with an assert as opposed to a comment? Then we could simplify both the comment and the conditional below to isPPC64 && CodeModel == CodeModel::Small, as well as remove the assert following the comment // Small code model has been handled. below.

5120 ↗(On Diff #210376)

minor nit: LWZtocL(@sym, ADDIStocHA(%r2, @sym))

5122 ↗(On Diff #210376)

LDtocL(@sym, ADDIStocHA8(%x2, @sym))

5124 ↗(On Diff #210376)

ditto

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14314 ↗(On Diff #210376)

minor nit: The second half of the comment doesn't really fit in with the rest of the function. Its an implementation detail (which is also explained at the point we perform said lowering). This function is about when we have to add indirection when accessing an operand. I would suggests shortening the comment to:

// If it is small or large code model, module locals are accessed
// indirectly by loading their address from .toc/.got.
llvm/lib/Target/PowerPC/PPCInstrInfo.td
3173 ↗(On Diff #208859)

You should be able to rebase onto master to pick up the ADDIStocHA --> ADDIStocHA8 patch.

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
251 ↗(On Diff #209932)

And there is no Darwin LIT testcase failure currently.

Nice.

Xiangling_L marked 6 inline comments as done.

minor changes

sfertile accepted this revision.Fri, Jul 26, 10:55 AM

A minor comment, but otherwise LGTM.

llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp
115 ↗(On Diff #211650)

minor nit: We only need to check isPPC64(), and not isAIXABI()'. If you want to document that isAIXABI()` is true whenever isPPC64() is false then an assert will do that.

This revision is now accepted and ready to land.Fri, Jul 26, 10:55 AM
Xiangling_L marked 2 inline comments as done.Mon, Jul 29, 7:42 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp
115 ↗(On Diff #211650)

I see what you mean, but I have a concern that the original code const unsigned TOCReg = PPC::X2 reserves X2 for PPC64 and PPC32, which though has no effect on 32bit mode. But if we do const unsigned TOCReg = (isPPC64) ? PPC::X2 : PPC::R2;, we do affect other 32bit platform other than AIX by reserving r2 considering this machine pass runs for all PPC target. Is this okay?

Xiangling_L marked 2 inline comments as done.Mon, Jul 29, 8:04 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp
115 ↗(On Diff #211650)

To be more clear, as I mentioned before, when I added an assertion for isPPC64 there were 200ish testcases failures, it indicates that there are other 32bit PPC platform running this function, so only check isPPC64 like const unsigned TOCReg = (isPPC64) ? PPC::X2 : PPC::R2; may affect those 32bit platform?

sfertile added inline comments.Mon, Jul 29, 11:01 AM
llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp
115 ↗(On Diff #211650)

Sorry I forgot about this running for every PPC target,. That rules out the assert I mentioned.

the original code const unsigned TOCReg = PPC::X2 reserves X2 for PPC64 and PPC32, which though has no effect on 32bit mode

IIUC it seems to me you are implying setting X2 as an implicit dependence on a 32-bit instruction has no effect. I don't believe this is true. This pass only has no effect on 32-bit ELF/Darwin because hasTOCLoReloc returns false for every instruction on those platforms; None of those opcodes will be used on 32-bit ELF or any Darwin, and as we saw previously PPCII::MO_TOC_LO is only ever (currently) set on 64-bit instructions.

The way this pass runs for every platform but will not possibly be able to modify any instructions on some of those platforms is confusing. I find adding code like the below line extends that confusion, because on its own it seems to imply there are 32-bit targets other then AIX that this pass is relevant too, and for some reason on those targets X2 is a dependence even though its a 32-bit instruction.

If we want to guard against setting an implicit dependence on a platform where its not needed/expected then we need to do exactly that (likely in hasTOCReloc). Using the wrong register as a dependence isn't the way to proceed.

minor changes

Some initial comments.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5080 ↗(On Diff #212235)

Minor nit: Fix the spacing.

5083 ↗(On Diff #212235)

Move this to after dealing with some initial 32-bit cases.

5091 ↗(On Diff #212235)

Put this into a !isPPC64 block.

Xiangling_L marked 4 inline comments as done.Tue, Jul 30, 11:52 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5091 ↗(On Diff #212235)

I think the usage of replaceWithLWZtoc itself is guarded by !isPPC64, do we still need add !isPPC64 for this block? Or we can add a comment specifying it's 32bit only?

Xiangling_L marked an inline comment as done.

adjust the flow of the code

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5075 ↗(On Diff #212671)

This can be made const.

5076 ↗(On Diff #212671)

Use English "code model" or code-style CodeModel.
Use clang-format.

assert((CModel != CodeModel::Tiny || CModel != CodeModel::Kernel) &&
       "PowerPC doesn't support tiny or kernel code models.");
5090 ↗(On Diff #212671)

Should dl be captured by copy? It seems to be passed by reference to getMachineNode. Also, a SDLoc holds a DebugLoc, and DebugLoc::hasTrivialDestructor indicates that the destruction is not always free.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5084 ↗(On Diff #212671)

Missing space before LDtocCPT.

5088 ↗(On Diff #212671)

It might help to have a comment before the block to indicate that it handles the rest of the cases for the small code model.

5100 ↗(On Diff #212671)

Indentation is four spaces in from the surrounding context here (should be two).

5106 ↗(On Diff #212671)

Closing brace does not line up with the corresponding if.

5114 ↗(On Diff #212671)

Add an assertion before this:

assert(CModel != CodeModel::Small);
5114 ↗(On Diff #212671)

The English does not match the assertion. The assertion says that either we are dealing with 64-bit (ELF or AIX) or 32-bit AIX.

5118 ↗(On Diff #212671)

[ ... ] or 64-bit medium (ELF-only) or large (ELF and AIX) code model code. [ ... ]

5119 ↗(On Diff #212671)

Missing space after the period.

5124 ↗(On Diff #212671)

Please clarify the treatment of 64-bit AIX.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5132 ↗(On Diff #212671)

Remove the extra parentheses and use clang-format:

SDNode *Tmp = CurDAG->getMachineNode(
    isPPC64 ? PPC::ADDIStocHA8 : PPC::ADDIStocHA, dl, VT, TOCbase, GA);
5139 ↗(On Diff #212671)

Same comment.

SDNode *MN = CurDAG->getMachineNode(isPPC64 ? PPC::LDtocL : PPC::LWZtocL,
                                    dl, VT, GA, SDValue(Tmp, 0));
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
2671 ↗(On Diff #212671)

To be consistent with how the function parameters are declared (note the positioning of the space):

const PPCSubtarget &Subtarget =

Anyhow, I would much like to avoid having this cast here. Can this function be made a private member function of PPCTargetLowering?

2673 ↗(On Diff #212671)

This can be const.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
3169 ↗(On Diff #212671)

To be consistent with the above lines, add a space before the :.

llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp
115 ↗(On Diff #212671)

Remove the extra parentheses.

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
258 ↗(On Diff #212671)

"OSes" is the spelling used in various messages.

263 ↗(On Diff #212671)

Fix the two consecutive spaces.

llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix.ll
2 ↗(On Diff #212671)

lit can display stderr separately from stdout. If we are not checking for error messages, then redirecting stderr to stdout does not seem advantageous.

9 ↗(On Diff #212671)

Same comment.

13 ↗(On Diff #212671)

Same comment.

llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix.ll
2 ↗(On Diff #212671)

Same comment.

9 ↗(On Diff #212671)

Same comment.

13 ↗(On Diff #212671)

Same comment.

@Xiangling_L, I'd be happy to work with you on this off-list to move this along.

Xiangling_L marked 25 inline comments as done.Mon, Aug 12, 12:17 PM
Xiangling_L added inline comments.
llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix.ll
2 ↗(On Diff #212671)

We redirect stderr to stdout because the option -print-before=simple-register-coalescing is used to output the register usage details[eg. nor0] and it outputs to stderr.

Xiangling_L marked an inline comment as done.

Address latest comments

LGTM with a minor issue that can be fixed on check-in.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5076 ↗(On Diff #214691)

Minor issue: This is currently (!A || !B), but !(A || B) is meant.

jasonliu added inline comments.Wed, Aug 14, 6:59 AM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5076 ↗(On Diff #214691)

The landed commit did not address this comment. Could you do an NFC commit to address it?

Xiangling_L marked 3 inline comments as done.Wed, Aug 14, 7:28 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5076 ↗(On Diff #214691)

Oops.... I addressed it but forgot to commit it together. Will do asap.

Closed by commit rL368860: [NFC][AIX] Change assertion (authored by xiangling_liao, committed by ). · Explain WhyWed, Aug 14, 7:58 AM
This revision was automatically updated to reflect the committed changes.