Page MenuHomePhabricator

davezarzycki (David Zarzycki)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 28 2017, 11:01 AM (199 w, 7 h)

Recent Activity

Mon, Jun 7

davezarzycki added a comment to D103820: [X86] Prefer vpmovq2m over vpternlogd + vpcmpgtq.

@davezarzycki What's going on the way phab is reporting the path in Revision Contents pane? llvm/lib/Target/X86 -> i/llvm/lib/Target/X86 ?!?

Mon, Jun 7, 12:50 PM · Restricted Project
davezarzycki added a comment to D103820: [X86] Prefer vpmovq2m over vpternlogd + vpcmpgtq.

I filed a bug about the IR not being canonicalized. Let's wait for that before proceeding on generating VPMOVQ2M:

Mon, Jun 7, 11:19 AM · Restricted Project
davezarzycki added inline comments to D103820: [X86] Prefer vpmovq2m over vpternlogd + vpcmpgtq.
Mon, Jun 7, 10:29 AM · Restricted Project
davezarzycki added a comment to D103820: [X86] Prefer vpmovq2m over vpternlogd + vpcmpgtq.

Actually, wait. Something weird is going on at the mid-level. These two functions should generate the same optimized IR, right?

typedef int V __attribute__((vector_size(64)));

V lt_zero_x_y(V mask, V x, V y) { return mask <  0 ? x : y; }
V ge_zero_y_x(V mask, V x, V y) { return mask >= 0 ? y : x; }

I'm not sure we canonicalize select operand order.

vpmovq2m only exists with avx512dq. Is this transform still useful with just avx512f?

Mon, Jun 7, 10:14 AM · Restricted Project
davezarzycki added a comment to D103820: [X86] Prefer vpmovq2m over vpternlogd + vpcmpgtq.

Actually, wait. Something weird is going on at the mid-level. These two functions should generate the same optimized IR, right?

Mon, Jun 7, 9:46 AM · Restricted Project
davezarzycki updated the diff for D103820: [X86] Prefer vpmovq2m over vpternlogd + vpcmpgtq.
Mon, Jun 7, 9:17 AM · Restricted Project
davezarzycki added a comment to D103820: [X86] Prefer vpmovq2m over vpternlogd + vpcmpgtq.

I'm surprised that this didn't break existing tests. Where is the right place to update the test suite?

Mon, Jun 7, 8:27 AM · Restricted Project
davezarzycki requested review of D103820: [X86] Prefer vpmovq2m over vpternlogd + vpcmpgtq.
Mon, Jun 7, 8:26 AM · Restricted Project

Fri, May 28

davezarzycki added a comment to D103128: [GlobalISel] Don't emit lost debug location remarks when legalizing tail calls.

https://bugs.llvm.org/show_bug.cgi?id=50516

Fri, May 28, 3:33 AM · Restricted Project

Thu, May 27

davezarzycki added a comment to D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..

LGTM but I don't feel ownership over this code.

Thu, May 27, 12:05 PM · Restricted Project
davezarzycki added a comment to D103128: [GlobalISel] Don't emit lost debug location remarks when legalizing tail calls.

Sorry for the noise. I reran the bisect. The test in question is still failing, but it turns out to be failing unreliably so git-blame is fairly useless without a reliable test.

Thu, May 27, 10:33 AM · Restricted Project
davezarzycki updated subscribers of D103128: [GlobalISel] Don't emit lost debug location remarks when legalizing tail calls.

I have an auto-bisecting multi-stage cron job that has identified this change as the source of a regression. Does the following seem plausible?

Thu, May 27, 6:40 AM · Restricted Project

Tue, May 25

davezarzycki added a comment to D101970: [X86FixupLEAs] Transform the sequence LEA/SUB to SUB/SUB.

I've verified that this patch no longer causes my multi-stage cron job to regress. Thanks!

Tue, May 25, 12:38 PM · Restricted Project
davezarzycki added a comment to D101970: [X86FixupLEAs] Transform the sequence LEA/SUB to SUB/SUB.

Actually, it is more complicated, and you're right that it also can involve overflow. I'm not sure if this is a trivial optimization when dependent comparisons are involved.

Tue, May 25, 5:09 AM · Restricted Project
davezarzycki added a comment to D101970: [X86FixupLEAs] Transform the sequence LEA/SUB to SUB/SUB.

I can check it, but I'm a bit concerned about the comment and what it implies. The problem is NOT overflow, but that comparison operations need to be redistributed during this optimization (or dependent comparisons need to disable the optimization). So in pseudocode, the original code that miscompiled looks like this:

Tue, May 25, 4:55 AM · Restricted Project

May 19 2021

davezarzycki added a comment to D101970: [X86FixupLEAs] Transform the sequence LEA/SUB to SUB/SUB.

Thanks! Also, I feel fairly confident about this. My auto-bisecting cron job is really paranoid. For example, it always does a multi-stage test to verify that the commit *before* the commit blamed by git-bisect actually works. That's what I was waiting for. Also, I then verified by trying to move one commit ahead in the history to this commit to get an A/B test and I then confirmed that this is the regression. Thanks again for reverting this.

May 19 2021, 7:11 AM · Restricted Project
davezarzycki added a comment to D101970: [X86FixupLEAs] Transform the sequence LEA/SUB to SUB/SUB.

Verified. Second stage miscompile. If it matters (and it sometimes does), my first stage is built without asserts.

May 19 2021, 5:40 AM · Restricted Project
davezarzycki added a comment to D101970: [X86FixupLEAs] Transform the sequence LEA/SUB to SUB/SUB.

Lots of crashes like this:

FAIL: Clang-Unit :: Format/./FormatTests/FormatTest.FormatsCompactNamespaces (23265 of 76779)
******************** TEST 'Clang-Unit :: Format/./FormatTests/FormatTest.FormatsCompactNamespaces' FAILED ********************
Note: Google Test filter = FormatTest.FormatsCompactNamespaces
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.FormatsCompactNamespaces
FormatTests: /home/dave/ro_s/lp/clang/lib/Basic/SourceManager.cpp:865: clang::FileID clang::SourceManager::getFileIDLoaded(unsigned int) const: Assertion `0 && "Invalid SLocOffset or bad function choice"' failed.
 #0 0x0000000000604313 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x604313)
 #1 0x0000000000602962 llvm::sys::RunSignalHandlers() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x602962)
 #2 0x0000000000604d8a SignalHandler(int) Signals.cpp:0:0
 #3 0x00007ffff7fa4a20 (/lib64/libpthread.so.0+0x13a20)
 #4 0x00007ffff7b392a2 raise (/lib64/libc.so.6+0x3d2a2)
 #5 0x00007ffff7b228a4 abort (/lib64/libc.so.6+0x268a4)
 #6 0x00007ffff7b22789 (/lib64/libc.so.6+0x26789)
 #7 0x00007ffff7b31a16 (/lib64/libc.so.6+0x35a16)
 #8 0x000000000064922c clang::SourceManager::getFileIDLoaded(unsigned int) const (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x64922c)
 #9 0x00000000003e734c clang::SourceManager::getDecomposedLoc(clang::SourceLocation) const CleanupTest.cpp:0:0
#10 0x000000000064b231 clang::SourceManager::isBeforeInTranslationUnit(clang::SourceLocation, clang::SourceLocation) const (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x64b231)
#11 0x000000000066c05e clang::format::AffectedRangeManager::computeAffectedLines(llvm::SmallVectorImpl<clang::format::AnnotatedLine*>&) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x66c05e)
#12 0x00000000006ae393 clang::format::UsingDeclarationsSorter::analyze(clang::format::TokenAnnotator&, llvm::SmallVectorImpl<clang::format::AnnotatedLine*>&, clang::format::FormatTokenLexer&) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x6ae393)
#13 0x0000000000690d1a clang::format::TokenAnalyzer::process() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x690d1a)
#14 0x0000000000665643 std::__1::__function::__func<clang::format::internal::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus*)::$_4, std::__1::allocator<clang::format::internal::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus*)::$_4>, std::__1::pair<clang::tooling::Replacements, unsigned int> (clang::format::Environment const&)>::operator()(clang::format::Environment const&) Format.cpp:0:0
#15 0x0000000000655d6e clang::format::internal::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus*) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x655d6e)
#16 0x000000000065640f clang::format::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, llvm::StringRef, clang::format::FormattingAttemptStatus*) (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x65640f)
#17 0x00000000003ef27f clang::format::(anonymous namespace)::FormatTest::format(llvm::StringRef, clang::format::FormatStyle const&, clang::format::(anonymous namespace)::FormatTest::StatusCheck) FormatTest.cpp:0:0
#18 0x0000000000407eed clang::format::(anonymous namespace)::FormatTest_FormatsCompactNamespaces_Test::TestBody() FormatTest.cpp:0:0
#19 0x000000000060cc53 testing::Test::Run() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x60cc53)
#20 0x000000000060dc16 testing::TestInfo::Run() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x60dc16)
#21 0x000000000060e470 testing::TestSuite::Run() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x60e470)
#22 0x000000000061bb23 testing::internal::UnitTestImpl::RunAllTests() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x61bb23)
#23 0x000000000061b59d testing::UnitTest::Run() (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x61b59d)
#24 0x000000000060555b main (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x60555b)
#25 0x00007ffff7b23b75 __libc_start_main (/lib64/libc.so.6+0x27b75)
#26 0x00000000003ddd8e _start (/tmp/_update_lc/t/tools/clang/unittests/Format/./FormatTests+0x3ddd8e)
May 19 2021, 4:41 AM · Restricted Project
davezarzycki added a comment to D101970: [X86FixupLEAs] Transform the sequence LEA/SUB to SUB/SUB.

Just a heads up, my auto-bisecting multi-stage cron job has identified this change as the source of a second stage regression in a bunch of clang unit tests. I'm about to verify by hand, but it'll take a while.

May 19 2021, 4:28 AM · Restricted Project

May 17 2021

davezarzycki added a comment to D100043: [lit] Fix compatibility with upstream gtest.

LGTM but I don't feel authoritative for the lit code.

May 17 2021, 4:28 AM · Restricted Project

May 6 2021

davezarzycki added a comment to D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed.

Yes, Fedora 34 (x86-64).

May 6 2021, 2:38 PM · Restricted Project

May 4 2021

davezarzycki committed rG05146fe51710: [llvm] Unbreak no-assertion testing (authored by davezarzycki).
[llvm] Unbreak no-assertion testing
May 4 2021, 3:06 AM

Apr 29 2021

davezarzycki committed rG3eb2be67b997: Unbreak no-asserts testing (authored by davezarzycki).
Unbreak no-asserts testing
Apr 29 2021, 7:19 AM

Apr 27 2021

davezarzycki committed rG646b007d1128: [llvm] Unbreak no-asserts testing after 18839be9c5c8b9f882dd241769784035b082d4e1 (authored by davezarzycki).
[llvm] Unbreak no-asserts testing after 18839be9c5c8b9f882dd241769784035b082d4e1
Apr 27 2021, 2:47 AM

Apr 23 2021

davezarzycki added a comment to D101180: Mark type test intrinsics as speculatable to fix inline cost.

I have an auto bisecting cron job that has twice identified this change as the source of a regression when building release (no asserts). Is the following expected or can we get a quick fix?

Apr 23 2021, 3:18 PM · Restricted Project
davezarzycki added a comment to D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed.

It's still failing but I can disable HSA on this machine for now. FYI --

FAILED: tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o
/p/tllvm/bin/clang++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/amdgpu-arch -I/home/dave/ro_s/lp/clang/tools/amdgpu-arch -I/home/dave/ro_s/lp/clang/include -Itools/clang/include -Iinclude -I/home/dave/ro_s/lp/llvm/include -Werror=switch -Wno-deprecated-copy -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O2 -DNDEBUG  -march=skylake -fno-vectorize -fno-slp-vectorize -fno-tree-slp-vectorize -fno-exceptions -fno-rtti -std=c++14 -MD -MT tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o -MF tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o.d -o tools/clang/tools/amdgpu-arch/CMakeFiles/amdgpu-arch.dir/AMDGPUArch.cpp.o -c /home/dave/ro_s/lp/clang/tools/amdgpu-arch/AMDGPUArch.cpp
/home/dave/ro_s/lp/clang/tools/amdgpu-arch/AMDGPUArch.cpp:14:10: fatal error: 'hsa.h' file not found
#include <hsa.h>
         ^~~~~~~
1 error generated.
Apr 23 2021, 6:33 AM · Restricted Project

Apr 22 2021

davezarzycki added a comment to D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed.

I removed all HSA/ROCM via the package manager and everything is fine now. Have you considered using __has_include() instead of CMake? It seems to be supported by all contemporary compilers, even MSVC.

Apr 22 2021, 3:20 PM · Restricted Project
davezarzycki added a comment to D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed.

It's not obvious to me why any of those stages would get a different result for the search for rocr. Do you do things with chroot/jails to ensure isolation for some of them?

Apr 22 2021, 12:11 PM · Restricted Project
davezarzycki added a comment to D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed.

That's interesting. The various permutations I have on disk are also all path/include/hsa/hsa.h. The existing in tree use of hsa.h is the amdgpu plugin, which uses #include "hsa.h" and #include <hsa.h>, which seems unlikely to be correct.

I'm going to patch this on the fly to hsa/hsa.h as that looks very likely to be the fix.

edit: or not, the include path cmake summons on my local system is -isystem $HOME/llvm-install/include/hsa, so hsa/hsa doesn't work.

Tiresome. Will revert this for now.

Apr 22 2021, 11:41 AM · Restricted Project
davezarzycki added a comment to D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed.

And if it helps:

Apr 22 2021, 11:16 AM · Restricted Project
davezarzycki added a comment to D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed.

This change broke multi-stage builds (even if AMDGPU is disabled as a target). The problem is that clang/tools/amdgpu-arch/AMDGPUArch.cpp can't find hsa.h. Is there a quick fix?

That should not be the case. The cmake for building AMDGPUArch.cpp is guarded by

find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm)
if (NOT ${hsa-runtime64_FOUND})
    return

The intent is for this to be built only when hsa-runtime64 is found on disk, at which point hsa.h is meant to be found within that package via cmake magic. Perhaps we need to explicitly specify the include directory, though we do not do so in the openmp plugin this was copied from, and I do not have hsa.h on any global search path.

Little harm will be done by reverting this (again...) and reapplying, but it would be really helpful if you can share some more information about the build config that is failing.

Apr 22 2021, 11:14 AM · Restricted Project
davezarzycki added a comment to D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed.

This change broke multi-stage builds (even if AMDGPU is disabled as a target). The problem is that clang/tools/amdgpu-arch/AMDGPUArch.cpp can't find hsa.h. Is there a quick fix?

Apr 22 2021, 10:21 AM · Restricted Project
davezarzycki added a comment to D100073: [libcxx][iterator] adds `std::indirectly_readable` and `std::indirectly_writable`.

I'm the third person to notice the same problems, but this time on Fedora 34 (x86-64) so I reverted the change.

Apr 22 2021, 6:52 AM · Restricted Project
davezarzycki added a reverting change for rG04733181b513: [libcxx][iterator] adds `std::indirectly_readable` and `std…: rGa9f11cc0d965: Revert "[libcxx][iterator] adds `std::indirectly_readable` and `std….
Apr 22 2021, 6:50 AM
davezarzycki committed rGa9f11cc0d965: Revert "[libcxx][iterator] adds `std::indirectly_readable` and `std… (authored by davezarzycki).
Revert "[libcxx][iterator] adds `std::indirectly_readable` and `std…
Apr 22 2021, 6:50 AM
davezarzycki added a reverting change for D100073: [libcxx][iterator] adds `std::indirectly_readable` and `std::indirectly_writable`: rGa9f11cc0d965: Revert "[libcxx][iterator] adds `std::indirectly_readable` and `std….
Apr 22 2021, 6:50 AM · Restricted Project
davezarzycki added a comment to D100876: [libc++] [test] Remove epicyclic workarounds for vector/span; use T[] or std::array.

My mistake. The bisect tripped over a unreliable test. The real problem was 04733181b5136e2b3df2b37c6bdd9e25f8afecd0 and people are already piling onto that as being broken.

Apr 22 2021, 6:38 AM · Restricted Project
davezarzycki added a comment to D100876: [libc++] [test] Remove epicyclic workarounds for vector/span; use T[] or std::array.

My automatically bisecting cron job identified this commit as the source of a regression on Fedora 34 (x86-64). It seems like a few dozen libc++ tests are now failing with errors like this:

Apr 22 2021, 5:12 AM · Restricted Project

Apr 15 2021

davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Something related to the time recording seems to fail intermittently on buildbots: https://lab.llvm.org/buildbot#builders/93/builds/2697

I'll cut to the fix here: @davezarzycki is there any reason why the timing data is stored in a CSV format, and not JSON?
The fix is to switch to latter.

Apr 15 2021, 7:28 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Apr 12 2021

davezarzycki added a comment to D91093: [X86][PPC] Tune SelectionDAG CTPOP emulation via TLI hook.

Sorry, I got distracted. If it were up to me, I'd prefer tuning numbers that are, without checking, unambiguously better than the current code gen. In particular the code gen for KNL is awful. Furthermore, I think that fine tuning this further has diminishing returns for various reasons. Are people okay with that?

Apr 12 2021, 5:25 AM · Restricted Project

Apr 6 2021

davezarzycki added a comment to D98884: [IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction.

These failures were not caught by LLVM bots which run on a patch. Can I do a trial run of the bots and run all tests that would run when the patch lands?

Apr 6 2021, 12:54 PM · Restricted Project
davezarzycki added a comment to D98884: [IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction.

I have an auto-bisecting cron job that has identified this change as a second-stage regression on Fedora 34 (x86-64). Basically llvm-tblgen crashes reliably:

FAILED: lib/Target/X86/X86GenDAGISel.inc
cd /tmp/_update_lc/t && /tmp/_update_lc/t/bin/llvm-tblgen -gen-dag-isel -I /home/dave/ro_s/lp/llvm/lib/Target/X86 -I/tmp/_update_lc/t/include -I/home/dave/ro_s/lp/llvm/include -I /home/dave/ro_s/lp/llvm/lib/Target -omit-comments /home/dave/ro_s/lp/llvm/lib/Target/X86/X86.td --write-if-changed -o lib/Target/X86/X86GenDAGISel.inc -d lib/Target/X86/X86GenDAGISel.inc.d
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.  Program arguments: /tmp/_update_lc/t/bin/llvm-tblgen -gen-dag-isel -I /home/dave/ro_s/lp/llvm/lib/Target/X86 -I/tmp/_update_lc/t/include -I/home/dave/ro_s/lp/llvm/include -I /home/dave/ro_s/lp/llvm/lib/Target -omit-comments /home/dave/ro_s/lp/llvm/lib/Target/X86/X86.td --write-if-changed -o lib/Target/X86/X86GenDAGISel.inc -d lib/Target/X86/X86GenDAGISel.inc.d
#0 0x000000000050b153 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/tmp/_update_lc/t/bin/llvm-tblgen+0x50b153)
#1 0x00000000005094b2 llvm::sys::RunSignalHandlers() (/tmp/_update_lc/t/bin/llvm-tblgen+0x5094b2)
#2 0x000000000050b4fa SignalHandler(int) Signals.cpp:0:0
#3 0x00007ffff7fa6a00 __restore_rt sigaction.c:0:0
#4 0x0000000000316a7d (/tmp/_update_lc/t/bin/llvm-tblgen+0x316a7d)
Apr 6 2021, 7:42 AM · Restricted Project
davezarzycki accepted D99858: [lit testing] Fix xfail-cl.py test worker count.

Good catch! Thanks

Apr 6 2021, 2:08 AM · Restricted Project

Mar 25 2021

davezarzycki added inline comments to D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin.
Mar 25 2021, 3:20 PM · Restricted Project

Mar 24 2021

davezarzycki added a comment to D97913: [runtimes] Use add_lit_testsuite to register lit testsuites.

Why do you use these settings?

-DLIBCXX_ENABLE_SHARED=FALSE
-DLIBCXXABI_ENABLE_SHARED=FALSE
-DLIBUNWIND_ENABLE_SHARED=FALSE

What is the intent of your build job?

I try to ensure that building/testing works with PIC disabled, which (of course) requires disabling shared libraries.

@davezarzycki The problem here is that this change enabled the libc++ and libc++abi tests in your job, which were not being run before. I think there might be issues with the configuration you're using and this change only makes those issues apparent.

Mar 24 2021, 8:07 AM · Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin.

The tests are failing because Dave's bot is running without enabled Python. The same is true for the Windows bot. Putting the plugin behind #ifdef LLDB_ENABLE_PYTHON should fix this.

Mar 24 2021, 7:53 AM · Restricted Project
davezarzycki added a comment to D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin.

I've reverted this. If you need help debugging this, please let me know. Also, as a reminder, please see the attached build log from earlier in the conversation. Thanks!

Mar 24 2021, 4:10 AM · Restricted Project
davezarzycki added a reverting change for rGdd391e1ef762: [lldb/Plugins] Add ScriptedProcess Process Plugin: rG952bc6c92e21: Revert "[lldb/Plugins] Add ScriptedProcess Process Plugin".
Mar 24 2021, 4:08 AM
davezarzycki committed rG952bc6c92e21: Revert "[lldb/Plugins] Add ScriptedProcess Process Plugin" (authored by davezarzycki).
Revert "[lldb/Plugins] Add ScriptedProcess Process Plugin"
Mar 24 2021, 4:08 AM
davezarzycki added a reverting change for D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin: rG952bc6c92e21: Revert "[lldb/Plugins] Add ScriptedProcess Process Plugin".
Mar 24 2021, 4:08 AM · Restricted Project

Mar 23 2021

davezarzycki added a comment to D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin.

An auto-bisecting cron job has identified this change as a regression on Fedora 33 (x86-64). Can we get a quick fix or revert this? Here is the build output:

Mar 23 2021, 6:52 PM · Restricted Project

Mar 22 2021

davezarzycki resigned from D99073: [lit] Reliable progress indicator and ETA.

Interesting. Thanks for looking into this and building on D98179. I'm sure that people will appreciate this change. :-)

Mar 22 2021, 6:43 AM · Restricted Project

Mar 20 2021

davezarzycki added a comment to rG243333ef3ec6: Revert "[Driver] Drop obsoleted Ubuntu 11.04 gcc detection".

Adding -stdlib=libstdc++ fixes it. Clang can default to libcxx so if the C++ standard library matters then the test needs to be explicit.

Mar 20 2021, 10:59 AM
davezarzycki added a comment to rG243333ef3ec6: Revert "[Driver] Drop obsoleted Ubuntu 11.04 gcc detection".

The very first RUN line seems to fail. Here is the output, sans FileCheck:

Mar 20 2021, 10:29 AM
davezarzycki added a comment to rG243333ef3ec6: Revert "[Driver] Drop obsoleted Ubuntu 11.04 gcc detection".

This seems a misfire. bed9933a461e7b3d0c8c5a8fa770aa1b49802660 had fixed the failure.

Mar 20 2021, 10:26 AM
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Why are timeouts important? Our use case is running Alive2 with the test suite. Alive2 is heavy machinery and runs into timeouts. Running the tests in roughly the same order every time is important to avoid timeout tests flipping to failed or vice-versa. Plus slow tests = tests that consume a lot of memory (in our scenario), so we can't bundle slow tests together.
Adding a --ignore-timing-data would be great, yes! But I still feel that sorting the list of failed tests is important for user experience. I diff these all the time.

That still sounds incredibly brittle. If there is any variety in test machine performance, then any and all attempts at sorting should be futile because the underlying hardware will perturb different timeouts. Is this not your experience? How do you reconcile hardware performance and configuration details (like SMT) with timeout settings?

Of course it's brittle :) Changing from a time-based setting to a ticks-based system is ongoing work, such that resource exhaustion becomes deterministic.
Nevertheless, on a same machine, we don't see many test flips. It's quite stable most of the times (just one test flip once in a while).

Mar 20 2021, 7:43 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

I'm talking about sorting just the summary of failed tests, not the whole output. We need the whole -vv output, but that can be out of order.

Why are timeouts important? Our use case is running Alive2 with the test suite. Alive2 is heavy machinery and runs into timeouts. Running the tests in roughly the same order every time is important to avoid timeout tests flipping to failed or vice-versa. Plus slow tests = tests that consume a lot of memory (in our scenario), so we can't bundle slow tests together.
Adding a --ignore-timing-data would be great, yes! But I still feel that sorting the list of failed tests is important for user experience. I diff these all the time.

Mar 20 2021, 5:13 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

I'm talking about sorting just the summary of failed tests, not the whole output. We need the whole -vv output, but that can be out of order.

Aha! +1 then, i think sorting summary should be both easy and nice to have.

Mar 20 2021, 4:55 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki committed rG5cbe2279f723: [lit] Sort testing summary output (authored by davezarzycki).
[lit] Sort testing summary output
Mar 20 2021, 4:55 AM
davezarzycki added a reverting change for rGbdf39e6b0ed4: [Driver] Drop obsoleted Ubuntu 11.04 gcc detection: rG243333ef3ec6: Revert "[Driver] Drop obsoleted Ubuntu 11.04 gcc detection".
Mar 20 2021, 4:31 AM
davezarzycki committed rG243333ef3ec6: Revert "[Driver] Drop obsoleted Ubuntu 11.04 gcc detection" (authored by davezarzycki).
Revert "[Driver] Drop obsoleted Ubuntu 11.04 gcc detection"
Mar 20 2021, 4:31 AM
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Can we revert to the previous behavior please? The current behavior is not user friendly. Thanks!

To clarify: you care about the order in the final summary, not the actual execution order, right? (the goal of this patch is the latter, if it changes the former this is just a side-effect I believe)

I don't love that the default test order was changed. For our use case that isn't good, but I can just delete the .lit_test_times.txt file and get the old fixed order. (the reason why having a fixed order is good is because of timeouts. Also, slow tests often consume more memory in our setting, so it's not good to run all the slow tests at the same time).

Mar 20 2021, 4:05 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

No, I'm running lit and dumping into a file. sorting is not as easy as piping through sort, as I'm running with -vv (required).

Since tests run in parallel, don't you already have some non-determinism in the -vv output? Isn't it printing as test finish? (just curious, not pushing back on anything here).

Mar 20 2021, 3:45 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 19 2021

davezarzycki added a comment to D97913: [runtimes] Use add_lit_testsuite to register lit testsuites.

Why do you use these settings?

-DLIBCXX_ENABLE_SHARED=FALSE
-DLIBCXXABI_ENABLE_SHARED=FALSE
-DLIBUNWIND_ENABLE_SHARED=FALSE

What is the intent of your build job?

Mar 19 2021, 4:19 AM · Restricted Project, Restricted Project, Restricted Project

Mar 17 2021

davezarzycki added inline comments to D98767: [lit] Harmonize test timing data between Unix and Windows.
Mar 17 2021, 12:12 PM · Restricted Project
davezarzycki committed rG2b20df2d798a: [lit] Harmonize test timing data between Unix and Windows (authored by davezarzycki).
[lit] Harmonize test timing data between Unix and Windows
Mar 17 2021, 4:43 AM
davezarzycki closed D98767: [lit] Harmonize test timing data between Unix and Windows.
Mar 17 2021, 4:43 AM · Restricted Project
davezarzycki requested review of D98767: [lit] Harmonize test timing data between Unix and Windows.
Mar 17 2021, 2:53 AM · Restricted Project

Mar 16 2021

davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

I've run out of time for today, but for the record, I think we should use slash as the canonical separator for the timing data file. The actual separator doesn't matter because these strings are never used as paths, just as keys into a dictionary. (Therefore any character would work.) This would let us harmonize the Unix and Windows timing data files.

Mar 16 2021, 10:37 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Ah yes, of course. I've marked that test as unavailable on Window for now: 61ca706461c5e1edc18526c9ddc3250fe074ed94

Mar 16 2021, 7:55 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki committed rG61ca706461c5: [lit testing] Mark reorder.py as unavailable on Windows (authored by davezarzycki).
[lit testing] Mark reorder.py as unavailable on Windows
Mar 16 2021, 7:54 AM
davezarzycki committed rG49d0e115d5df: [lit testing] Fix Windows reliability? (authored by davezarzycki).
[lit testing] Fix Windows reliability?
Mar 16 2021, 6:12 AM
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Try: 49d0e115d5df40aa89339f4ace7a8dee378c03bb

Mar 16 2021, 6:12 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Said differently, the Unix side seems to always run the lit tests from a fresh/clean setup, even for incremental builds. If Windows tests lit differently then ya, things will blow up.

Mar 16 2021, 6:06 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

That implies a different testing environment. How different is the Windows testing environment compared to Unix? Do you specifically have a custom testing setup?

Mar 16 2021, 6:05 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki committed rG0fda5e844128: [llvm-exegesis testing] Workaround unreliable test (authored by davezarzycki).
[llvm-exegesis testing] Workaround unreliable test
Mar 16 2021, 5:00 AM
davezarzycki committed rG1d297f90649d: [lit] Sort test start times based on prior test timing data (authored by davezarzycki).
[lit] Sort test start times based on prior test timing data
Mar 16 2021, 2:23 AM
davezarzycki closed D98179: [lit] Sort test start times based on prior test timing data.
Mar 16 2021, 2:23 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki updated the diff for D98179: [lit] Sort test start times based on prior test timing data.

Removed the commented only-failures and fixed a nit before committing.

Mar 16 2021, 2:21 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 15 2021

davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Might somebody be willing to sign off on this change (this week or next)? I'd like to cherry-pick it to Swift's LLVM branch. Thanks for all the feedback so far.

Mar 15 2021, 7:55 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 12 2021

davezarzycki updated the diff for D98179: [lit] Sort test start times based on prior test timing data.

I fixed the call to split as requested. Being unfamiliar with python, I didn't know that it supported keyword arguments / parameter labels. Thanks.

Mar 12 2021, 3:53 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki updated the diff for D98179: [lit] Sort test start times based on prior test timing data.

I've made all of the requested changes to date. Two notes:

Mar 12 2021, 3:16 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 10 2021

davezarzycki removed 1 blocking reviewer(s) for D98179: [lit] Sort test start times based on prior test timing data: Restricted Project.

I'm downgrading libcxx as a reviewer to non-blocking because the latest patch no longer requires changes to their project.

Mar 10 2021, 7:30 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki updated the diff for D98179: [lit] Sort test start times based on prior test timing data.

I renamed the file variable as requested.

Mar 10 2021, 7:27 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki updated the diff for D98179: [lit] Sort test start times based on prior test timing data.

I believe I have addressed all of the feedback to date.

Mar 10 2021, 4:23 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 9 2021

davezarzycki updated the diff for D98179: [lit] Sort test start times based on prior test timing data.

I believe I've addressed all of the feedback to date.

Mar 9 2021, 3:29 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added inline comments to D98179: [lit] Sort test start times based on prior test timing data.
Mar 9 2021, 3:26 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 8 2021

davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Nice!
I think this should also be used to improve the ETA time prediction.

Q: Where are these time files stored? I think it would not be good to store them next to the tests themselves.

Mar 8 2021, 3:34 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D98179: [lit] Sort test start times based on prior test timing data.

Whoops. A couple regression were introduced when I tried to simplify this patch before posting it. Please don't try this yet.

Mar 8 2021, 8:37 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki added a comment to D97046: [lit] Generalize `early_tests`.

For people following along, here is the replacement differential: https://reviews.llvm.org/D98179

Mar 8 2021, 7:17 AM · Restricted Project, Restricted Project
davezarzycki updated the summary of D98179: [lit] Sort test start times based on prior test timing data.
Mar 8 2021, 7:12 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
davezarzycki requested review of D98179: [lit] Sort test start times based on prior test timing data.
Mar 8 2021, 7:12 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 7 2021

davezarzycki added a comment to D97913: [runtimes] Use add_lit_testsuite to register lit testsuites.

From my build script:

Mar 7 2021, 11:51 AM · Restricted Project, Restricted Project, Restricted Project
davezarzycki abandoned D97046: [lit] Generalize `early_tests`.

I have a patch that implements the record-and-reorder feature. It turned out to be simpler than I feared. Now I just need to figure out how to write a test for it that makes sense and is reliable. For ninja check-llvm, the record-and-reorder mode completes the test suite 51% faster on my 48-core machine. I think it's time to abandon this change proposal.

Nice! Thanks for all your work.

If I understand how the record-and-reorder feature works, I assume you can test the record and reorder phases separately. The reorder phase should be easy to test with -j1. The record phase is probably harder to test because it depends on test times. You could at least check that it produces a sanely formatted record. I suppose it might be possible to also have one test sleep for a long time and then check that it is ordered before one that terminates immediately, but I'm not sure how brittle that will be on the bots, and it will slow down lit's test suite.

Mar 7 2021, 7:54 AM · Restricted Project, Restricted Project
davezarzycki added a comment to D97046: [lit] Generalize `early_tests`.

I have a patch that implements the record-and-reorder feature. It turned out to be simpler than I feared. Now I just need to figure out how to write a test for it that makes sense and is reliable. For ninja check-llvm, the record-and-reorder mode completes the test suite 51% faster on my 48-core machine. I think it's time to abandon this change proposal.

Mar 7 2021, 6:13 AM · Restricted Project, Restricted Project

Mar 6 2021

davezarzycki resigned from D97071: [lit] Add a TAP-formatted output mode to lit.
Mar 6 2021, 9:56 AM · Restricted Project
davezarzycki added a comment to D97046: [lit] Generalize `early_tests`.
In D97046#2607752, @yln wrote:

@lebedev.ri
I don't think we need a RFC for this as long as nothing changes for people who don't use the early_tests or new, generalized ordering feature. We shouldn't make Dave pay for past debts. (early_tests was introduced 5 years ago!)

@davezarzycki
Thanks for explaining so patiently!

I now understand that there are two separate, but related features: record-and-reorder, and manually declared test phases. Let me summarize my understanding so you can check it.

record-and-reorder:
The sole goal here is to optimize execution time. lit can do this transparently, so maybe it would even make a good default so everyone can benefit. Making it the default is maybe something that benefit from an RFC so we can learn about people's concerns.
The one detail I want to push back on is that Dave argued that this would not apply to clean/full builds. Assume we store the order into a file. With enough motivation there is certainly a way to configure CI bots not to delete this file (or place it somewhere where it wouldn't be deleted).

manually-declared order:
Multiple goals: optimize execution time, execute high/low value tests first/last. Any other reasons to control order?
Although it is unfortunate that we have some overlap with --incremental here, I get that it doesn't accomplish all of these goals at the same time.

For me, the question is now whether the additional utility of "manually-declared order" justifies having it or if we should just got with "record-and-reorder". I think Dave is trying to make the argument that the answer here is "yes". I can see his arguments and would be happy to sign-off on an implementation, especially if the underlying machinery could be re-used to implement the other.

Design-wise I would prefer a solution that directly states test path prefixes, not "test phases". I think your sketch already did this, but let me restate:

// simply a list or dict of `path prefix -> priority`
config.test_order = [
'very/slow/test.c',
'directory/of/slow-tests/',
'<magic-value-for_all-other-tests>',  // name just for demonstration purposes ;)
'directory/of/low-value-tests/',
]

In lit configs, prepending to this list would mean "execute first", appending would mean "execute last".
And I imagine that "record-and-reorder" could be implemented (in a separate effort) on top of this by expanding <magic-value-for_all-other-tests> with the recorded order. (This would mean that the manually declared order is always respected.)

Let me know what you think

Mar 6 2021, 5:28 AM · Restricted Project, Restricted Project
davezarzycki added a reverting change for rGe1173c8794f4: [runtimes] Use add_lit_testsuite to register lit testsuites: rGf4059cc35267: Partially revert "[runtimes] Use add_lit_testsuite to register lit testsuites".
Mar 6 2021, 3:08 AM
davezarzycki committed rGf4059cc35267: Partially revert "[runtimes] Use add_lit_testsuite to register lit testsuites" (authored by davezarzycki).
Partially revert "[runtimes] Use add_lit_testsuite to register lit testsuites"
Mar 6 2021, 3:08 AM
davezarzycki added a reverting change for D97913: [runtimes] Use add_lit_testsuite to register lit testsuites: rGf4059cc35267: Partially revert "[runtimes] Use add_lit_testsuite to register lit testsuites".
Mar 6 2021, 3:08 AM · Restricted Project, Restricted Project, Restricted Project