Page MenuHomePhabricator

brzycki (Brian Rzycki)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Tue, Apr 16

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

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

Tue, Apr 16, 3:03 PM · Restricted Project

Mon, Apr 15

brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 47-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.

Mon, Apr 15, 8:43 AM · Restricted Project

Mon, Apr 8

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

Thu, Apr 4

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.

Thu, Apr 4, 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.

Thu, Apr 4, 2:19 PM · Restricted Project
brzycki added inline comments to D60284: [JumpThreading] [PR40992] Fix miscompile when folding a successor of an indirectbr.
Thu, Apr 4, 2:07 PM · Restricted Project
brzycki added a comment to D60243: [LSan][AArch64] Speed-up leak and address sanitizers on AArch64 for 47-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.

Thu, Apr 4, 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.

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

Tue, Apr 2

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.

Tue, Apr 2, 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
Tue, Apr 2, 10:03 AM · Restricted Project

Thu, Mar 21

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

Rebase patchset to tip.

Thu, Mar 21, 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
brzycki added a comment to D54978: Move the SMT API to LLVM.

This commit is causing a build-break for our nightly cross compilers of arm and aarch64. The immediately preceding commit of D54977 does not break with the exact same invocation.

Feb 7 2019, 9:45 AM · Restricted Project, Restricted Project

Jan 8 2019

brzycki accepted D56284: [UnrollRuntime] Fix domTree failure in multiexit unrolling.

I am accepting this revision.

Jan 8 2019, 9:18 AM
brzycki added a comment to D56284: [UnrollRuntime] Fix domTree failure in multiexit unrolling.

@anna thank you for your patience and perseverance with this fix and welcome to the hairy world of manual dominator updates. :)

Jan 8 2019, 8:52 AM

Jan 7 2019

brzycki added a comment to D56419: [gn build] Move .gn file to the root of the monorepo.

Sorry, I don't see how a hidden file that says "this is unsupported" has much promotional value.

Placing .gn at the top-level with a comment that it's unsupported contradicts users expectations. To the casual user outside the day-to-day of llvm-dev they have no idea if they should or should not use .gn. It's confusing at best: is the .gn file location stale or the comment stale or is something else going on? I've worked in this industry long enough to see too many unsupported tools quickly become the cornerstone of key infrastructure.

Jan 7 2019, 5:38 PM
brzycki added a comment to D56419: [gn build] Move .gn file to the root of the monorepo.

This is not changing. The first line in the added file here is: "# LLVM's GN build is unsupported, see llvm/utils/gn/README.rst for details." Is that not clear enough? I'm happy to repeat that line 100 times and add another 100 lines whitespace to make it more obvious.

Jan 7 2019, 4:28 PM
brzycki added a comment to D56419: [gn build] Move .gn file to the root of the monorepo.

I am strongly against this commit. @thakis when this was discussed on llvm-dev the original proposal was the compromise of placing .gn in utils to signify that it was not the default, recommended, method to build LLVM. With this change LLVM will truly have two, parallel build systems once again.

Jan 7 2019, 4:03 PM

Jan 3 2019

brzycki updated subscribers of D56284: [UnrollRuntime] Fix domTree failure in multiexit unrolling.

LGTM. @kuhar may wish to comment.

Jan 3 2019, 11:58 AM
brzycki added a comment to D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor..

@efriedma have you had any luck generating an artificial testcase?

Jan 3 2019, 8:50 AM
brzycki added a comment to D55264: [Jump Threading] Unfold a select instruction that feeds a switch statement via a phi node.

@amehsan gentle ping. Have you resolved your push access?

Jan 3 2019, 8:48 AM

Dec 10 2018

brzycki accepted D55264: [Jump Threading] Unfold a select instruction that feeds a switch statement via a phi node.

Awesome, thanks for doing this @amehsan .

Dec 10 2018, 8:11 AM

Dec 6 2018

brzycki added a comment to D55264: [Jump Threading] Unfold a select instruction that feeds a switch statement via a phi node.

Hi @amehsan , I think the patch looks mostly good but for a few small comments or variable names to enhance readability.

Dec 6 2018, 10:10 AM

Nov 29 2018

brzycki added a comment to D55018: [CodeExtractor] Split PHI nodes with incoming values from outlined region (PR39433).

@kachkov98 thank you for helping to solve this bug. I agree with @vsk 's review comments and I'm most concerned about skipping PHIs with only one predecessor. I think it would be good to add code coverage testing in Utils/CodeExtractorTest.cpp explicitly testing your assumptions of the new code in severSplitPHINodesOfExits()

Nov 29 2018, 7:54 AM

Nov 26 2018

brzycki added a comment to D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor..

I just re-ran both to make sure it wasn't caused by other factors on the host. There's some movement in the <= 2% range but tramp3d-v4 is the same regression.

#  left column: D54730/results.json
# right column: tip/results.json
#  metric name: compile_time
     43.7640 -> 39.6280      [ 10.44%]  CTMark/tramp3d-v4/tramp3d-v4.test
     26.4680 <- 27.0880      [  2.34%]  CTMark/sqlite3/sqlite3.test
    130.5600 -> 128.1000     [  1.92%]  CTMark/7zip/7zip-benchmark.test
     35.4600 <- 36.1000      [  1.80%]  CTMark/kimwitu++/kc.test
     32.2400 -> 31.7520      [  1.54%]  CTMark/consumer-typeset/consumer-typeset.test
     23.3600 <- 23.6480      [  1.23%]  CTMark/mafft/pairlocalalign.test
     52.7280 -> 52.5520      [  0.33%]  CTMark/lencod/lencod.test
     50.8120 <- 50.9560      [  0.28%]  CTMark/ClamAV/clamscan.test
     44.1040 <- 44.2160      [  0.25%]  CTMark/SPASS/SPASS.test
     86.8800 -> 86.8200      [  0.07%]  CTMark/Bullet/bullet.test
Nov 26 2018, 12:48 PM
brzycki added a comment to D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor..

@efriedma I ran CTMark on the patch with the following patch as tip (both compilers are Release):

commit 9b6b09070e777106e6e91c15406e197c7d18e1fb (origin/master, origin/HEAD)
Author: Ilya Biryukov <ibiryukov@google.com>
Date:   Mon Nov 26 15:38:01 2018 +0000
Nov 26 2018, 12:44 PM

Nov 20 2018

brzycki added a comment to D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor..

Has anyone run CTMark test-suite numbers on this patch before and after? @efriedma is it possible to post the extreme C++ case to analyze exactly what's happening in the Dominator update routines and why?

Nov 20 2018, 11:49 AM

Nov 9 2018

brzycki accepted D54317: [IPSCCP,PM] Preserve PDT in the new pass manager..

LGTM. Thanks again for doing this. :)

Nov 9 2018, 8:46 AM

Nov 8 2018

brzycki added a comment to D47259: [IPSCCP,PM] Preserve DT in the new pass manager..

LGTM and I'm glad to see other passes working to preserve DTU.

Nov 8 2018, 10:29 AM
brzycki accepted D54239: [JumpThreading] Fix exponential time algorithm computing known values..

+1 for me as well. JumpThreading, as it stands today, is quite haphazard in how it attacks a function and anything that helps reduce time spent recomputing known values is desirable.

Nov 8 2018, 7:42 AM

Oct 26 2018

brzycki added a comment to D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929).

Since bug 37929 is a regression in version 7.0, should we backport this patch into branch 7.0.1 later (maybe November) if there isn't anyone reporting issues on this patch?

Sounds like a good idea

Oct 26 2018, 7:54 AM

Oct 25 2018

brzycki accepted D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929).

LGTM. I have no doubt someone will speak up if they detect a regression. :)

Oct 25 2018, 10:19 AM
brzycki added a comment to D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929).

The issue I find with cl::opt here is that this is a generic class that may not necessarily have a corresponding cpp file -- there can be multiple clients to GenericDomTreeConstruction, each with different requirements. Because of that, it's not clear to me in which translation unit one would put cl::opt for the constant in.
Is that a solved problem somewhere else in LLVM? I'm not familiar with using cl::opt in header files.

Fair enough. I'm more used to working on function passes that have several cl::opt() lines at the top of the .cpp file. If we don't have a corresponding cpp it's non-trivial to add it as a command-line argument. I too have never tried to use cl::opt() in a .h.

Oct 25 2018, 9:01 AM

Oct 24 2018

brzycki added a comment to D22630: Loop rotation.

Gentle ping and a few comments. There are a few proprietary benchmarks that perform better with this pass and I'd like to see this (hopefully) moving towards a commit.

Oct 24 2018, 9:19 AM
brzycki added a comment to D48181: [JumpThreading] Ignore nil destionation when determining whether a block only goes to a single destination.

Hi @trentxintong , did you decide if this patch was useful?

Oct 24 2018, 8:05 AM
brzycki added a comment to D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929).

I'd still like to see the constant as a hidden command line argument cl::opt(). Otherwise I think this is an excellent patch with considerable compile-time speedups. Nice work @NutshellySima .

Oct 24 2018, 8:02 AM

Oct 16 2018

brzycki added a comment to D52904: [hot-cold-split] fix static analysis of cold regions.

Nice work @sebpop. Here's a delayed "LGTM". :)

Oct 16 2018, 10:57 AM

Oct 15 2018

brzycki added a comment to D53254: [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported.

Hi @hiraditya , comments inline.

Oct 15 2018, 9:50 AM
brzycki added a comment to D53253: [Merge SImilar Function ThinLTO 4/n] Make merge function decisions before the thin-lto stage.

Hi @hiraditya , comments inline.

Oct 15 2018, 9:28 AM
brzycki added a comment to D52966: [Merge SImilar Function ThinLTO 3/n] Add hash code to function summary.

Hi @hiraditya , pardon my ignorance in this part of llvm. Most of my comments are questions about your reasoning.

Oct 15 2018, 9:10 AM
brzycki added a comment to D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929).

Ii might also be worth investigating to make these constant hidden command line parameters. For one it makes testing faster and it also gives users an emergency knob (the cl::opt() mechanism).

Oct 15 2018, 8:47 AM

Oct 9 2018

brzycki added a comment to D52904: [hot-cold-split] fix static analysis of cold regions.

Hi @sebpop, I have a few comments inline regarding the function determining side effect status.

Oct 9 2018, 10:48 AM

Oct 2 2018

brzycki added a comment to D52708: Add support for new pass manager.

LGTM as well, provided @tejohnson 's test complete successfully.

Clarification: I wasn't going to test until this patch was submitted. I wanted this one to go in so I can test the underlying hot cold splitting pass with the new PM. So please don't wait for me!

Oct 2 2018, 10:20 AM
brzycki accepted D52708: Add support for new pass manager.

LGTM as well, provided @tejohnson 's test complete successfully.

Oct 2 2018, 10:14 AM

Aug 2 2018

brzycki accepted D49982: [TailCallElim] Preserve DT and PDT.

LGTM.

Aug 2 2018, 1:14 PM
brzycki added a comment to D49731: [Dominators] Assert if there is modification to DelBB while it is awaiting deletion.

Delayed LGTM. It's a good assert to know if someone tries to alter DelBB after it's marked for deletion.

Aug 2 2018, 10:47 AM
brzycki added inline comments to D49982: [TailCallElim] Preserve DT and PDT.
Aug 2 2018, 10:45 AM