Page MenuHomePhabricator

ychen (Yuanfang Chen)
User

Projects

User Details

User Since
Nov 19 2013, 9:02 PM (449 w, 6 d)

Recent Activity

Yesterday

ychen added inline comments to D129025: [SimplifyCFG] Skip hoisting common instructions that return token type.
Mon, Jul 4, 3:15 PM · Restricted Project, Restricted Project
ychen updated the diff for D129025: [SimplifyCFG] Skip hoisting common instructions that return token type.

Address Nikita's feeback

Mon, Jul 4, 3:15 PM · Restricted Project, Restricted Project
ychen added inline comments to D129025: [SimplifyCFG] Skip hoisting common instructions that return token type.
Mon, Jul 4, 1:02 PM · Restricted Project, Restricted Project
ychen updated the diff for D129025: [SimplifyCFG] Skip hoisting common instructions that return token type.
  • rebase
  • simplify the test case
  • use update_test_checks.py
Mon, Jul 4, 12:48 PM · Restricted Project, Restricted Project

Fri, Jul 1

ychen updated the diff for D129025: [SimplifyCFG] Skip hoisting common instructions that return token type.

run update_test_checks.py on llvm/test/Transforms/LowerMatrixIntrinsics/transpose-opts.ll

Fri, Jul 1, 4:48 PM · Restricted Project, Restricted Project
ychen requested review of D129025: [SimplifyCFG] Skip hoisting common instructions that return token type.
Fri, Jul 1, 3:57 PM · Restricted Project, Restricted Project
ychen added a comment to D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.

@ychen This patch causes 20% .o size increase (x86_64) Is this expected?

Fri, Jul 1, 12:30 PM · Restricted Project, Restricted Project, Restricted Project

Wed, Jun 29

ychen added a comment to D107695: [llvm] [lit] Support forcing lexical test order.

Hey, I don't know if this has anything to do with this change.

But I see the "llvm-clang-x86_64-sie-win" buildbot (https://lab.llvm.org/buildbot/#/builders/216) failing the reorder.py testcase randomly every 10-20 builds.

See https://lab.llvm.org/buildbot/#/builders/216 for the list of recent builds.

I didn't look deeply into the root cause here, but thought it would be good to ping here in case anyone has an idea to get the bot more stable.

I'm the owner of that buildbot, but unfortunately have not been able to get the failure to reproduce outside of the buildbot initiated build environment, so I haven't been able to really look into the issue. Any ideas how to possibly debug and/or fix the issue would be greatly appreciated as I've been a bit stuck on how to get it more stable!

Wed, Jun 29, 5:16 PM · Restricted Project, Restricted Project

Tue, Jun 28

ychen added a reviewer for D128750: [c++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions: Restricted Project.
Tue, Jun 28, 3:33 PM · Restricted Project, Restricted Project
ychen updated the diff for D128750: [c++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions.
  • add release notes
  • update www-status
Tue, Jun 28, 3:14 PM · Restricted Project, Restricted Project
ychen abandoned D63280: [llvm-objdump] Use <first-symbol>-<offset> as the section start symbol.
Tue, Jun 28, 2:48 PM · Restricted Project, Restricted Project
ychen abandoned D114728: [Coroutine] Remove the prologue data of `-fsanitize=function` for split functions.

D115844 and D116130 supersede this.

Tue, Jun 28, 2:47 PM · Restricted Project, Restricted Project, Restricted Project
ychen added a comment to D128750: [c++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions.

Is there any chance you can validate this against https://reviews.llvm.org/D126907 as well? We touch similar code, and I'm intending to get that committed in the near future. Otherwise, after a quick look, I don't see anything particualrly bad, other than a lack of release notes and update of www-status.

Sure. I will test it.

Tue, Jun 28, 1:11 PM · Restricted Project, Restricted Project
ychen added a comment to D128750: [c++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions.

Is there any chance you can validate this against https://reviews.llvm.org/D126907 as well? We touch similar code, and I'm intending to get that committed in the near future. Otherwise, after a quick look, I don't see anything particualrly bad, other than a lack of release notes and update of www-status.

Tue, Jun 28, 12:13 PM · Restricted Project, Restricted Project
ychen requested review of D128750: [c++20] Implement P2113R0: Changes to the Partial Ordering of Constrained Functions.
Tue, Jun 28, 12:06 PM · Restricted Project, Restricted Project
ychen updated the diff for D128745: [Sema] fix trailing parameter pack handling for function template partial ordering.

fix typo

Tue, Jun 28, 11:18 AM · Restricted Project, Restricted Project
ychen requested review of D128745: [Sema] fix trailing parameter pack handling for function template partial ordering.
Tue, Jun 28, 11:11 AM · Restricted Project, Restricted Project
ychen committed rG9d37895a71cb: [lit][test] relaxed GTEST_TOTAL_SHARDS checking for some googletests (2) (authored by ychen).
[lit][test] relaxed GTEST_TOTAL_SHARDS checking for some googletests (2)
Tue, Jun 28, 10:34 AM · Restricted Project, Restricted Project

Mon, Jun 27

ychen committed rG14d3021c10d0: [lit][test] relaxed GTEST_TOTAL_SHARDS checking for some googletests (authored by ychen).
[lit][test] relaxed GTEST_TOTAL_SHARDS checking for some googletests
Mon, Jun 27, 4:17 PM · Restricted Project, Restricted Project
ychen added a comment to D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission.

ping.

Mon, Jun 27, 3:12 PM · Restricted Project, Restricted Project
ychen committed rG6e2b3cc6caac: Fix sphinx docs build (authored by ychen).
Fix sphinx docs build
Mon, Jun 27, 12:23 PM · Restricted Project, Restricted Project
ychen committed rGe2e9e708e5c2: [Coroutine] Remove the '!func_sanitize' metadata for split functions (authored by ychen).
[Coroutine] Remove the '!func_sanitize' metadata for split functions
Mon, Jun 27, 12:12 PM · Restricted Project, Restricted Project
ychen committed rG6678f8e505b1: [ubsan] Using metadata instead of prologue data for function sanitizer (authored by ychen).
[ubsan] Using metadata instead of prologue data for function sanitizer
Mon, Jun 27, 12:12 PM · Restricted Project, Restricted Project, Restricted Project
ychen closed D116130: [Coroutine] Remove the '!func_sanitize' metadata for split functions.
Mon, Jun 27, 12:12 PM · Restricted Project, Restricted Project
ychen closed D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.
Mon, Jun 27, 12:12 PM · Restricted Project, Restricted Project, Restricted Project
ychen updated the diff for D116130: [Coroutine] Remove the '!func_sanitize' metadata for split functions.

rebase

Mon, Jun 27, 11:50 AM · Restricted Project, Restricted Project
ychen updated the diff for D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.

Update LangRef.

Mon, Jun 27, 11:26 AM · Restricted Project, Restricted Project, Restricted Project
ychen updated the summary of D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.
Mon, Jun 27, 11:24 AM · Restricted Project, Restricted Project, Restricted Project

Thu, Jun 16

ychen added a reviewer for D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission: aaron.ballman.
Thu, Jun 16, 3:18 PM · Restricted Project, Restricted Project
ychen added a comment to D127259: [CodeGen] guarantee templated static variables are initialized in the reverse instantiation order.

I think Richard had some concerns in the other review that this may not be enough to really guarantee initialization order within the TU. I couldn't say either way, I shouldn't review this code. Conceptually, this change seems small enough to me. Can we ask @aaron.ballman to take a look?

Thu, Jun 16, 3:18 PM · Restricted Project, Restricted Project
ychen added a comment to D127259: [CodeGen] guarantee templated static variables are initialized in the reverse instantiation order.

gentle ping..

Thu, Jun 16, 12:10 PM · Restricted Project, Restricted Project
ychen added a comment to D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission.

gentle ping..

Thu, Jun 16, 12:09 PM · Restricted Project, Restricted Project

Mon, Jun 13

ychen added a comment to D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.

gentle ping..

Mon, Jun 13, 9:56 AM · Restricted Project, Restricted Project, Restricted Project

Thu, Jun 9

ychen added inline comments to D119296: KCFI sanitizer.
Thu, Jun 9, 6:13 PM · Restricted Project, Restricted Project, Restricted Project
ychen retitled D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission from [CodeGen] Sort llvm.global_ctors by lexing order before emission to [CodeGen] Sort llvm.global_ctors by lexical order before emission.
Thu, Jun 9, 11:31 AM · Restricted Project, Restricted Project

Tue, Jun 7

ychen updated the summary of D127259: [CodeGen] guarantee templated static variables are initialized in the reverse instantiation order.
Tue, Jun 7, 3:46 PM · Restricted Project, Restricted Project
ychen requested review of D127259: [CodeGen] guarantee templated static variables are initialized in the reverse instantiation order.
Tue, Jun 7, 3:35 PM · Restricted Project, Restricted Project
ychen updated the diff for D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission.

lint

Tue, Jun 7, 10:36 AM · Restricted Project, Restricted Project
ychen requested review of D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission.
Tue, Jun 7, 10:34 AM · Restricted Project, Restricted Project

Jun 3 2022

ychen added a comment to D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order.

Well, I guess we're out of luck, but that seems like a very poorly considered requirement from the standard. If we can't use comdats for inline variables, every time you include a header with a dynamically initialized variable, it will generate extra initialization code in every TU that cannot be optimized away. This reminds me of the problems people used to have where every TU including <iostream> emitted extra initialization code.

I think we have two options:

  1. Full conformance: Stop using comdats altogether and suffer costs in code size and startup time
  2. Partial / compromised conformance: Provide ordering guarantees between global_ctors entries so that we can ensure that inline variables in headers have the expected initialization order
Jun 3 2022, 12:58 PM · Restricted Project, Restricted Project
ychen added inline comments to D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.
Jun 3 2022, 10:20 AM · Restricted Project, Restricted Project, Restricted Project

May 25 2022

ychen added a comment to D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order.

I'm somewhat supportive of the goal here, but I think there are still some underlying issues.

First, why should these guarantees be limited to instantiations and not inline variables? Such as:

int f();
inline int gv1 = f();
inline int gv2 = gv1 + 1; // rely on previous
May 25 2022, 4:07 PM · Restricted Project, Restricted Project
ychen added a comment to D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order.

Adding the language WG as a reviewer in case others have opinions.

The underlying problem is basically wg21.link/cwg362 which has no concensus yet.

According to https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#362 this was resolved in CD1 and we track it as being not applicable to us (https://clang.llvm.org/cxx_dr_status.html#362). (Is our status actually correct for this?)

Will the reviewers be supportive if I make the original cwg362 test case work too?

To me, it depends on what it does to compile times and memory overhead for the compiler when run on large projects. If the extra tracking is cheap and doesn't really impact anything, I think it's reasonable to want to match GCC's behavior. If it turns out this is expensive, I'm less keen on matching GCC.

May 25 2022, 10:45 AM · Restricted Project, Restricted Project

May 24 2022

ychen updated the diff for D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order.

rebase

May 24 2022, 4:02 PM · Restricted Project, Restricted Project
ychen requested review of D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order.
May 24 2022, 4:01 PM · Restricted Project, Restricted Project

May 20 2022

ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

Another effect of this: If a unit test hangs forever, killing the process now no longer prints the hanging test name in the results. (Example: http://45.33.8.238/linux/76575/step_9.txt , where clangd unit tests ran for a very long time, until I killed that process.) I'm guessing it's https://github.com/llvm/llvm-project/issues/48455 since we have this known, rare, hang in those tests -- but if I didn't know about that issue, I'd be stumped.

What's the recommendation for finding out which test a test process is hanging in now?

May 20 2022, 6:17 PM · Restricted Project, Restricted Project

May 12 2022

ychen added inline comments to D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.
May 12 2022, 11:29 AM · Restricted Project, Restricted Project, Restricted Project

May 11 2022

ychen committed rG52af5df8aef7: [Driver][test] run one test in darwin-dsymutil.c for Darwin only (authored by ychen).
[Driver][test] run one test in darwin-dsymutil.c for Darwin only
May 11 2022, 2:45 PM · Restricted Project, Restricted Project

May 4 2022

ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

The buildbot totals are counting the number of shards across all unit tests. The summary at the end of the log is counting the number of test cases like before.

lit used to report (or include in the report) the number of individual test functions across all the unittests. Now I believe it is reporting the number of shards, instead? I should think the code in lit that parses googletest output should be able to extract the number of tests executed by each shard (and if not, it could be upgraded to do so). That would recover the number that people have been expecting to see (both directly from lit, from buildbots, and possibly from metrics recorded elsewhere).

May 4 2022, 11:47 AM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

This has changed test totals reported on the buildbot dashboard, so I would be interested in a comment on that.

As others have mentioned, some tests are now reported as a kind of summary. For example I used to get 1332 lines of "ADTTests" like:

LLVM-Unit :: ADT/./ADTTests/AnyTest.BadAnyCast

and now get three lines in total:

LLVM-Unit :: ADT/./ADTTests/0/3
LLVM-Unit :: ADT/./ADTTests/1/3
LLVM-Unit :: ADT/./ADTTests/2/3
(3-core VM)

Before this change, the XCore buildbot totals on the buildbot staging dashboard matched the totals at the end of the "check-all" log:

https://lab.llvm.org/staging/#/builders/145/builds/5438

But after this change, the dashboard totals do not match the totals at the end of the log. The passed total is down by 20,000 and "skipped" (skipped Google unit tests) is not reported at all:

https://lab.llvm.org/staging/#/builders/145/builds/5439

I think the buildbot dashboard report counts log lines, rather than just displaying the totals at the end of the log.

A green buildbot with the change is clang-ppc64le-linux-multistage on main buildbot master:

https://lab.llvm.org/buildbot/#/builders/121/builds/18347
https://lab.llvm.org/buildbot/#/builders/121/builds/18350

Again the number of "expected passes" does not match the end of the log, "skipped" is not on the dashboard, and the log has numbers instead of test names.

(I tried the follow-up commit but it does not give test names of passing tests.)

Is this all intended and/or expected? What do you think about the buildbot totals?

Thanks,
Nigel

May 4 2022, 10:20 AM · Restricted Project, Restricted Project

May 3 2022

ychen committed rGa61c8e1ebdff: tsan: for unittests, change to use test fixtures to clear racy stacks (authored by ychen).
tsan: for unittests, change to use test fixtures to clear racy stacks
May 3 2022, 10:18 AM · Restricted Project, Restricted Project
ychen closed D124591: tsan: for unittests, change to use test fixtures to clear racy stacks.
May 3 2022, 10:18 AM · Restricted Project, Restricted Project

Apr 28 2022

ychen added a reviewer for D124591: tsan: for unittests, change to use test fixtures to clear racy stacks: vitalybuka.
Apr 28 2022, 10:41 PM · Restricted Project, Restricted Project
ychen added a comment to D122107: [OpenMP] Add support for ompt_callback_dispatch.

Hello! It seems the loop_dispatch.c test is occasionally causing errors on one of the builders: https://lab.llvm.org/buildbot/#/builders/84/builds/22238/steps/6/logs/FAIL__libomp__loop_dispatch_c (also see the builder's history)
Anyone would have a chance to look at it please? Thanks in advance!

Apr 28 2022, 1:48 PM · Restricted Project, Restricted Project
ychen committed rG0e554ebf029f: [lit][unit] set the default result start and pid (authored by ychen).
[lit][unit] set the default result start and pid
Apr 28 2022, 1:39 PM · Restricted Project, Restricted Project

Apr 27 2022

ychen requested review of D124591: tsan: for unittests, change to use test fixtures to clear racy stacks.
Apr 27 2022, 8:24 PM · Restricted Project, Restricted Project
ychen added a comment to D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.

gentle ping. @pcc could we please move this forward if the direction looks good to you? It has been sitting for a while...

Apr 27 2022, 8:20 PM · Restricted Project, Restricted Project, Restricted Project

Apr 25 2022

ychen added a comment to D123797: [lit] Keep stdout/stderr when using GoogleTest.

@ychen it would have been nice to give me a heads up you were going to land your own implementation. Sorry I didn’t manage to add a unit test before I went on vacation.

Apr 25 2022, 12:48 PM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

But can this be reverted or made an off-by-default option until it can provide equivalent information & convenience?

I appreciate the speed improvement on Windows, but I'd like to second @sammccall's request.
Currently I'm intermittently getting the output below from my CI, and it's too opaque.

Script(shard):
--
GTEST_COLOR=no
GTEST_SHUFFLE=0
GTEST_TOTAL_SHARDS=83
GTEST_SHARD_INDEX=69
GTEST_OUTPUT=json:/path/to/clang/unittests/Tooling/./ToolingTests-Clang-Unit-54664-69-83.json
/path/to/clang/unittests/Tooling/./ToolingTests
--
shard JSON output does not exist: /path/to/clang/unittests/Tooling/./ToolingTests-Clang-Unit-54664-69-83.json

Something, somewhere is sometimes, somehow causing a crash but the information the test system provides is not helpful to figuring out what that is.
Given the choice between faster opaque tests and slower, more helpful ones I'd like the slower ones back please.

Apr 25 2022, 12:44 PM · Restricted Project, Restricted Project
ychen committed rGd3efa577f549: [lit] Keep stdout/stderr when using GoogleTest format (authored by ychen).
[lit] Keep stdout/stderr when using GoogleTest format
Apr 25 2022, 12:26 PM · Restricted Project, Restricted Project

Apr 21 2022

ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

After this change, failure details from unittests are not printed both locally and on bots: https://lab.llvm.org/buildbot/#/builders/109/builds/37071
(Assuming this is a bug that can be fixed)

Having read the change more carefully, it sounds like it's *expected* that a crashing unittest no longer prints a stack trace on stdout.

I think this is an unacceptable regression. Crashes/asserts are a common way that tests fail and the stack trace from a debug build is how we investigate them.
The rerun commands are hard to find, require manually rebuilding to iterate on, and don't filter output to the test in question as lit did (prior to this change).
Moreover, it's not possible to manually rerun the test on a bot, and many times the crash trace/assertion messages from buildbots have been *critical* to debugging failures that only occur in certain buildbot configurations (e.g. platforms I don't have access to).
Finally, some crashes are rare/nondeterministic, and so rerunning is not always a good option.

I'm not familiar enough with this part of lit to suggest solutions here.
But can this be reverted or made an off-by-default option until it can provide equivalent information & convenience?

Apr 21 2022, 12:50 PM · Restricted Project, Restricted Project

Apr 20 2022

ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.
Apr 20 2022, 1:12 PM · Restricted Project, Restricted Project

Apr 19 2022

ychen added inline comments to D122251: [lit] Use sharding for GoogleTest format.
Apr 19 2022, 8:53 PM · Restricted Project, Restricted Project

Apr 14 2022

ychen added a comment to D123797: [lit] Keep stdout/stderr when using GoogleTest.

sounds good. Could you please add a test? Probably in llvm/utils/lit/tests/googletest-format.py.

Apr 14 2022, 10:01 AM · Restricted Project, Restricted Project

Apr 12 2022

ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

I think this change broke libc++ tests. _executeScriptInternal in libcxx/utils/libcxx/test/dsl.py does not pass an argument for LitConfig's new order parameter:

rprichard@cashew:/x/llvm-upstream/out$ cmake -GNinja -S ../llvm-project/runtimes -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind" -DLIBCXXABI_USE_LLVM_UNWINDER=ON
...
rprichard@cashew:/x/llvm-upstream/out$ ninja
...
rprichard@cashew:/x/llvm-upstream/out$ ninja check-cxx
[0/1] Running libcxx tests
llvm-lit: /x/llvm-upstream/llvm-project/libcxx/utils/libcxx/test/newconfig.py:23: note: Applied 'add Lit feature target=x86_64-unknown-linux-gnu' as a result of parameter 'target_triple=x86_64-unknown-linux-gnu'
llvm-lit: /x/llvm-upstream/llvm-project/llvm/utils/lit/lit/TestingConfig.py:103: fatal: unable to parse config file '/x/llvm-upstream/out/libcxx/test/lit.site.cfg', traceback: Traceback (most recent call last):
  File "/x/llvm-upstream/llvm-project/llvm/utils/lit/lit/TestingConfig.py", line 92, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/x/llvm-upstream/out/libcxx/test/lit.site.cfg", line 21, in <module>
    libcxx.test.newconfig.configure(
  File "/x/llvm-upstream/llvm-project/libcxx/utils/libcxx/test/newconfig.py", line 22, in configure
    action.applyTo(config)
  File "/x/llvm-upstream/llvm-project/libcxx/utils/libcxx/test/dsl.py", line 400, in applyTo
    if hasCompileFlag(config, flag):
  File "/x/llvm-upstream/llvm-project/libcxx/utils/libcxx/test/dsl.py", line 64, in f
    cache[cacheKey] = function(config, *args, **kwargs)
  File "/x/llvm-upstream/llvm-project/libcxx/utils/libcxx/test/dsl.py", line 203, in hasCompileFlag
    out, err, exitCode, timeoutInfo = _executeScriptInternal(test, [
  File "/x/llvm-upstream/llvm-project/libcxx/utils/libcxx/test/dsl.py", line 80, in _executeScriptInternal
    litConfig = lit.LitConfig.LitConfig(
TypeError: __init__() missing 1 required positional argument: 'order'

FAILED: libcxx/test/CMakeFiles/check-cxx /x/llvm-upstream/out/libcxx/test/CMakeFiles/check-cxx 
cd /x/llvm-upstream/out/libcxx/test && /usr/bin/python3.9 /x/llvm-upstream/out/bin/llvm-lit -sv --show-xfail --show-unsupported /x/llvm-upstream/out/libcxx/test
ninja: build stopped: subcommand failed.

Fixed in 81b51b61f849fa91bdf5d069591

Apr 12 2022, 3:43 PM · Restricted Project, Restricted Project
ychen committed rG81b51b61f849: Fix libcxx build after cd0a5889d71c62ae7cefc (authored by ychen).
Fix libcxx build after cd0a5889d71c62ae7cefc
Apr 12 2022, 3:43 PM · Restricted Project, Restricted Project
ychen committed rGcd0a5889d71c: [Reland][lit] Use sharding for GoogleTest format (authored by ychen).
[Reland][lit] Use sharding for GoogleTest format
Apr 12 2022, 2:51 PM · Restricted Project, Restricted Project
ychen closed D122251: [lit] Use sharding for GoogleTest format.
Apr 12 2022, 2:51 PM · Restricted Project, Restricted Project

Apr 5 2022

ychen requested review of D122251: [lit] Use sharding for GoogleTest format.
Apr 5 2022, 6:45 PM · Restricted Project, Restricted Project
ychen updated the diff for D122251: [lit] Use sharding for GoogleTest format.

Thanks, everyone for the valuable feedback. I believe I addressed all of them in the updated patch.

Apr 5 2022, 6:42 PM · Restricted Project, Restricted Project
ychen committed rGc32f8f34614d: [unittests] fix intermittent SupportTests failures (authored by ychen).
[unittests] fix intermittent SupportTests failures
Apr 5 2022, 6:20 PM · Restricted Project, Restricted Project

Apr 4 2022

ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

http://45.33.8.238/win/55614/summary.html did not regress which is expected since these runs do not include unit tests.

check-llvm includes llvm's unit tests, check-lld includes lld's unit tests, check-clang includes clang's unit tests. That build does include unit tests.

If you compare the stdout:
http://45.33.8.238/linux/72772/step_7.txt
http://45.33.8.238/linux/72807/step_7.txt

Apr 4 2022, 5:18 PM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

http://45.33.8.238/win/55614/summary.html did not regress which is expected since these runs do not include unit tests.

check-llvm includes llvm's unit tests, check-lld includes lld's unit tests, check-clang includes clang's unit tests. That build does include unit tests.

If you compare the stdout:
http://45.33.8.238/linux/72772/step_7.txt
http://45.33.8.238/linux/72807/step_7.txt

then one says "30362 tests" while the other says "16250 tests" (which I think is expected since we now count number of unit test processes instead of number of tests?), so it does seem to have an effect. It's just not (much) faster.

before (~270) https://lab.llvm.org/buildbot/#/builders/216/builds/2323/steps/7/logs/stdio
after (~240) https://lab.llvm.org/buildbot/#/builders/216/builds/2356/steps/7/logs/stdio

The time fluctuates to various degrees for bots. bot 216 is relatively stable and the speedup is what I would expect for a windows machine.

Even on that it it's a 30s improvement on a bit with a 9-15 min cycle time, so a 3-5% speedup. Is that worth giving up the property that we're guaranteed we don't have ordering dependencies between tests?

Apr 4 2022, 5:11 PM · Restricted Project, Restricted Project
ychen added inline comments to D122251: [lit] Use sharding for GoogleTest format.
Apr 4 2022, 1:36 PM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

All x86 tests passed but non-x86 tests failed with

Traceback (most recent call last):
  File "/b/sanitizer-x86_64-linux-qemu/build/llvm_build0/./bin/llvm-lit", line 45, in <module>
    main(builtin_parameters)
  File "/b/sanitizer-x86_64-linux-qemu/build/llvm-project/llvm/utils/lit/lit/main.py", line 45, in main
    opts.indirectlyRunCheck)
  File "/b/sanitizer-x86_64-linux-qemu/build/llvm-project/llvm/utils/lit/lit/discovery.py", line 279, in find_tests_for_inputs
    local_config_cache, indirectlyRunCheck)[1])
  File "/b/sanitizer-x86_64-linux-qemu/build/llvm-project/llvm/utils/lit/lit/discovery.py", line 200, in getTestsInSuite
    litConfig, lc):
  File "/b/sanitizer-x86_64-linux-qemu/build/llvm-project/llvm/utils/lit/lit/formats/googletest.py", line 50, in getTestsInDirectory
    num_tests = self.get_num_tests(execpath, localConfig)
  File "/b/sanitizer-x86_64-linux-qemu/build/llvm-project/llvm/utils/lit/lit/formats/googletest.py", line 32, in get_num_tests
    out, _, exitCode = lit.util.executeCommand(cmd, env=localConfig.environment)
  File "/b/sanitizer-x86_64-linux-qemu/build/llvm-project/llvm/utils/lit/lit/util.py", line 342, in executeCommand
    env=env, close_fds=kUseCloseFDs)
  File "/usr/lib/python3.7/subprocess.py", line 775, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.7/subprocess.py", line 1522, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 8] Exec format error: '/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_aarch64_qemu/lib/scudo/standalone/tests/./ScudoUnitTest-aarch64-Test'

I suspect the bot has some setup issue. How do I find out who is the bot owner?

new code probably missed COMPILER_RT_EMULATOR

Apr 4 2022, 1:29 PM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.
Apr 4 2022, 10:40 AM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

When using result_db output from lit we are seeing

Traceback (most recent call last):
  File "/b/s/w/ir/x/w/staging/llvm_build/./bin/llvm-lit", line 50, in <module>
    main(builtin_parameters)
  File "/b/s/w/ir/x/w/llvm-llvm-project/llvm/utils/lit/lit/main.py", line 121, in main
    report.write_results(tests_for_report, elapsed)
  File "/b/s/w/ir/x/w/llvm-llvm-project/llvm/utils/lit/lit/reports.py", line 208, in write_results
    gen_resultdb_test_entry(
  File "/b/s/w/ir/x/w/llvm-llvm-project/llvm/utils/lit/lit/reports.py", line 163, in gen_resultdb_test_entry
    'start_time': datetime.datetime.fromtimestamp(start_time).isoformat() + 'Z',
TypeError: an integer is required (got type NoneType)

https://github.com/llvm/llvm-project/blob/main/llvm/utils/lit/lit/reports.py#L163

Apr 4 2022, 10:33 AM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

I don't see it discussed above: An advantage of the previous scheme is that it guarantees that there are no order dependencies between tests and that each tests passes in isolation.

The speedup is nice, but it's also somewhat small relative to total check-llvm / check-clang time. Is that worth sacrificing guaranteed independent tests for?

FWIW, I don't see a speedup on Win on my bot.
Before: http://45.33.8.238/win/55580/summary.html
After: http://45.33.8.238/win/55605/summary.html

If anything, check-clang got 30s slower.

Apr 4 2022, 10:30 AM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

Not sure if it is related, but it looks like the output strings for failed unit tests regressed over the last day, it's now something like Clangd Unit Tests :: ./ClangdTests/24/133 instead of the test name.

Apr 4 2022, 10:17 AM · Restricted Project, Restricted Project

Apr 3 2022

ychen committed rG948f3deca91a: Reland "[lit] Use sharding for GoogleTest format" (authored by ychen).
Reland "[lit] Use sharding for GoogleTest format"
Apr 3 2022, 10:37 PM · Restricted Project, Restricted Project
ychen added a reverting change for rGa87ba5c86d5d: [lit] Use sharding for GoogleTest format: rGc0f90c84b1a8: Revert "[lit] Use sharding for GoogleTest format".
Apr 3 2022, 8:05 PM · Restricted Project, Restricted Project
ychen committed rGc0f90c84b1a8: Revert "[lit] Use sharding for GoogleTest format" (authored by ychen).
Revert "[lit] Use sharding for GoogleTest format"
Apr 3 2022, 8:05 PM · Restricted Project, Restricted Project
ychen added a reverting change for D122251: [lit] Use sharding for GoogleTest format: rGc0f90c84b1a8: Revert "[lit] Use sharding for GoogleTest format".
Apr 3 2022, 8:05 PM · Restricted Project, Restricted Project
ychen committed rGa87ba5c86d5d: [lit] Use sharding for GoogleTest format (authored by ychen).
[lit] Use sharding for GoogleTest format
Apr 3 2022, 7:47 PM · Restricted Project, Restricted Project
ychen closed D122251: [lit] Use sharding for GoogleTest format.
Apr 3 2022, 7:47 PM · Restricted Project, Restricted Project

Apr 1 2022

ychen added a comment to D115844: [ubsan] Using metadata instead of prologue data for function sanitizer.

ping..

Apr 1 2022, 3:58 PM · Restricted Project, Restricted Project, Restricted Project
ychen accepted D122869: [lit] Fix setup of sanitizer environment.

I'm looking at relevant code recently. This LGTM.

Apr 1 2022, 3:56 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
ychen added inline comments to D122251: [lit] Use sharding for GoogleTest format.
Apr 1 2022, 3:28 PM · Restricted Project, Restricted Project
ychen added inline comments to D122251: [lit] Use sharding for GoogleTest format.
Apr 1 2022, 3:21 PM · Restricted Project, Restricted Project
ychen updated the diff for D122251: [lit] Use sharding for GoogleTest format.
  • format
Apr 1 2022, 3:20 PM · Restricted Project, Restricted Project
ychen updated the diff for D122251: [lit] Use sharding for GoogleTest format.
  • move post processing method to class GoogleTest
Apr 1 2022, 3:19 PM · Restricted Project, Restricted Project
ychen added inline comments to D122869: [lit] Fix setup of sanitizer environment.
Apr 1 2022, 12:47 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 31 2022

ychen requested review of D122837: [compiler-rt][lit] initialize LIT LLVMConfig instance.
Mar 31 2022, 10:29 AM · Restricted Project, Restricted Project
ychen added a reverting change for D83486: [compiler-rt] [test] Use the parent process env as base env in tests: D122837: [compiler-rt][lit] initialize LIT LLVMConfig instance.
Mar 31 2022, 10:29 AM · Restricted Project

Mar 30 2022

ychen added inline comments to D122251: [lit] Use sharding for GoogleTest format.
Mar 30 2022, 6:51 PM · Restricted Project, Restricted Project
ychen updated the diff for D122251: [lit] Use sharding for GoogleTest format.
  • return test failure for shard crash (added a new test case)
  • print the shard command if any tests in the shard fail.
Mar 30 2022, 6:51 PM · Restricted Project, Restricted Project

Mar 29 2022

ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

I should add, I think it's worth adding a test that shows what happens when a shard fails. This is doable, just have the fake gtest process exit without producing a json file.

Mar 29 2022, 12:54 PM · Restricted Project, Restricted Project
ychen added a comment to D122251: [lit] Use sharding for GoogleTest format.

I don't have time to review the code, but this seems like a good direction.

What happens if the unit test shard crashes without writing the final JSON file? Does lit handle that? Is or can it be tested, and if not, what gives you confidence that it will work?

Mar 29 2022, 12:22 PM · Restricted Project, Restricted Project
ychen added reviewers for D122251: [lit] Use sharding for GoogleTest format: rnk, mstorsjo, compnerd.
Mar 29 2022, 10:47 AM · Restricted Project, Restricted Project
ychen updated subscribers of D122251: [lit] Use sharding for GoogleTest format.

My knowledge of this area is relatively limited, so I don't feel like I have a good enough understanding of the intricacies to progress this review further. Thanks for the explanations as to why this approach is most appropriate. The real cost sounds like it's due to process spawning overhead primarily (and any startup costs within the program itself). That does make me wonder whether there's a natural way of threading things within the test programs themselves, but I suspect not as I doubt the unit tests are thread-safe. Consequently, this approach seems sound, but hopefully others with knowledge in this area might be able to chime in?

Mar 29 2022, 10:46 AM · Restricted Project, Restricted Project