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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay 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;
----------------
jsji wrote:
> 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. 
Include which change?

Shall I use `!Subtarget->isPPC64()` or `Subtarget->is32BitELFABI()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72067