http://lists.llvm.org/pipermail/llvm-dev/2018-August/125614.html developers have agreed to remove Darwin support from POWER backends.
Details
- Reviewers
hfinkel jsji nemanjai kbarton cebowleratibm sfertile - Group Reviewers
Restricted Project - Commits
- rG8e1f0974c280: [PowerPC] Delete PPCSubtarget::isDarwin and isDarwinABI
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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. |
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
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(). | |
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? |
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
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. |
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
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. |
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp | ||
---|---|---|
473 | Include which change? Shall I use !Subtarget->isPPC64() or Subtarget->is32BitELFABI()? |
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. Then you can rebase this patch. |
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. |
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
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. |
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
Why we still need to call stripRegisterPrefix here? Why not just