[PATCH] D63547: [AIX]Global Address Lowering

Mon Jul 29 11:05:31 PDT 2019

sfertile added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCTOCRegDeps.cpp:115
+        MBB.getParent()->getSubtarget<PPCSubtarget>();
+      const bool is32BitAIX = !Subtarget.isPPC64() && Subtarget.isAIXABI();
+      const unsigned TOCReg = (!is32BitAIX) ? PPC::X2 : PPC::R2;
----------------
Xiangling_L wrote:
> Xiangling_L wrote:
> > sfertile wrote:
> > > 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.
> > 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?
> 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?
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63547/new/

https://reviews.llvm.org/D63547