Page MenuHomePhabricator

[ELF][PPC] Simplify {read,write}FromHalf16
ClosedPublic

Authored by MaskRay on Jun 7 2019, 9:13 PM.

Details

Summary

I've change the variable name used in PPC64.cpp from "Instr" to "Insn"
because "Insn" seems a more common abbreviation for "instruction".

While changing PPC64.cpp relocateOne(), make R_PPC64_ADDR16_LO{_DS}
slightly more efficient by saving a read and a write for the TocOptimize
case.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jun 7 2019, 9:13 PM
Herald added a project: Restricted Project. · View Herald Transcript
ruiu accepted this revision.Jun 7 2019, 9:43 PM

LGTM

This revision is now accepted and ready to land.Jun 7 2019, 9:43 PM
This revision was automatically updated to reflect the committed changes.

@MaskRay You broke the PowerpC64 build bot, and have been asked by both the buildbot maintainer (@anil9) and me to fix it. As of this morning the bot is still failing dues to the same problem (http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/4263/steps/build-stage2-unified-tree/logs/stdio) Can you please address that before committing PPC64 changes like this. I don't understand why you would consider committing more PowerPC64 changes when the bot is in this state, rather then fixing it first.

ruiu added a comment.Jun 11 2019, 5:51 AM

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

This revision is probably not the broken change. I'll check in a minute.

ruiu added a comment.Jun 11 2019, 6:07 AM

Oh sorry I already reverted this. Once you fix the bot, feel free to resubmit (but please watch the bot after that)

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

Thanks. I tried reverting the breaking commit and a commit that built on top of it last week and was able to fix the PowerPC buildbot but broke all the X86 bots in the process. Unfortunately I don't have access to an X86 machine that can build llvm/lld right now so I backed out that change and ask @MaskRay to fix the bot.

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

This revision is probably not the broken change. I'll check in a minute.

Your right, I believe it was commits 729111cf1824159bb4dd331cab8a829eab30313f and f49f58527a6d8147524d8d6f2eb1feb70f856292 that need to be pulled out.

Can you please address that before committing PPC64 changes like this. I don't understand why you would consider committing more PowerPC64 changes when the bot is in this state, rather then fixing it first.

@sfertile @anil9 I haven't received any email from ppc64le-lld-multistage-test since May 23. I am sorry I missed Anil's earlier email on rL361830.

The old links on rL361830 are no longer available. From http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/4263/steps/build-stage2-unified-tree/logs/stdio

ld.lld: error: relocation refers to a discarded section: .rodata._ZNK4llvm3MVT13getSizeInBitsEv
>>> defined in utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o
>>> referenced by CodeGenRegisters.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o:(.toc+0x0)

I don't think this revision is responsible for the breakage.

@sfertile I find you reverted rL361830 and D62840 then restored them. I can't find the explanation why you did that. Sorry if I missed them.

AFAICT, D61583 (relanded by rL361830) caused some unhelpful errors that has been suppressed by D62840.

I'm still puzzled by the error on ppc64le because I think if my commit were bad, it should have caused other build bots to fail. May I ask you to run LLD_REPRODUCE=/tmp/reproduce.tar clang++ ... to get a reproduce tarball and upload it somewhere so that I can analyze it? Can you also check if ld.bfd or gold gives similar errors? (replace -fuse-ld=lld with -fuse-ld=bfd or -fuse-ld=gold)

@ruiu I don't think this commit is responsible for the build failure...

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

Thanks. I tried reverting the breaking commit and a commit that built on top of it last week and was able to fix the PowerPC buildbot but broke all the X86 bots in the process. Unfortunately I don't have access to an X86 machine that can build llvm/lld right now so I backed out that change and ask @MaskRay to fix the bot.

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

This revision is probably not the broken change. I'll check in a minute.

Your right, I believe it was commits 729111cf1824159bb4dd331cab8a829eab30313f and f49f58527a6d8147524d8d6f2eb1feb70f856292 that need to be pulled out.

@sfertile Sorry but I can't find commit 729111cf1824159bb4dd331cab8a829eab30313f or f49f58527a6d8147524d8d6f2eb1feb70f856292 in either llvm/llvm-project (git monorepo) or llvm-mirror/lld (git multirepo). Do you have the revision numbers, e.g. r362867 (or rL362867 rLLD362867; the latters give a clickable link on Phabricator)

@sfertile Sorry but I can't find commit 729111cf1824159bb4dd331cab8a829eab30313f or f49f58527a6d8147524d8d6f2eb1feb70f856292 in either llvm/llvm-project (git monorepo) or llvm-mirror/lld (git multirepo). Do you have the revision numbers, e.g. r362867 (or rL362867 rLLD362867; the latters give a clickable link on Phabricator)

Sorry I'm not sure where I grabbed those hashes from. The commits I pulled were https://reviews.llvm.org/rLLD362497 and https://reviews.llvm.org/rLLD361830. I verified the link failure on a local machine, and then pulling those 2 commits and rebuilding fixed the problem. I'm not sure there is a problem with your commits per se, rather it seemed we are garbage collecting a section that is referenced by toc relocations.

Can you please address that before committing PPC64 changes like this. I don't understand why you would consider committing more PowerPC64 changes when the bot is in this state, rather then fixing it first.

@sfertile @anil9 I haven't received any email from ppc64le-lld-multistage-test since May 23. I am sorry I missed Anil's earlier email on rL361830.
@sfertile I find you reverted rL361830 and D62840 then restored them. I can't find the explanation why you did that. Sorry if I missed them.

There seems to be some confusion all around, and I am sorry for that. I can see that Anils first message was before you pulled and updated your patch and we never followed up with you after your reland failed to fix the PowerPC build bot. Either way reverting those 2 patches is needed to get the bot back to green. Once they are pulled we can investigate what the underlying issue is. I'm reaching out to the current build bot monitor right now to get you the reproducer and so they can check if gold/bfd have a similar issue. (FWIW I checked the day I reverted the patches and there were no issues building with llvm and linking with gold).

@sfertile Someone gave me access to a unicamp ppc64le machine. I think I have pinpointed the root cause. D63182 has a workaround.