Page MenuHomePhabricator

brzycki (Brian Rzycki)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 30 2016, 1:50 PM (164 w, 1 d)

Recent Activity

Mon, Jul 29

brzycki added a comment to D65357: [AArch64][GlobalISel] Implement narrowing of G_SEXT..

@aemerson This patch resolves the compiler crash I reported in rL367075. Thank you for the fix.

Mon, Jul 29, 7:58 AM · Restricted Project

Fri, Jul 26

brzycki added a comment to rL367075: [AArch64][GlobalISel] Simplify zext/sext selection, use MachineIRBuilder. NFC..

@aemerson , this commit causes a compiler crash when compiling libcxx for aarch64. Specifically, the file filesystem/operations.cpp:

FAILED: src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o
/work/b.rzycki/b/bin/aarch64-linux-clang++  -DNDEBUG -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_HAS_COMMENT_LIB_PRAGMA -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Iinclude/c++build -I/tmp/tmp.vruMese6eR/src/libcxx/include --target=aarch64-sarc-linux-gnu --sysroot=/work/b.rzycki/b/aarch64-sarc-linux-gnu/sysroot --gcc-toolchain=/work/b.rzycki/b/bin -fPIC   --target=aarch64-sarc-linux-gnu --sysroot=/work/b.rzycki/b/aarch64-sarc-linux-gnu/sysroot --gcc-toolchain=/work/b.rzycki/b/bin -DLIBCXX_BUILDING_LIBCXXABI -std=c++11 -nostdinc++ -fvisibility-inlines-hidden -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wno-user-defined-literals -Wno-covered-switch-default -Wno-ignored-attributes -Wno-error -MD -MT src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o -MF src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o.d -o src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o -c /tmp/tmp.vruMese6eR/src/libcxx/src/filesystem/operations.cpp
clang-10: /tmp/tmp.vruMese6eR/src/llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp:2040: virtual bool (anonymous namespace)::AArch64InstructionSelector::select(llvm::MachineInstr &, llvm::CodeGenCoverage &) const: Assertion `(*RBI.getRegBank(DefReg, MRI, TRI)).getID() == AArch64::GPRRegBankID && "Unexpected ext regbank"' failed.
Stack dump:
0.      Program arguments: /work/b.rzycki/b/bin/clang-10 -cc1 -triple aarch64-sarc-linux-gnu -emit-obj -mrelax-all -disable-free -main-file-name operations.cpp -mrelocation-model pic -pic-level 2 -mthread-model posix -mframe-pointer=all -fmath-errno -masm-verbose -mconstructor-aliases -fuse-init-array -target-cpu generic -target-feature +neon -target-abi aapcs -fallow-half-arguments-and-returns -dwarf-column-info -debugger-tuning=gdb -coverage-notes-file /tmp/tmp.vruMese6eR/build/libcxx-shared/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.gcno -nostdinc++ -resource-dir /work/b.rzycki/b/lib/clang/10.0.0 -dependency-file src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o.d -sys-header-deps -MT src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o -cxx-isystem /work/b.rzycki/b/aarch64-sarc-linux-gnu/include/c++/8.3.0 -cxx-isystem /work/b.rzycki/b/aarch64-sarc-linux-gnu/include/c++/8.3.0/aarch64-sarc-linux-gnu -D _GLIBCXX_INCLUDE_NEXT_C_HEADERS -D NDEBUG -D _LIBCPP_BUILDING_LIBRARY -D _LIBCPP_HAS_COMMENT_LIB_PRAGMA -D _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -I include/c++build -I /tmp/tmp.vruMese6eR/src/libcxx/include -D LIBCXX_BUILDING_LIBCXXABI -isysroot /work/b.rzycki/b/aarch64-sarc-linux-gnu/sysroot -internal-isystem /work/b.rzycki/b/aarch64-sarc-linux-gnu/sysroot/usr/local/include -internal-isystem /work/b.rzycki/b/lib/clang/10.0.0/include -internal-externc-isystem /work/b.rzycki/b/aarch64-sarc-linux-gnu/sysroot/include -internal-externc-isystem /work/b.rzycki/b/aarch64-sarc-linux-gnu/sysroot/usr/include -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wno-user-defined-literals -Wno-covered-switch-default -Wno-ignored-attributes -Wno-error -std=c++11 -fdeprecated-macro -fdebug-compilation-dir /tmp/tmp.vruMese6eR/build/libcxx-shared -ferror-limit 19 -fmessage-length 0 -fvisibility-inlines-hidden -fno-signed-char -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -faddrsig -o src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o -x c++ /tmp/tmp.vruMese6eR/src/libcxx/src/filesystem/operations.cpp
1.      <eof> parser at end of file
2.      Code generation
3.      Running pass 'Function Pass Manager' on module '/tmp/tmp.vruMese6eR/src/libcxx/src/filesystem/operations.cpp'.
4.      Running pass 'InstructionSelect' on function '@_ZNSt3__16chrono8durationInNS_5ratioILl1ELl1EEEEC2IlEERKT_PNS_9enable_ifIXaasr14is_convertibleIS6_nEE5valueooL_ZNS_17integral_constantIbLb0EE5valueEEntsr23treat_as_floating_pointIS6_EE5valueEvE4typeE'
 #0 0x00007f177532f954 PrintStackTraceSignalHandler(void*) (/work/b.rzycki/b/lib/libLLVMSupport.so.10svn+0x180954)
 #1 0x00007f177532d63e llvm::sys::RunSignalHandlers() (/work/b.rzycki/b/lib/libLLVMSupport.so.10svn+0x17e63e)
 #2 0x00007f177532fc08 SignalHandler(int) (/work/b.rzycki/b/lib/libLLVMSupport.so.10svn+0x180c08)
 #3 0x00007f17796c2890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
 #4 0x00007f17727ade97 raise /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #5 0x00007f17727af801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0
 #6 0x00007f177279f39a __assert_fail_base /build/glibc-OTsEL5/glibc-2.27/assert/assert.c:89:0
 #7 0x00007f177279f412 (/lib/x86_64-linux-gnu/libc.so.6+0x30412)
 #8 0x00007f177ca9206f (anonymous namespace)::AArch64InstructionSelector::select(llvm::MachineInstr&, llvm::CodeGenCoverage&) const (/work/b.rzycki/b/lib/libLLVMAArch64CodeGen.so.10svn+0x12a06f)
 #9 0x00007f17721bfe13 llvm::InstructionSelect::runOnMachineFunction(llvm::MachineFunction&) (/work/b.rzycki/b/lib/libLLVMGlobalISel.so.10svn+0x4ae13)
#10 0x00007f177a49440a llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/work/b.rzycki/b/lib/libLLVMCodeGen.so.10svn+0x22840a)
#11 0x00007f17769f0e43 llvm::FPPassManager::runOnFunction(llvm::Function&) (/work/b.rzycki/b/lib/libLLVMCore.so.10svn+0x1e1e43)
#12 0x00007f17769f1153 llvm::FPPassManager::runOnModule(llvm::Module&) (/work/b.rzycki/b/lib/libLLVMCore.so.10svn+0x1e2153)
#13 0x00007f17769f17bd llvm::legacy::PassManagerImpl::run(llvm::Module&) (/work/b.rzycki/b/lib/libLLVMCore.so.10svn+0x1e27bd)
#14 0x00007f17788ac45c clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (/work/b.rzycki/b/lib/libclangCodeGen.so.10svn+0x9445c)
#15 0x00007f1778b63de1 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/work/b.rzycki/b/lib/libclangCodeGen.so.10svn+0x34bde1)
#16 0x00007f176f72cef3 clang::ParseAST(clang::Sema&, bool, bool) (/work/b.rzycki/b/lib/libclangParse.so.10svn+0x2eef3)
#17 0x00007f17781224f8 clang::FrontendAction::Execute() (/work/b.rzycki/b/lib/libclangFrontend.so.10svn+0xe84f8)
#18 0x00007f17780bd988 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/work/b.rzycki/b/lib/libclangFrontend.so.10svn+0x83988)
#19 0x00007f1777e3708c clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/work/b.rzycki/b/lib/libclangFrontendTool.so.10svn+0x408c)
#20 0x0000000000410d5c cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/work/b.rzycki/b/bin/clang-10+0x410d5c)
#21 0x000000000040eae4 main (/work/b.rzycki/b/bin/clang-10+0x40eae4)
#22 0x00007f1772790b97 __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:344:0
#23 0x000000000040bd9a _start (/work/b.rzycki/b/bin/clang-10+0x40bd9a)
clang-10: error: unable to execute command: Aborted (core dumped)
clang-10: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 10.0.0 (/sarc-c/compiler/git/tools/llvm-project.git c07fe307b48a98a149578948b167802f7b2825cf)
Target: aarch64-sarc-linux-gnu
Thread model: posix
InstalledDir: /work/b.rzycki/b/bin
clang-10: note: diagnostic msg: PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
clang-10: note: diagnostic msg:
********************
Fri, Jul 26, 8:28 AM

Thu, Jul 25

brzycki added a comment to D65079: [GlobalISel][AArch64] Fix narrowScalar for G_ICMP.

Thank you @aemerson , the fix in rL366943 seems to have prevented crashes when compiling libcxx for aarch64. I successfully compiled a nightly with sha f181dd99cf1c4e .

Thu, Jul 25, 8:25 AM · Restricted Project

Jul 22 2019

brzycki added a comment to D64856: [MIPS GlobalISel] ClampScalar and select pointer G_ICMP.

Hi @Petar.Avramovic , I replied to your testing query in D65079. Thank you again for looking into this.

Jul 22 2019, 1:35 PM · Restricted Project
brzycki updated subscribers of D65079: [GlobalISel][AArch64] Fix narrowScalar for G_ICMP.

Hello @Petar.Avramovic , I have verified this patch on a recent sha 510e6fadaae95a41bee0eb99d84a146efc612365 does not crash clang when compiling the libcxx shared library. Thank you for identifying the source of the crash.

Jul 22 2019, 1:28 PM · Restricted Project

Jul 19 2019

brzycki added a comment to D64984: [Verifier] Verify all blockaddress constants left over have at least one user..

Hello @fhahn , it looks like more tests are failing on tip 7df225dfc25adc8371188dc57f3adf300b0bd697 when I tested on x86_64:

Failing Tests (4):
    LLVM :: CodeGen/X86/retpoline.ll
    LLVM :: CodeGen/X86/speculative-load-hardening-indirect.ll
    LLVM :: Transforms/ConstProp/basictest.ll
    LLVM :: Transforms/IndirectBrExpand/basic.ll
Jul 19 2019, 1:03 PM · Restricted Project
brzycki accepted D64936: [Local] Zap blockaddress without users in ConstantFoldTerminator..

LGTM. I am assuming this passed test-suite and ninja check runs?

Jul 19 2019, 11:46 AM · Restricted Project
brzycki added a comment to D64856: [MIPS GlobalISel] ClampScalar and select pointer G_ICMP.

Hello @Petar.Avramovic , this commit is causing a compiler assert when I perform the following:

Jul 19 2019, 11:10 AM · Restricted Project

Jul 17 2019

brzycki added a comment to D64805: [AArch64] Expand bcmp() for small comparisons.

Hello @SjoerdMeijer , thank you for the review.

Jul 17 2019, 8:27 AM · Restricted Project

Jul 16 2019

brzycki created D64805: [AArch64] Expand bcmp() for small comparisons.
Jul 16 2019, 9:22 AM · Restricted Project

Jul 12 2019

brzycki added a comment to D64579: [clang-shlib] Fix clang-shlib for PRIVATE dependencies.

Hello @smeenai , this commit causes LLVM to fail to build when set set -D BUILD_SHARED_LIBS=ON. Here is the failing CMake and Ninja invocation:

/tools/build/cmake-3.14.5/bin/cmake \
    -G Ninja  \
    -D CMAKE_MAKE_PROGRAM=/tools/build/ninja-1.9.0/ninja \
    -D CMAKE_C_COMPILER=/usr/lib/llvm-7/bin/clang \
    -D CMAKE_CXX_COMPILER=/usr/lib/llvm-7/bin/clang++ \
    -D CMAKE_BUILD_TYPE=Release \
    -D CMAKE_INSTALL_PREFIX=/work/upstream/install \
    -D LLVM_TOOL_CLANG_BUILD=ON \
    -D LIBCXX_INCLUDE_BENCHMARKS=ON \
    -D BUILD_SHARED_LIBS=ON \
    -D LLVM_ENABLE_ASSERTIONS=ON \
    -D LLVM_TARGETS_TO_BUILD=X86 \
    /work/upstream/llvm-project/llvm
Jul 12 2019, 9:11 AM · Restricted Project, Restricted Project

Jul 2 2019

brzycki accepted D63913: [JumpThreading] Fix threading with unusual PHI nodes..

LGTM.

Jul 2 2019, 3:56 PM · Restricted Project
brzycki added a comment to D63913: [JumpThreading] Fix threading with unusual PHI nodes..

Hi @efriedma, comments are inline.

Jul 2 2019, 8:24 AM · Restricted Project

Jun 28 2019

brzycki abandoned D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.

Abandoning for @efriedma 's patch in D63913.

Jun 28 2019, 9:23 AM · Restricted Project
brzycki added a comment to D63913: [JumpThreading] Fix threading with unusual PHI nodes..

Hi @efriedma , thank you for debugging and identifying the true root cause of PR42085. My comments are inline.

Jun 28 2019, 9:22 AM · Restricted Project

Jun 27 2019

brzycki added a comment to D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.

How do we actually get a miscompile?

The new test-case below, pr42085-loopheader-detection.ll exhibits the miscompile on unpatched tip LLVM. In PR42085 I've attached a test harness which runs the code using a simple main.c and a shell script. The main.c looks like:

#include <libgen.h>
#include <stdio.h>
#include <stdlib.h>
Jun 27 2019, 8:44 AM · Restricted Project

Jun 25 2019

brzycki added a comment to D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.

Yes, FindFunctionBackedges. But not sure if it would not introduce new regressions..

Yes, that's my concern as well. The way that JumpThreading uses FindFunctionBackedges() is a bit unusual as a proxy for detecting loop headers without dominators or loopinfo. I'm unsure if alternate forms of loopheader detection using this analysis can remove the need to eliminate these invalid edges in F. The core problem of this IR shape is JT's analysis fails to detect the true loop headers and eventually we thread across one.

Jun 25 2019, 11:25 AM · Restricted Project
brzycki added a comment to D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.

Maybe you can use some reasonable BB size threshold to fix these regressions ?

Jun 25 2019, 10:54 AM · Restricted Project
brzycki updated the diff for D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.

Passes ninja check and test-suite compilation and lit execution. Compile time for some benchmarks improves for some benchmarks:

Jun 25 2019, 10:09 AM · Restricted Project

Jun 20 2019

brzycki updated the summary of D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.
Jun 20 2019, 3:58 PM · Restricted Project
brzycki added a comment to D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.

Folding conditional terminator instructions for basic blocks that are also loop headers can cause miscompiled code emitted from JumpThreading.

Please can you write a more detailed explanation of the problem in the patch description.
"cause miscompiled code emitted" is vague.

Jun 20 2019, 3:49 PM · Restricted Project
brzycki added a comment to D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.

This code passes ninja check and a full test-suite run.

Jun 20 2019, 3:34 PM · Restricted Project
brzycki created D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders.
Jun 20 2019, 3:34 PM · Restricted Project

Jun 11 2019

brzycki added a comment to D58700: [JumpThreading] Replace MergeBasicBlockIntoOnlyPred() with MergeBlockIntoPredecessor().

Thank you @asbirlea for the review. I'll rebase, verify everything looks ok, and commit. Hopefully in the next 7-10 days based on my current work load.

Jun 11 2019, 7:56 AM · Restricted Project

May 14 2019

brzycki added a comment to D61920: [JumpThreading] A bug fix for stale loop info after unfold select.

LGTM (but I'm not listed as a reviewer...)

May 14 2019, 3:27 PM · Restricted Project

May 13 2019

brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .

LGTM. The number of if defined() and duplicated code regions is minimal. I'm curious to know what @vitalybuka and @kcc think of this iteration of the patch.

May 13 2019, 9:01 AM · Restricted Project

May 10 2019

brzycki added inline comments to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .
May 10 2019, 8:45 AM · Restricted Project
brzycki added inline comments to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .
May 10 2019, 8:10 AM · Restricted Project
brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .

@sebpop welcome back! I'm glad you received clearance to work on this. :) My comments are inline.

May 10 2019, 8:09 AM · Restricted Project

May 9 2019

brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .

Hello @vitalybuka , I have uploaded a WIP diff in D61766 and I appreciate any insight you can give.

May 9 2019, 4:13 PM · Restricted Project
brzycki created D61766: WIP: Speed-up leak and address sanitizers lsan prototype.
May 9 2019, 3:51 PM · Restricted Project, Restricted Project
brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .

Hi @vitalybuka , I am attempting to use D61401 as you suggested but have run into an issue regarding use1 used inside static member functions. This error persists regardless if I attempt to make it a member variable or a global variable:

/work/b.rzycki/upstream/llvm-project/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_allocator_primary_pair.h:76:9: error: invalid use of member 'use1' in static member function
    if (use1)
        ^~~~
/work/b.rzycki/upstream/llvm-project/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_allocator_primary_pair.h:82:9: error: invalid use of member 'use1' in static member function
    if (use1)
        ^~~~

static bool use1 = false;

May 9 2019, 8:56 AM · Restricted Project

May 8 2019

brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .

Hello @vitalybuka , thank you for the example in D61401. Do you have a suggestion what file and namespace the DoubleAllocator template class definition should reside? The most recent diff for D60243 was authored April 3 and no longer cleanly applies (the file lsan_allocator.h has changed considerably since then). This is complicated by @sebpop changing jobs and requiring legal approval before he can help work on this again. In order to make progress I'm essentially re-writing this patch based on the new Allocator layout in lsan_allocator.h.

May 8 2019, 10:10 AM · Restricted Project

Apr 29 2019

brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .

I've started some refactoring there. So maybe it would be easier after that.

Apr 29 2019, 8:45 AM · Restricted Project

Apr 23 2019

brzycki added a comment to D61039: [libcxx] Use relative path for libc++ library when generating script.

+1 to this patch. It fixes our nightly cross-compilers for arm and aarch64.

Apr 23 2019, 3:54 PM

Apr 16 2019

brzycki updated the diff for D58700: [JumpThreading] Replace MergeBasicBlockIntoOnlyPred() with MergeBlockIntoPredecessor().

Rebase to tip. Reviews or comments are very much appreciated.

Apr 16 2019, 3:03 PM · Restricted Project

Apr 15 2019

brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .

Hi @kcc. @sebpop and I decided on this approach because we were unable to create a base allocator class. We ran into pointer coercion issues for 32-bit or 64-bit pointers returned from some of the methods. We either had to mask this with (void *) casts at all the callsites. Even if we did this every routine would require a runtime check for 32 vs 64 on aarch64. This latest patch reduces the number of ifdefs and is still a compile-time check for every non-aarch64 platform.

Apr 15 2019, 8:43 AM · Restricted Project

Apr 8 2019

brzycki committed rG887865c1ad6e: [JumpThreading] Fix incorrect fold conditional after indirectbr/callbr (authored by brzycki).
[JumpThreading] Fix incorrect fold conditional after indirectbr/callbr
Apr 8 2019, 11:20 AM
brzycki committed rL357930: [JumpThreading] Fix incorrect fold conditional after indirectbr/callbr.
[JumpThreading] Fix incorrect fold conditional after indirectbr/callbr
Apr 8 2019, 11:19 AM
brzycki closed D60284: [JumpThreading] [PR40992] Fix miscompile when folding a successor of an indirectbr.
Apr 8 2019, 11:19 AM · Restricted Project

Apr 4 2019

brzycki updated the diff for D60284: [JumpThreading] [PR40992] Fix miscompile when folding a successor of an indirectbr.

Added missing check to testcase ensuring %bb1 threads to %bb7 when %c is false.

Apr 4 2019, 2:28 PM · Restricted Project
brzycki updated the diff for D60284: [JumpThreading] [PR40992] Fix miscompile when folding a successor of an indirectbr.

Address review comments.

Apr 4 2019, 2:19 PM · Restricted Project
brzycki added inline comments to D60284: [JumpThreading] [PR40992] Fix miscompile when folding a successor of an indirectbr.
Apr 4 2019, 2:07 PM · Restricted Project
brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 48-bit VMA .

I think the changes are minimal and make sense given the define() blocks for other arches. I'll defer to @kcc on approval though.

Apr 4 2019, 2:06 PM · Restricted Project
brzycki added a comment to D60284: [JumpThreading] [PR40992] Fix miscompile when folding a successor of an indirectbr.

Passes make check and test-suite runs.

Apr 4 2019, 1:59 PM · Restricted Project
brzycki created D60284: [JumpThreading] [PR40992] Fix miscompile when folding a successor of an indirectbr.
Apr 4 2019, 1:53 PM · Restricted Project

Apr 2 2019

brzycki accepted D60158: [libc++abi] Actually set POSITION_INDEPENDENT_CODE when building shared library.

Thank you @sbc100 for the fast fix. I've verified my internal compiler builds succeed with tip + this patch.

Apr 2 2019, 3:52 PM · Restricted Project, Restricted Project
brzycki added a comment to D60049: [libc++abi] Add LIBCXXABI_ENABLE_PIC cmake option.

Hello @sbc100, I'm seeing a build break caused by D60005 when I attempt to do the following:

  1. compile llvm for aarch64 with a gnu toolchain sysroot
  2. use the freshly built llvm aarch64 compiler to build libcxxabi aarch64 before building libcxx aarch64
Apr 2 2019, 10:03 AM · Restricted Project

Mar 21 2019

brzycki updated the diff for D58700: [JumpThreading] Replace MergeBasicBlockIntoOnlyPred() with MergeBlockIntoPredecessor().

Rebase patchset to tip.

Mar 21 2019, 11:14 AM · Restricted Project

Mar 8 2019

brzycki accepted D58963: [JumpThreading] Retain debug info when replacing branch instructions.

Thank you @StephenTozer , LGTM.

Mar 8 2019, 11:08 AM · Restricted Project

Mar 7 2019

brzycki added a comment to D58963: [JumpThreading] Retain debug info when replacing branch instructions.

The code change touches two places, but the test only seems to test one branch instruction - is one of the two code changes untested?

Yes; the second case seemed like it would need an additional, more complicated IR case to test, but the logic for performing the conditional-to-unconditional branch conversion is identical in both cases (could be extracted to a utility function perhaps?) and the semantic meaning is the same as well: "The condition at this state is guaranteed to be true/false, so remove the condition check".

Mar 7 2019, 9:38 AM · Restricted Project

Mar 6 2019

brzycki added a comment to D58700: [JumpThreading] Replace MergeBasicBlockIntoOnlyPred() with MergeBlockIntoPredecessor().

Bump. Reviews or feedback would be appreciated.

Mar 6 2019, 7:42 AM · Restricted Project
brzycki accepted D58963: [JumpThreading] Retain debug info when replacing branch instructions.

LGTM. It's a simple and clean change.

Mar 6 2019, 7:41 AM · Restricted Project

Mar 1 2019

brzycki accepted D58794: [scudo][standalone] Fix tests makefile.

Thank you for helping debug this @cryptoad .

Mar 1 2019, 7:36 AM · Restricted Project, Restricted Project

Feb 28 2019

brzycki added a comment to D58184: [scudo][standalone] Introduce platform specific code & mutexes.

Cool. Feel free to send it out if you want, or I'll do it when I am done with my current CL.
For the RHEL I thought linking against librt would do, but I don't have a way to debug it locally fast.

Good news: the patch fixes the RHEL 6.7 build issues as well.

Feb 28 2019, 1:57 PM · Restricted Project, Restricted Project
brzycki added a comment to D58184: [scudo][standalone] Introduce platform specific code & mutexes.

I seemed to have fixed it locally:

add_library("RTScudoStandalone.test.${arch}" STATIC
  $<TARGET_OBJECTS:RTScudoStandalone.${arch}>)

The STATIC was missing. Let me know if that works for you.

Feb 28 2019, 11:05 AM · Restricted Project, Restricted Project

Feb 27 2019

brzycki added a comment to D58184: [scudo][standalone] Introduce platform specific code & mutexes.

append_list_if(COMPILER_RT_HAS_LIBRT -lrt LINK_FLAGS) did not work, nor did list(APPEND LINK_FLAGS -lrt) for RHEL 6.7.

Feb 27 2019, 1:20 PM · Restricted Project, Restricted Project
brzycki added a comment to D58184: [scudo][standalone] Introduce platform specific code & mutexes.

I'm about to test it on RHEL 6.7 to see if the two are orthogonal or related (I suspect the former).

As expected, it did not fix RHEL 6.7. I am now trying your append_list_if(COMPILER_RT_HAS_LIBRT -lrt LINK_FLAGS) suggestion.

Feb 27 2019, 12:53 PM · Restricted Project, Restricted Project
brzycki added a comment to D58184: [scudo][standalone] Introduce platform specific code & mutexes.

@cryptoad the following (terribly hacky) patch successfully builds for me on Ubuntu 18.04 LTS:

diff --git a/compiler-rt/cmake/base-config-ix.cmake b/compiler-rt/cmake/base-config-ix.cmake
index ee9426b715d..8cc1e922f50 100644
--- a/compiler-rt/cmake/base-config-ix.cmake
+++ b/compiler-rt/cmake/base-config-ix.cmake
@@ -161,7 +161,7 @@ macro(test_targets)
           endif()
         else()
           test_target_arch(x86_64 "" "-m64")
-          test_target_arch(i386 __i386__ "-m32")
+          #test_target_arch(i386 __i386__ "-m32")
         endif()
       else()
         if (CMAKE_SIZEOF_VOID_P EQUAL 4)
Feb 27 2019, 12:29 PM · Restricted Project, Restricted Project
brzycki added a comment to D58184: [scudo][standalone] Introduce platform specific code & mutexes.

Hello @cryptoad , I am seeing compiler build failures starting with the commit for this patch (sha 41aba567d991c2bd551c ).

Feb 27 2019, 11:19 AM · Restricted Project, Restricted Project

Feb 26 2019

brzycki updated the summary of D58700: [JumpThreading] Replace MergeBasicBlockIntoOnlyPred() with MergeBlockIntoPredecessor().
Feb 26 2019, 3:30 PM · Restricted Project
brzycki created D58700: [JumpThreading] Replace MergeBasicBlockIntoOnlyPred() with MergeBlockIntoPredecessor().
Feb 26 2019, 3:28 PM · Restricted Project
brzycki accepted D58444: Make MergeBlockIntoPredecessor conformant to the precondition of calling DTU.applyUpdates.

I finished performing functional testing using MergeBlockIntoPredecessor in JumpThreading.cpp. With this change I was able to pass make-check and test-suite. I saw no crashes and believe this to be the correct updates for DTU.

Feb 26 2019, 2:13 PM · Restricted Project

Feb 25 2019

brzycki added a comment to D58444: Make MergeBlockIntoPredecessor conformant to the precondition of calling DTU.applyUpdates.

Thanks! Sounds like it will be possible to avoid DomTree recalculations then. :) I’m glad to help if there is anything involving DTU to the best of my knowledge.

I appreciate it. So far the issues I've had to deal with are all related to test cases, LVI, or the ABI change that MErgeBlockIntoPredecessor can return false and not merge blocks. I'm running test-suite today to double-check my changes. If that goes well I'll probably give you a "LGTM" on this change.

Feb 25 2019, 10:52 AM · Restricted Project

Feb 22 2019

brzycki added a comment to D58444: Make MergeBlockIntoPredecessor conformant to the precondition of calling DTU.applyUpdates.

Thank you for looking into this. I was working on applying MergeBLockIntoPredecessor replacement in JumpThreading.cpp (via D48202) and ran into check failures that I haven't had time to properly debug. I suspect at least one of these is related to the change you made above.

Thanks for your comment! I am worried about the difficulty in discovering and debugging bugs involving incremental DT updating. I think it is indeed easy to violet the preconditions of applyUpdates() and I believe if it is a concern, a strict validation should be added to check if preconditions are fulfilled in DEBUG mode later.

Feb 22 2019, 8:40 AM · Restricted Project

Feb 21 2019

brzycki added inline comments to D57953: [Jump Threading] Convert conditional branches into unconditional branches using GVN results.
Feb 21 2019, 10:54 AM

Feb 20 2019

brzycki accepted D58170: [DTU] Refine the interface and logic of applyUpdates.

LGTM.

Feb 20 2019, 12:43 PM · Restricted Project
brzycki added a comment to D58327: [Dominators] Simplify and optimize path compression used in link-eval forest..

@brzycki Thanks for the tip! Do you know how to create testing .bc from LLVM test-suite for benchmarks?

Hello @MaskRay, test-suite has a subdirectory named LLVMSource that contains IR variants of tests. However, I see no reference to that sub-directory in the CMake build files and the last update to that directory was in 2012. I have never tried it and have no idea if lit can run tests in there. The LLVMSource/Makefile does pull in all files in that directory so if it worked you should be able to just add a *.ll file there for it to be picked up.

Feb 20 2019, 9:57 AM · Restricted Project
brzycki accepted D58443: [DTU] Deprecate insertEdge*/deleteEdge*.

+1 Thank you. LGTM.

Feb 20 2019, 8:53 AM · Restricted Project
brzycki added a comment to D58444: Make MergeBlockIntoPredecessor conformant to the precondition of calling DTU.applyUpdates.

Thank you for looking into this. I was working on applying MergeBLockIntoPredecessor replacement in JumpThreading.cpp (via D48202) and ran into check failures that I haven't had time to properly debug. I suspect at least one of these is related to the change you made above.

Feb 20 2019, 8:50 AM · Restricted Project
brzycki added a comment to D58170: [DTU] Refine the interface and logic of applyUpdates.

@NutshellySima I like the idea of the new naming scheme that makes usage more explicit.

Feb 20 2019, 8:43 AM · Restricted Project

Feb 19 2019

brzycki added a comment to D58327: [Dominators] Simplify and optimize path compression used in link-eval forest..

In addition, make sure that your performance governor is set to performance and that you pin the process to a single cpu core with its sibling disabled. (like in https://llvm.org/docs/Benchmarking.html#linux). Benchmarking on x86 is hard.

+1

Feb 19 2019, 2:05 PM · Restricted Project

Feb 15 2019

brzycki added inline comments to D58170: [DTU] Refine the interface and logic of applyUpdates.
Feb 15 2019, 9:50 AM · Restricted Project
brzycki accepted D57881: [DTU] Refine the document of mutation APIs [NFC] (PR40528).
Feb 15 2019, 7:57 AM · Restricted Project

Feb 14 2019

brzycki added a comment to D54978: Move the SMT API to LLVM.

Here's my proposed logic as actual CMake code. @mikhail.ramalho if you can get your code to emit the version string I made a TODO placeholder for it in the 3rd block below.

Feb 14 2019, 3:44 PM · Restricted Project, Restricted Project
brzycki added a comment to D54978: Move the SMT API to LLVM.

I just sent the first prototype of the solution. All the magic happens inside the CHECK_CXX_SOURCE_RUNS call.

I think hardcoding the version into the binary is too brittle. Instead, your program can just print out the current z3 version (much like the current execution of the z3 binary), and the remaining logic can remain in CMake, with the fallbacks as suggested above by @brzycki.

Feb 14 2019, 3:38 PM · Restricted Project, Restricted Project
brzycki added a comment to D57953: [Jump Threading] Convert conditional branches into unconditional branches using GVN results.

Hello @masakiarai, the new patch looks better, thank you. My updated comments are inline.

Feb 14 2019, 9:01 AM
Herald added a project to D48202: Generalize MergeBlockIntoPredecessor. Replace uses of MergeBasicBlockIntoOnlyPred.: Restricted Project.

My apologies for missing this review, for some reason I wasn't notified of being added. I will look into the potential replacement of MergeBlockIntoPredecessor for JumpThreading.

Feb 14 2019, 8:51 AM · Restricted Project
brzycki added inline comments to D58170: [DTU] Refine the interface and logic of applyUpdates.
Feb 14 2019, 8:44 AM · Restricted Project

Feb 13 2019

brzycki added a comment to D54978: Move the SMT API to LLVM.

The old version.h header isn't externally exposed, only the newer z3_version.h header starting with version 4.8.1. I built a copy of 4.7.1 from source, and I don't see it, so unfortunately I think the second check for version.h is superfluous. Instead, I think it'd be ok to still query the executable as the primary, then fallback to this header as the secondary?

Thanks @ddcc for checking on version.h, I meant to do so but got side-tracked with other items today.

Feb 13 2019, 1:18 PM · Restricted Project, Restricted Project
brzycki added a comment to D54978: Move the SMT API to LLVM.

Hi @brzycki, but isn't it exactly what we want? I mean, if we try to cross-compile and it fails for any reason (non-native library, wrong version), then Z3_FOUND shouldn't be set.

I'm not sure, cross-compilation is tricky to get right.

Feb 13 2019, 11:38 AM · Restricted Project, Restricted Project
brzycki added inline comments to D58170: [DTU] Refine the interface and logic of applyUpdates.
Feb 13 2019, 11:20 AM · Restricted Project
brzycki added inline comments to D57881: [DTU] Refine the document of mutation APIs [NFC] (PR40528).
Feb 13 2019, 9:35 AM · Restricted Project
brzycki added a comment to D58187: Teach DTU to recalculate DT/PDT automatically when EntryBB is changed.

recalculate() can be deprecated. But there are still edge cases, for example, when we only insert a new block to be the entry block or move a block up to be the entry block, it is still needed to call flush() to detect this change. (But I don't see usage like the above scenario I mentioned currently.)

If there are still edge cases then we need to keep it as a last-ditch method of resetting the DTU state without creating a new class instance. It might be a good idea to add documentation stating the interface isn't meant to be used except as a last-ditch fix that cannot be addressed by any other means.

Feb 13 2019, 9:24 AM · Restricted Project
brzycki added a comment to D58187: Teach DTU to recalculate DT/PDT automatically when EntryBB is changed.

This is much cleaner, I like this very much. Originally adding the method recalculate() was a compromise. With this change do we even need the public recalculate()method? Does it ever make sense to make a new entry BB that has zero successors?

Feb 13 2019, 9:03 AM · Restricted Project
brzycki added a comment to D57953: [Jump Threading] Convert conditional branches into unconditional branches using GVN results.

I uploaded my patch with some fixes.

Thank you for the comments and re-upload @masakiarai. I will review the patch again either today or tomorrow.

Feb 13 2019, 8:56 AM
brzycki added a comment to D54978: Move the SMT API to LLVM.

perhaps something like this:

if(Z3_INCLUDE_DIR AND EXISTS "${Z3_INCLUDE_DIR }/z3_version.h")
  file(STRINGS "${Z3_INCLUDE_DIR }/z3_version.h" z3_version_str REGEX "^#define[\t ]+Z3_FULL_VERSION[\t ]+\".*\"")

  string(REGEX REPLACE "^.*Z3_FULL_VERSION[\t ]+\"Z3 ([0-9\.]+)\".*$" "\\1" Z3_VERSION_STRING "${z3_version_str}")
  unset(z3_version_str)
endif()
Feb 13 2019, 8:32 AM · Restricted Project, Restricted Project

Feb 12 2019

brzycki added a comment to D54978: Move the SMT API to LLVM.

I'm wondering if we can remove the binary requirement all together: is it possible to run a small program that would return EXIT_SUCCESS if the library is the correct version?

Hi @mikhail.ramalho, I don't think this is feasible unfortunately. If we're using a cross-compiler the emitted binary won't be native to the platform. In other words, you cannot test for Z3 as a run-time property.

Feb 12 2019, 3:55 PM · Restricted Project, Restricted Project
brzycki added a comment to D54978: Move the SMT API to LLVM.

I think I found why others are struggling to recreate this issue. I did not have the package z3/bionic installed. This provides the /usr/bin/z3 executable which llvm/cmake/modules/FindZ3.cmake relies upon to determine the correct Z3_VERSION_STRING.

Yeah, that's what I meant by best-effort only. Due to upstream Z3's design, without the binary, there is no easy way to retrieve the current installed version, so when I originally wrote the Z3 integration, it seemed fine to let the version check pass. Given the issues that have come up, it might make more sense to at least emit a warning, or explicitly fail if the version can't be determined, and prompt the user to install the binary.

Feb 12 2019, 2:24 PM · Restricted Project, Restricted Project
brzycki added a comment to D54978: Move the SMT API to LLVM.

I think I found why others are struggling to recreate this issue. I did not have the package z3/bionic installed. This provides the /usr/bin/z3 executable which llvm/cmake/modules/FindZ3.cmake relies upon to determine the correct Z3_VERSION_STRING.

Feb 12 2019, 12:16 PM · Restricted Project, Restricted Project
brzycki added a comment to D54978: Move the SMT API to LLVM.

The following patch:

diff --git a/llvm/cmake/modules/CrossCompile.cmake b/llvm/cmake/modules/CrossCompile.cmake
index bc3b210f018..0c30b88f80f 100644
--- a/llvm/cmake/modules/CrossCompile.cmake
+++ b/llvm/cmake/modules/CrossCompile.cmake
@@ -53,6 +53,7 @@ function(llvm_create_cross_target_internal target_name toolchain buildtype)
         -DLLVM_DEFAULT_TARGET_TRIPLE="${TARGET_TRIPLE}"
         -DLLVM_TARGET_ARCH="${LLVM_TARGET_ARCH}"
         -DLLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN="${LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN}"
+        -DLLVM_ENABLE_Z3_SOLVER="${LLVM_ENABLE_Z3_SOLVER}"
         ${build_type_flags} ${linker_flag} ${external_clang_dir}
     WORKING_DIRECTORY ${LLVM_${target_name}_BUILD}
     DEPENDS CREATE_LLVM_${target_name}
Feb 12 2019, 10:29 AM · Restricted Project, Restricted Project
brzycki added a comment to D54978: Move the SMT API to LLVM.

Got it now, sorry about being dense.

No problem, I appreciate you looking into this. :)

Feb 12 2019, 7:54 AM · Restricted Project, Restricted Project

Feb 11 2019

brzycki added a comment to D54978: Move the SMT API to LLVM.

Do you understand why the default matters for you? You seem to explicitly disable the setting, and you still get Z3 as part of your build. Did you make a clean build dir before turning it to OFF?

Yes, Please see my recreation instructions above. I created a new, empty build directory.

Feb 11 2019, 4:22 PM · Restricted Project, Restricted Project
brzycki added a comment to D57953: [Jump Threading] Convert conditional branches into unconditional branches using GVN results.

Hello @masakiarai, my comments are inline.

Feb 11 2019, 2:30 PM
brzycki added a comment to D54978: Move the SMT API to LLVM.

Shouldn't that be off by default?

The default for LLVM_ENABLE_Z3_SOLVER depends entirely on what CMake detects from find_package(). Here is the relevant code in llvm/CMakeLists.txt:

find_package(Z3 4.7.1)
...
set(LLVM_ENABLE_Z3_SOLVER_DEFAULT "${Z3_FOUND}")
Feb 11 2019, 1:11 PM · Restricted Project, Restricted Project
brzycki added a comment to D57881: [DTU] Refine the document of mutation APIs [NFC] (PR40528).

That looks *highly* fishy. Between which two points [of insertEdge()] we should have exactly one update?

If you are using the Eager strategy these updates will be immediately applied to the DT and PDT internal to the DTU class. As the comment states there are internal checks of the CFG to determine if the insertion is valid. Because DT, PDT, and CFG are three distinct and separate structures we have to treat the CFG as the canonical shape of the IR. In Lazy strategy mode the update is simply queued until the next flush event.

Feb 11 2019, 8:59 AM · Restricted Project
brzycki added a comment to D54978: Move the SMT API to LLVM.

@brzycki, I can't reproduce your error. Maybe you're missing -DLLVM_ENABLE_Z3_SOLVER=OFF?

Feb 11 2019, 8:07 AM · Restricted Project, Restricted Project

Feb 8 2019

brzycki added a comment to D54978: Move the SMT API to LLVM.

Thanks for the analysis. I think it's fine if you revert, given that.

Feb 8 2019, 10:42 AM · Restricted Project, Restricted Project
brzycki added a comment to D54978: Move the SMT API to LLVM.

From what I understand, setting -DLLVM_ENABLE_Z3_SOLVER=OFF is supposed to work

Hello @thakis, I have reduced it down to the minimal required flags on Ubuntu 18.04. I ran this on llvm-project SHA b0a227049fda9d0d229ea801ae77bf1b812f7328 from Feb 8, 2019.

Feb 8 2019, 8:23 AM · Restricted Project, Restricted Project

Feb 7 2019

brzycki added a comment to D57881: [DTU] Refine the document of mutation APIs [NFC] (PR40528).

Nice, thanks for working on this!

Agreed, thank you!

Feb 7 2019, 9:58 AM · Restricted Project