[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:
> jsji wrote:
> > 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.
> > 
> I would argue that this code is dead on AIX due to the early check we have [[ https://github.com/llvm/llvm-project/blob/4117c8c0194cdf59e229f6826e0908eb3f2bcfc6/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp#L1756 | here ]] and so the change is still NFC. Even if that is being too loose about what NFC means, I don't see any value in keeping a patch NFC if it means we have to add code we know to be wrong.
:), OK. 
But since we will need another patch for AIX below, maybe it is better to include this change within that AIX patch,  they both are related to AIX logically. 


================
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
----------------
MaskRay wrote:
> jsji wrote:
> > Why we are removing whole testcase? Can we just use `llvm.sinl.ppcf128` instead?
> D50988 did
> 
> ```
> -target triple = "powerpc-apple-darwin10.0"
> +target triple = "powerpc-unknown-linux-gnu"
> ```
> 
> `*$LDBL128` is used exclusively by Darwin. We have more appropriate tests for ELF ppc_fp128 in ppc_fp128*.ll ppc_f128* and various other files.
Sounds good.. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72067