Page MenuHomePhabricator

[M680x0] Add ELF and Triple info
Needs ReviewPublic

Authored by m4yers on Aug 16 2018, 11:28 AM.

Details

Reviewers
kristina
Summary

This patch is part of a bigger one D50314. It adds M680x0 ELF and Triple
information.

Diff Detail

Event Timeline

m4yers created this revision.Aug 16 2018, 11:28 AM

Please add appropriate test coverage to unittests/ADT/TripleTest.cpp.

I'm not sure how ELF/relocation stuff should be tested, but i *think* it can also be split into a second review.
Though

llvm/test$ find -iname *Relocs*

does find some things.

I'm not sure how ELF/relocation stuff should be tested, but i *think* it can also be split into a second review.
Though

llvm/test$ find -iname *Relocs*

does find some things.

There are tests for relocations in the main diff D50314 test/CodeGen/M680x0/OBJ/Relocations, but those require the full backend available. Aside from that I'm not really sure how to test them in a separate diff. For now they are just data copied from the m68k manual.

Please add appropriate test coverage to unittests/ADT/TripleTest.cpp.

Will do

m4yers updated this revision to Diff 162020.Aug 22 2018, 12:03 PM

Add Triple tests

glaubitz added inline comments.Aug 30 2018, 1:57 PM
unittests/ADT/TripleTest.cpp
871

Is this a formatting error (tabs vs. spaces maybe)? The syntax highlighter shows a yellow block at the beginning of the line.

m4yers marked an inline comment as done.Sep 2 2018, 5:37 AM
m4yers added inline comments.
unittests/ADT/TripleTest.cpp
871

It seems the diff tool assumes a space was copied from line 838... anyhow, there are only spaces in this diff.

@m4yers I think you need to add reviewers here as well, no?

kristina accepted this revision.Sep 13 2018, 12:16 PM
kristina added a reviewer: kristina.

LGTM, thank you for not forgetting to update the modulemap!

This revision is now accepted and ready to land.Sep 13 2018, 12:17 PM

Do you have commit access? If not, I can land it for you, need a second to test it out with more configurations first though.

I think i lost track, was there some consensus on this 'support for m680x0' somewhere on the mailing list?
I don't dis-welcome this, just asking.

Hi Kristina!

Would be awesome if you could commit that for us!

Thanks a lot for your review!

I think i lost track, was there some consensus on this 'support for m680x0' somewhere on the mailing list?
I don't dis-welcome this, just asking.

I don't believe there was. I don't believe any objections to it were raised either however given how many diffs are blocked on this.

kristina requested changes to this revision.Sep 13 2018, 12:51 PM

Build failure, x86_64 Linux, Modular, ThinLTO, clean cache:

/SourceCache/llvm-trunk-8.0.4116/lib/MC/MCExpr.cpp:200:8: error: use of undeclared identifier 'VK_GOTPC'; did you mean 'VK_GOT'?
  case VK_GOTPC: return "GOTPC";
       ^~~~~~~~
       VK_GOT
/SourceCache/llvm-trunk-8.0.4116/include/llvm/MC/MCExpr.h:172:5: note: 'VK_GOT' declared here
    VK_GOT,
    ^
/SourceCache/llvm-trunk-8.0.4116/lib/MC/MCExpr.cpp:200:8: error: duplicate case value 'VK_GOT'
  case VK_GOTPC: return "GOTPC";
       ^
/SourceCache/llvm-trunk-8.0.4116/lib/MC/MCExpr.cpp:197:8: note: previous case defined here
  case VK_GOT: return "GOT";
       ^
/SourceCache/llvm-trunk-8.0.4116/lib/MC/MCExpr.cpp:327:20: error: use of undeclared identifier 'VK_GOTPC'; did you mean 'VK_GOT'?
    .Case("gotpc", VK_GOTPC)
                   ^~~~~~~~
                   VK_GOT
/SourceCache/llvm-trunk-8.0.4116/include/llvm/MC/MCExpr.h:172:5: note: 'VK_GOT' declared here
    VK_GOT,
    ^
3 errors generated.
This revision now requires changes to proceed.Sep 13 2018, 12:51 PM

Did you by any chance leave out a file from this patch?

Also if possible can you resubmit all of these with context, it doesn't look like they have any and it's possible patch missed something when I applied them to run test builds. Let me know when that's been done, the current patches break the build, unless it's an issue on my end with my buildbots or misapplied patch due to missing context, it should be revised.

Also, sorry for the comment flurry, but would you mind bringing this up on llvm-dev as a proposed target? There isn't anything on there, and I was too hasty to accept it since adding a new target is a fairly significant change. I'd be happy to re-review it once the build is fixed and once there's some feedback from the development mailing list.

Hi Kristina!

Thanks a lot for your comments and review!

There was actually a discussion started on llvm-dev, see: lists.llvm.org/pipermail/llvm-dev/2018-August/125317.html

Linux on m68k is very well maintained and there are also a maintained m68k port for NetBSD.

The community is very active and pretty large given the fact that the architecture is very old. But there are alternative implementations based on FPGAs and emulators, of course.

Adrian

Apologies, I couldn't find it when I tried. It seems good, and I'm happy to re-review it and land it once it's in a build-able/testable state. My only remaining issue is addressing the failed builds, if you can revise it, I'll rerun it and make sure it's all good since I understand this is a blocking patch for most of the other ones. Again I do apologize and I'm sorry it went unreviewed for so long.

No worries. Very glad it got finally picked up for review.

The original author is Artyom, so we better wait for him to rebase and fix the patches!

Again, thanks a lot for the review. Much appreciated!

Also, can I add that since this is a new target, it requires a code owner, could I ask that this is brought up with @chandlerc with regards to that on the mailing lists or even on IRC, since you will be taking responsibility for this (experimental) target and understand the implications of that?

I'd be very happy to support Artyom as a co-maintainer. I assume Artyom wants to be the code owner since he already stated, he wants to develop the code at LLVM upstream.

Before we add yet another experimental backend, I think it is somewhat important to know what the long-term user story is and the long-term maintenance story.

I'd love for some folks already active in the community, and especially actively maintaining parts of backends to have looked at the larger new backend effort here and to be supportive of it going in... Even an experimental backend adds significant maintenance burden on the larger community. We're already struggling to support and integrate the current experimental backends we have, and its not clear they are all on a healthy path to becoming non-experimental. I don't think we want eternally experimental backends, I think we should have a really clear path for backends to become 100% supported or be removed. So far, no one from the community has even commented on the llvm-dev thread for this. I think reviewing or landing patches is premature until these higher-order issues are sorted, which probably best happens on llvm-dev as the individual patches may not be a clear place to have that discussion.

Similarly, we have (and to an extent, continue to have) problems with experimental backends never having bots turned up to keep them tested and working. Is there a plan for build bots here?

I don't want to seem super negative about this particular backend -- I don't know anything about it and assume it is generally a good backend. And I have no opinions in any direction about the target. I also don't want to seem negative about the general idea of growing more backends. I think LLVM *should* be actively pursuing new backends where we have the community support for them.

However, I want to make sure that we actually have the necessary community support. Currently, I don't see any evidence of that community support on llvm-dev. I think that needs to be addressed before investing more energy in patches.

Hi Chandler!

Thanks a lot for your insight. I agree, testing and proper maintenance are crucial for a new backend and I am also very much aware of the responsibilities.

While I cannot vouch that this new backend will be perfectly maintained and issues are always fixed quickly, there is actually a build bot which regularly builds LLVM snapshots on a large number of targets and that is the build infrastructure in Debian.

As you can see here: https://buildd.debian.org/status/package.php?p=llvm-toolchain-snapshot&suite=sid , the latest SVN revision currently built is r340819, so I assume that's pretty new.

You can see the build logs by clicking the green "Installed" or "Build-Attempted" fields respectively. More build logs can be found in the right-most column.

The various branches are also being built regularly:

https://buildd.debian.org/status/package.php?p=llvm-toolchain-6.0&suite=sid
https://buildd.debian.org/status/package.php?p=llvm-toolchain-7&suite=sid

Other Linux distributions like Fedora or openSUSE are also providing publicly accessible build logs, although these distributions don't cover as many targets as Debian.

Again, I cannot promise to be the perfect maintainer, but we're doing our best in Debian downstream to find issues as quickly as possible and report them upstream as soon as they show up - and hopefully also help fixing them.

Thanks again for your input!

Adrian

m4yers updated this revision to Diff 165679.Sep 16 2018, 5:58 AM
m4yers marked an inline comment as done.

Add missed enum value

Thanks @kristina for the review, I’ve updated the diff, now it should be buildable.

@chandlerc @glaubitz I'll move this discussion to the mailing list

Hi, I'll leave it in "Needs Review" state until there's some sort of clear consensus on the mailing lists, as far as I understand it you're backed by Debian CI infra, which means you at least have buildbots to ensure this back-end doesn't break or cause regressions in other parts of LLVM. I think as far as that part goes it is fine, but as Chandler explained, it's best to have an existing non-experimental back-end endorse you or getting some form of (even informal) downstream endorsement from Canonical or Debian that can be as simple as confirming you are able and have been maintaining this back-end downstream and that it has kept pace with the current development of LLVM. And as you mentioned yourself, that part is best left to the mailing lists. I would suggest discussing this over with at least one existing non-experimental backend owner at the very least regardless, you can find the list in CODE_OWNERS.txt. If there's some community interest and some form of endorsement (or heavy endorsement from one of the major downstream vendors), I'd be happy to re-review this. Leaving it open as I'll watch the lists and see how this goes and huge apologies for my hasty initial reply.

I was so much looking forward to this getting merged :-(.

How do we move forward now?

kristina added a subscriber: asb.Sep 28 2018, 10:18 PM

I think talking to the RISC-V back-end (@asb) over email owner would be one option and seeing if he's willing to back this, along with some official endorsement from downstream. There weren't many responses on the mailing lists from within the LLVM community and while you can bring the topic up again, I think the best course of action would be getting some support from downstream showing that there is a reasonable amount of interest in this backend within the Debian community (I assume there is since your CI is based there) as well as talking to @asb and getting his endorsement (seems to be the most suitable back-end similarity-wise). Other things that would help is showing that you've been keeping it up to date with the current development trunk of LLVM (I presume it is hosted downstream with Debian?). I'm sorry, I can't really give you anymore advice than this. Chandler has outlined the issue very well, in that code owners for back-ends have a huge responsibility since LLVM development moves at a fast pace and it's their responsibility their back-end doesn't break or end up causing build failures and other disruptions because of any affiliated people being absent, at which point it may flat out get removed. Unless you have worked things out this diff isn't really the place to discuss this, but I hope the advice is of some help.

Thank you.

Hi Kristina!

I will try to get in touch with @asb.

Thanks!
Adrian

kristina resigned from this revision.Dec 6 2018, 12:51 AM