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

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked an inline comment as done.
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:
> 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.
Thanks for the suggestion Jinsong.  I committed this as a standalone [[ https://reviews.llvm.org/rG1a1dbea24df51e441f3517abb8e251df0029dad7 | patch ]]. The other change is going to take longer then I expected.


================
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:
> 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.
The code is wrong for AIX. AIX has a dedicated area for saving the cr register in the linkage area similar to the 64-bit ELF ABIs. I wrote a mir test for testing crsaves, but it will crash on PowerPC64 because we create a CSRInfo with a bad frame index. Once I fix AIX's behaviour I suspect it will have the same problem. Unfortunately I'm going on vacation tomorrow and won't be able to fix this until after I get back. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72067