[PATCH] D72067: [PowerPC] Delete PPCSubtarget::isDarwin and isDarwinABI

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:488
 
-  if (!Subtarget->isPPC64() && !Subtarget->isDarwin() &&
-      isPositionIndependent())
+  if (!Subtarget->isPPC64() && isPositionIndependent())
     Kind = MCSymbolRefExpr::VK_PLT;
----------------
sfertile wrote:
> `if (Subtarget->is32BitELFABI() &&  isPositionIndependent())`
`is32BitELFABI()` is NOT equivalent of `!Subtarget->isPPC64()`. 
If we are going to do this for AIX, I would prefer we do it in another patch, not in this one.
This should be a NFC patch.



================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:299
       {
-        const char *RegName = "r0";
-        if (!Subtarget->isDarwin())
-          RegName = PPCRegisterInfo::stripRegisterPrefix(RegName);
-        O << RegName << ", ";
+        O << PPCRegisterInfo::stripRegisterPrefix("r0") << ", ";
         printOperand(MI, OpNo, O);
----------------
Why we still need to call `stripRegisterPrefix` here? Why not just 
```
O << "0, ";
```


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2351
+    // VRSAVE can appear here if, for example, @llvm.eh.unwind.init() is used.
     // Darwin, ignore it.
+    if (Reg == PPC::VRSAVE)
----------------
Remove this as well?


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:1804
   // function uses CR 2, 3, or 4.
-  if (!isPPC64 && !IsDarwinABI &&
-      (SavedRegs.test(PPC::CR2) ||
-       SavedRegs.test(PPC::CR3) ||
-       SavedRegs.test(PPC::CR4))) {
+  if (!isPPC64 && (SavedRegs.test(PPC::CR2) || SavedRegs.test(PPC::CR3) ||
+                   SavedRegs.test(PPC::CR4))) {
----------------
sfertile wrote:
> If the comment is correct, the proper check would be `if (Subtarget.is32BitELFABI() && (...))`. I believe @cebowleratibm should be able to clarify what AIX's behavior should be.
Again, I think we should do the change for AIX in another patch, don't mix a NFC patch with some functional changes for AIX.


================
Comment at: llvm/test/CodeGen/PowerPC/2008-10-31-PPCF128Libcalls.ll:16
-	%2 = load ppc_fp128, ppc_fp128* @b, align 16		; <ppc_fp128> [#uses=1]
-	%3 = call ppc_fp128 @"\01_sinl$LDBL128"(ppc_fp128 %2) nounwind readonly		; <ppc_fp128> [#uses=1]
-	store ppc_fp128 %3, ppc_fp128* @b, align 16
----------------
Why we are removing whole testcase? Can we just use `llvm.sinl.ppcf128` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72067