Page MenuHomePhabricator

[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

Unit TestsFailed

TimeTest
571,693 msTSAN (https://buildkite.com/llvm-project/libcxx-ci/builds/60#5f13445e-9235-4b89-94fa-a7359a8a4542)
--- Generating CMake -- The C compiler identification is Clang 11.0.0 -- The CXX compiler identification is Clang 11.0.0
2,165,932 msUBSAN (https://buildkite.com/llvm-project/libcxx-ci/builds/60#fc38c9a6-12c8-480b-8674-d791df2c0b8b)
--- Generating CMake -- The C compiler identification is Clang 11.0.0 -- The CXX compiler identification is Clang 11.0.0
1,130,832 ms-fno-exceptions (https://buildkite.com/llvm-project/libcxx-ci/builds/60#35969f53-ac5f-4841-b44a-90205ccc9156)
--- Generating CMake -- The C compiler identification is Clang 11.0.0 -- The CXX compiler identification is Clang 11.0.0
2,223,796 msASAN (https://buildkite.com/llvm-project/libcxx-ci/builds/60#3a0ba7d0-4166-4629-b0f7-75265ffb6080)
--- Generating CMake -- The C compiler identification is Clang 11.0.0 -- The CXX compiler identification is Clang 11.0.0
983,015 msApple system (https://buildkite.com/llvm-project/libcxx-ci/builds/60#3039a203-48b0-4394-8328-48ce0b272c80)
--- Generating CMake loading initial cache file /private/tmp/buildkite-agent/builds/libcxx-mbp-local-1/llvm-project/libcxx-ci/libcxx/cmake/caches/Apple.cmake -- The C compiler identification is AppleClang 12.0.0.12000032
View Full Test Results (2 Failed · 12 Passed)

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