Address Nikita's feeback

• rebase
• simplify the test case
• use update_test_checks.py

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

In D115844#3625776, @vitalybuka wrote:

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

In D107695#3620822, @dyung wrote:

In D107695#3617159, @MatzeB wrote:

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!

• add release notes
• update www-status

D115844 and D116130 supersede this.

In D128750#3616471, @ychen wrote:

In D128750#3616449, @erichkeane wrote:

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.

In D128750#3616449, @erichkeane wrote:

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.

fix typo

ping.

rebase

Update LangRef.

In D127259#3590127, @rnk wrote:

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?

gentle ping..

gentle ping..

gentle ping..

lint

In D126341#3554286, @rnk wrote:

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

In D126341#3537947, @rnk wrote:

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

In D126341#3537675, @aaron.ballman wrote:

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.

rebase

In D122251#3528961, @thakis wrote:

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?

In D122251#3491815, @probinson wrote:

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).

In D122251#3490585, @nigelp-xmos wrote:

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

In D122107#3439169, @aganea wrote:

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!

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

In D123797#3472697, @lenary wrote:

@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.

In D122251#3472718, @michaelplatings wrote:

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.

In D122251#3465359, @sammccall wrote:

In D122251#3465326, @sammccall wrote:

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?

In D122251#3462980, @ahatanak wrote:

It looks like this commit is causing https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/22234/testReport/junit/ThreadSanitizer-Unit/rtl___TsanRtlTest-x86_64-Test_ThreadSanitizer/RaceWithDeadThread/ to fail. Can you take a look?

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

In D122251#3446870, @rprichard wrote:

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

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

In D122251#3428173, @thakis wrote:

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

In D122251#3428173, @thakis wrote:

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?

In D122251#3427536, @vitalybuka wrote:

In D122251#3426909, @ychen wrote:

In D122251#3426905, @browneee wrote:

https://github.com/llvm/llvm-project/commit/948f3deca91a66caf4a618f826ff6de8277bed9c

This re-land seems to have broken this buildbot: https://lab.llvm.org/buildbot/#/builders/169

Previous Good: https://lab.llvm.org/buildbot/#/builders/169/builds/7234
Broken: https://lab.llvm.org/buildbot/#/builders/169/builds/7235

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

In D122251#3426905, @browneee wrote:

https://github.com/llvm/llvm-project/commit/948f3deca91a66caf4a618f826ff6de8277bed9c

This re-land seems to have broken this buildbot: https://lab.llvm.org/buildbot/#/builders/169

Previous Good: https://lab.llvm.org/buildbot/#/builders/169/builds/7234
Broken: https://lab.llvm.org/buildbot/#/builders/169/builds/7235

In D122251#3426344, @abrachet wrote:

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

In D122251#3426198, @thakis wrote:

In D122251#3426061, @thakis wrote:

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.

In D122251#3425750, @fhahn wrote:

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.

ping..

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

• format

• move post processing method to class GoogleTest

• return test failure for shard crash (added a new test case)
• print the shard command if any tests in the shard fail.

In D122251#3414948, @rnk wrote:

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.

In D122251#3414630, @rnk wrote:

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?

In D122251#3407354, @jhenderson wrote:

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?