[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;
----------------
MaskRay wrote:
> 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()`?
:), I am talking to Sean. He mentioned `let me land a patch to fix the behavior with testing before we commit this one, that way we don't add purposefully wrong code in this patch.`
I meant he can change `!Subtarget->isPPC64()` to `Subtarget->is32BitELFABI()` in his AIX patch too.

Then you can rebase this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72067