[PATCH] D63547: [AIX]Global Address Lowering

Xiangling Liao via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 9 18:49:59 PDT 2019


Xiangling_L marked an inline comment as done.
Xiangling_L added inline comments.


================
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 "
----------------
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?


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