[PATCH] D63547: [AIX]Global Address Lowering
Sean Fertile via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 16 14:03:47 PDT 2019
sfertile added inline comments.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5070
+ const bool isPPC64 = PPCSubTarget->isPPC64();
+ const bool isELFABI = PPCSubTarget->isTargetELF();
+ const bool isAIXABI = PPCSubTarget->isAIXABI();
----------------
sfertile wrote:
> minor nit: we should probably check for SVR4 explicitly instead of if the object file format is ELF, just to be pedantic.
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();`
================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5142
+ // handle this, selecting one of LDtoc, LDtocJTI, LDtocCPT, and LDtocBA.
if (CModel != CodeModel::Medium && CModel != CodeModel::Large)
break;
----------------
sfertile wrote:
> Can we move this up either directly before or directly after the medium code model error for AIX?
>
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.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5072
+ const bool isAIXABI = PPCSubTarget->isAIXABI();
+ const bool isDarwin = PPCSubTarget->isDarwin();
+
----------------
minor nit: do we need a local for this? As far as I can tell its used just once.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5116
+ assert (TM.isPositionIndependent() &&
+ "32-bit ELF can only have TOC entries in position independant"
+ " code.");
----------------
minor nit: independant --> independent
================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5118
+ " code.");
+ // 32-bit ELF always uses a small codemodel toc access.
+ replaceWithLWZtoc(N);
----------------
minor nit: `codemodel `--> `code model`
================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5136
+
+ if (isAIXABI && CModel == CodeModel::Medium)
+ report_fatal_error("Medium code model is not supported on AIX.");
----------------
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.
================
Comment at: llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp:118
+ const bool isAIXABI = Subtarget.isAIXABI();
+ assert(((isPPC64 && isSVR4ABI) || isAIXABI) &&
+ "Only supported for 64-bit SVR4 ABI and AIX ABI under large code "
----------------
Xiangling_L wrote:
> sfertile wrote:
> > Xiangling_L wrote:
> > > sfertile wrote:
> > > > Xiangling_L wrote:
> > > > > sfertile wrote:
> > > > > > Does it help readability if we add some extra helpers to the subtarget combining ABI and pointer width checks?
> > > > > > eg `is64BitELF()`, `is32BitAIX()` etc?
> > > > > or we can do?:
> > > > >
> > > > > ```
> > > > > assert(((isPPC64 && isTargetELF()) || isAIXABI) &&...
> > > > > ```
> > > > > and
> > > > > ```
> > > > > const bool is32BitAIX = !isPPC64 && isAIXABI;
> > > > > const unsigned TOCReg = (is32BitAIX) ? PPC::X2 : PPC::R2;
> > > > > ```
> > > > Sure that works as well, stick with that for now. I think we already do a lot of checks like `isPPC64() && isSVR4ABI()` and with the addition of an AIX target we are going to end up with many more of these. I can post an NFC patch adding my suggestions and we evaluate if it helps or not in that context.
> > > Thank you for doing that, though I just realized that I shouldn't have checked `isPPC64()` here, because there does exist 32bit+non-PIC situation[eg. LowerConstantPool] will take advantage of this function. But it seems weird to me that we use R2 for 32-bit. But I guess it's better to keep it that way and make changes for AIX only?
> > I'm sorry, I'm not sure I follow.
> >
> > >... though I just realized that I shouldn't have checked isPPC64() here, because there does exist 32bit+non-PIC situation[eg. LowerConstantPool] will take advantage of this function.
> >
> > 32-bit non-PIC ELF code? That will use a LabelRef instead of a TOC entry, and the 32-bit PIC ELF code always uses a LWZtoc so there is no corresponding 'lo' instruction to have to mark an implicit dependency on. If someone modifies address lowering to add a large code model for 32-bit ELF then your assertion should fail, which is what I though you were guarding against.
> >
> > > But it seems weird to me that we use R2 for 32-bit.
> > Do you mean instead of the GlobalBaseReg? The instructions we are looking for will only exist on AIX where we use r2 as the TOC base.
> 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?
> 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.
================
Comment at: llvm/lib/Target/PowerPC/PPCTargetMachine.cpp:251
}
- if (!TT.isOSDarwin() && !JIT &&
+ if (!TT.isOSAIX() && !TT.isOSDarwin() && !JIT &&
(TT.getArch() == Triple::ppc64 || TT.getArch() == Triple::ppc64le))
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63547/new/
https://reviews.llvm.org/D63547
More information about the llvm-commits
mailing list