This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Delete PPCSubtarget::isDarwin and isDarwinABI
ClosedPublic

Authored by MaskRay on Jan 1 2020, 10:54 PM.

Details

Summary

http://lists.llvm.org/pipermail/llvm-dev/2018-August/125614.html developers have agreed to remove Darwin support from POWER backends.

Diff Detail

Event Timeline

MaskRay created this revision.Jan 1 2020, 10:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 1 2020, 10:54 PM
MaskRay added a reviewer: Restricted Project.Jan 1 2020, 10:54 PM

Unit tests: pass. 61167 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

The code you are modifying should be unreachable and should be safe to remove. However, can you please provide a description with some rationale about why you are removing this now. Namely, is there a pressing concern to get this removed soon or is this just a regular cleanup?

The reason I am asking is that we have started the work to remove all Darwin code from the PPC back end but have paused it for now as there are situations where the ongoing AIX implementation benefits from seeing how things were handled in Darwin due to some ABI similarities. Of course, this is convenience rather than necessity, so if you have some reason that necessitates removal of this code now, that definitely trumps the reasons I outlined above.

In any case, I am adding @cebowleratibm to the review as he is much more familiar with AIX concerns.

MaskRay edited the summary of this revision. (Show Details)Jan 2 2020, 9:34 AM

The code you are modifying should be unreachable and should be safe to remove. However, can you please provide a description with some rationale about why you are removing this now. Namely, is there a pressing concern to get this removed soon or is this just a regular cleanup?

The reason I am asking is that we have started the work to remove all Darwin code from the PPC back end but have paused it for now as there are situations where the ongoing AIX implementation benefits from seeing how things were handled in Darwin due to some ABI similarities. Of course, this is convenience rather than necessity, so if you have some reason that necessitates removal of this code now, that definitely trumps the reasons I outlined above.

In any case, I am adding @cebowleratibm to the review as he is much more familiar with AIX concerns.

No pressing concern. This is only a regular cleanup. Added the discussion to the summary.

From http://lists.llvm.org/pipermail/llvm-dev/2018-August/125614.html

  • it looks like we're all now in agreement to move forward.

The remaining Darwin PPC code is for its reference purposes. As you can see from this patch, the code now has no test. Let's wait for @cebowleratibm's opinions.

I think we should be safe removing the Darwin code in this patch, but Chris will know better.

The clang-format and clang-tidy comments are pretty cool, but whats expected when they show up on code that wasn't changed? We have a lot of code in the PowerPC directory which will trigger these. I've never bothered with cleaning that code up because I though keeping the history was more important than fixing the formatting. If these are going to be flooding every review though maybe its better we start to fix them.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
473

if (Subtarget->is32BitELFABI() && isPositionIndependent())

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
1806

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.

2178–2179

Comment needs to be updated.

2349–2350

Comment needs to be updated.

llvm/lib/Target/PowerPC/PPCSubtarget.cpp
149

I believe the member variable HasLazyResolverStubs should be dead with the Darwin removal. Likewise for PPCSubtarget::hasLazyResolverStub.

llvm/lib/Target/PowerPC/PPCSubtarget.h
290–291

minor nit: drop the (Non-DarwinABI) from the comment.

MaskRay updated this revision to Diff 235912.Jan 2 2020, 11:16 AM
MaskRay marked 5 inline comments as done.

Address sfertile's review comments

Unit tests: pass. 61174 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

jsji added inline comments.Jan 2 2020, 12:53 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
282–283

Why we still need to call stripRegisterPrefix here? Why not just

O << "0, ";
473

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.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
1806

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.

2349–2350

Remove this as well?

llvm/test/CodeGen/PowerPC/2008-10-31-PPCF128Libcalls.ll
16

Why we are removing whole testcase? Can we just use llvm.sinl.ppcf128 instead?

MaskRay updated this revision to Diff 235936.Jan 2 2020, 1:17 PM
MaskRay marked 5 inline comments as done.

Address jsji's comments

Unit tests: pass. 61174 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay added inline comments.Jan 2 2020, 5:02 PM
llvm/lib/Target/PowerPC/PPCSubtarget.cpp
149

It looks like PPCSubtarget::hasLazyResolverStub is the only member variable that is dead now. I'll delete related code.

llvm/test/CodeGen/PowerPC/2008-10-31-PPCF128Libcalls.ll
16

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.

MaskRay updated this revision to Diff 235974.Jan 2 2020, 5:02 PM

Delete more code related to hasLazyResolverStub

Unit tests: pass. 61174 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

sfertile added inline comments.Jan 2 2020, 5:37 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
473

I would argue that this code is dead on AIX due to the early check we have 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.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
1806

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.

jsji added inline comments.Jan 7 2020, 7:47 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
473

:), 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.

llvm/test/CodeGen/PowerPC/2008-10-31-PPCF128Libcalls.ll
16

Sounds good.. Thanks.

MaskRay marked an inline comment as done.Jan 7 2020, 9:57 AM
MaskRay added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
473

Include which change?

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

jsji added inline comments.Jan 7 2020, 10:10 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
473

:), 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.

sfertile marked an inline comment as done.Jan 9 2020, 11:33 AM
sfertile added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
473

Thanks for the suggestion Jinsong. I committed this as a standalone patch. The other change is going to take longer then I expected.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
1806

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.

MaskRay updated this revision to Diff 237429.Jan 10 2020, 1:46 PM

Rebase + ping

Unit tests: pass. 61767 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

sfertile accepted this revision as: sfertile.Jan 21 2020, 7:38 AM

Thanks for doing this cleanup. LGTM.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14853–14854

Minor nit: since we are touching this switch anyway we should format it as the Lint check suggests so we don't keep getting the warning in other patches touching this file.

llvm/lib/Target/PowerPC/PPCSubtarget.h
290–291

Ditto on the formatting.

This revision is now accepted and ready to land.Jan 21 2020, 7:38 AM
MaskRay updated this revision to Diff 239344.Jan 21 2020, 9:16 AM

Rebase. clang-format

Unit tests: fail. 62057 tests passed, 1 failed and 784 were skipped.

failed: Clang.Driver/cc-print-options.c

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This revision was automatically updated to reflect the committed changes.