This is an archive of the discontinued LLVM Phabricator instance.

[M680x0] Add CycleTimer implementation for M68K architecture
ClosedPublic

Authored by myhsu on Oct 5 2020, 8:32 PM.

Details

Reviewers
lebedev.ri
Group Reviewers
Restricted Project
Summary

This is a cherrypick of the upstream fix commit ffe1342 onto
llvm/utils/benchmakr and libcxx/utils/google-benchmark.
This adds CycleTimer implementation for M680x0, and simply
uses gettimeofday to implement, same as MIPS.

Diff Detail

Event Timeline

myhsu created this revision.Oct 5 2020, 8:32 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 5 2020, 8:32 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
myhsu requested review of this revision.Oct 5 2020, 8:32 PM
lebedev.ri accepted this revision.Oct 5 2020, 11:33 PM
lebedev.ri edited the summary of this revision. (Show Details)

Thanks, LG, please feel free to commit this.
There's also one in test-suite, you might want to patch that too.

myhsu added a comment.Oct 7 2020, 2:48 PM

Thanks, LG, please feel free to commit this.
There's also one in test-suite, you might want to patch that too.

Thanks for the swift review. I've created the test-suite part review https://reviews.llvm.org/D89011

myhsu removed a reviewer: Restricted Project.Oct 7 2020, 3:02 PM
This revision is now accepted and ready to land.Oct 7 2020, 3:02 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 7 2020, 3:02 PM
This revision now requires review to proceed.Oct 7 2020, 3:02 PM

@ldionne @mclow.lists @EricWF - can someone from the libc++ group accept this to unblock it please?

I don't believe this needs libc++'s signoff, it only mechanically replicates
the exact same change to the same external dependency into all the duplicate copies of said dependency.
@myhsu i still maintain that you can just commit this now.

ldionne accepted this revision as: Restricted Project.Dec 9 2020, 6:56 AM

Indeed, this is fine without libc++'s signoff. Libc++ is added automatically because it touches files in libcxx/. Thanks for asking, though.

This revision is now accepted and ready to land.Dec 9 2020, 6:56 AM
myhsu added a comment.Dec 9 2020, 1:33 PM

I don't believe this needs libc++'s signoff, it only mechanically replicates
the exact same change to the same external dependency into all the duplicate copies of said dependency.
@myhsu i still maintain that you can just commit this now.

As @ldionne mentioned libcxx was added automatically and I couldn't remove it. Otherwise I've already committed this patch a while ago: https://github.com/llvm/llvm-test-suite/commit/25883edc2f615a84c4fa741e997904293474bbc0

I don't believe this needs libc++'s signoff, it only mechanically replicates
the exact same change to the same external dependency into all the duplicate copies of said dependency.
@myhsu i still maintain that you can just commit this now.

As @ldionne mentioned libcxx was added automatically and I couldn't remove it. Otherwise I've already committed this patch a while ago: https://github.com/llvm/llvm-test-suite/commit/25883edc2f615a84c4fa741e997904293474bbc0

Isn't that just testsuite's copy of the code from D89011? It looks like at least the libcxx parts of this patch still need committing.

myhsu added a comment.Dec 10 2020, 8:07 AM

I don't believe this needs libc++'s signoff, it only mechanically replicates
the exact same change to the same external dependency into all the duplicate copies of said dependency.
@myhsu i still maintain that you can just commit this now.

As @ldionne mentioned libcxx was added automatically and I couldn't remove it. Otherwise I've already committed this patch a while ago: https://github.com/llvm/llvm-test-suite/commit/25883edc2f615a84c4fa741e997904293474bbc0

Isn't that just testsuite's copy of the code from D89011? It looks like at least the libcxx parts of this patch still need committing.

You’re right, this a a different one. I’ll commit this shortly

myhsu closed this revision.Dec 10 2020, 9:31 AM

It turns out I've already committed this at the same time with changes in llvm-test (after @lebedev.ri approved this) and I totally forgot about it :-P
https://github.com/llvm/llvm-project/commit/bd5fe7b010ea0d6c4b1e5e3740085f5eabf7def6