[PATCH] D63547: [AIX]Global Address Lowering

Mon Jul 29 08:05:51 PDT 2019

Xiangling_L marked 2 inline comments as done.
Xiangling_L 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:
> 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63547