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

[Sean Fertile via Phabricator via llvm-commits]

sfertile 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:
> > `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.


================
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))) {
----------------
jsji wrote:
> 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.
You are right on this one. Either this code is correct for AIX and only the comment needs to be updated to reflect that, or the behavior is ELF 32-bit only and we need to update it in a separate patch. Either way we missed this in the review for our initial AIX frame lowering patch. If it is the latter 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72067