Page MenuHomePhabricator

brzycki (Brian Rzycki)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

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.

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

+1 Thank you. LGTM.

Wed, Feb 20, 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.

Wed, Feb 20, 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.

Wed, Feb 20, 8:43 AM · Restricted Project

Yesterday

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

Tue, Feb 19, 2:05 PM · Restricted Project

Fri, Feb 15

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

Thu, Feb 14

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.

Thu, Feb 14, 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.

Thu, Feb 14, 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.

Thu, Feb 14, 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.

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

Wed, Feb 13

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.

Wed, Feb 13, 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.

Wed, Feb 13, 11:38 AM · Restricted Project, Restricted Project
brzycki added inline comments to D58170: [DTU] Refine the interface and logic of applyUpdates.
Wed, Feb 13, 11:20 AM · Restricted Project
brzycki added inline comments to D57881: [DTU] Refine the document of mutation APIs [NFC] (PR40528).
Wed, Feb 13, 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.

Wed, Feb 13, 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?

Wed, Feb 13, 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.

Wed, Feb 13, 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()
Wed, Feb 13, 8:32 AM · Restricted Project, Restricted Project

Tue, Feb 12

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.

Tue, Feb 12, 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.

Tue, Feb 12, 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.

Tue, Feb 12, 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}
Tue, Feb 12, 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. :)

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

Mon, Feb 11

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.

Mon, Feb 11, 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.

Mon, Feb 11, 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}")
Mon, Feb 11, 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.

Mon, Feb 11, 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?

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

Fri, Feb 8

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.

Fri, Feb 8, 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.

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

Thu, Feb 7

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

Nice, thanks for working on this!

Agreed, thank you!

Thu, Feb 7, 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.

Thu, Feb 7, 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
brzycki accepted D49747: [Dominators] Remove the DeferredDominance class.

LGTM. DTU should be the only way to do Lazy or Eager updates to DT.

Aug 2 2018, 10:42 AM
brzycki added inline comments to D49807: [Dominators] Make DTU be able to delete a BB before recalculation under Eager UpdateStrategy.
Aug 2 2018, 10:41 AM
brzycki added a comment to D49988: [ADCE] Remove the need of DomTree.

LGTM if you have done a full test-suite correctness test on this patch.

Aug 2 2018, 10:38 AM
brzycki accepted D49738: [Dominators] Make RemoveUnreachableBlocks return false if the BasicBlock is already awaiting deletion.

OK by me. Probably won't save much compile time but it is a bit cleaner to read.

Aug 2 2018, 10:33 AM
brzycki accepted D48967: [Dominators] Convert existing passes and utils to use the DomTreeUpdater class.

LGTM. I like how this revision of the patch is minimally invasive and most of the hard work is hidden in DTU. I strongly recommend running ninja check, ninja check-all, and a standard test-suite run on tip before committing.

Aug 2 2018, 10:30 AM

Jul 12 2018

brzycki added a comment to D48181: [JumpThreading] Ignore nil destionation when determining whether a block only goes to a single destination.

Gentle ping for @trentxintong .

Jul 12 2018, 7:56 AM

Jul 11 2018

brzycki added a comment to D48974: [DomTreeUpdater] Ignore updates when both DT and PDT are nullptrs.

@NutshellySima I think you said that BasicBlocks that are deleted with deleteBB() aren't removed immediately and are held until a a flush() event, correct? I don't see any comments informing the users that this is the case. I consider it to be unexpected behavior and should be noted.

Jul 11 2018, 10:26 AM

Jul 10 2018

brzycki added inline comments to D48967: [Dominators] Convert existing passes and utils to use the DomTreeUpdater class.
Jul 10 2018, 10:47 AM
brzycki added a comment to D48967: [Dominators] Convert existing passes and utils to use the DomTreeUpdater class.

Hello @NutshellySima , comment are again inline.

Jul 10 2018, 8:06 AM

Jul 9 2018

brzycki added a comment to D49056: [Dominators] Add isUpdateLazy() method to the DomTreeUpdater.

Perhaps it would make sense to expose a similar function for the Eager strategy?
And I would make it sorter, so just isLazy and isEager -- shouldn't make anything more ambiguous IMO.

Jul 9 2018, 5:22 PM
brzycki added a comment to D48967: [Dominators] Convert existing passes and utils to use the DomTreeUpdater class.

Hello @NutshellySima , comments are inline.

Jul 9 2018, 5:09 PM
brzycki added a comment to D48974: [DomTreeUpdater] Ignore updates when both DT and PDT are nullptrs.

(call delBB() still pends BasicBlock deletion until a flush event according to the doc).

This is an interesting idea as it gives callers a deferred deletion tool for Lazy and both trees are nullptr. It'd be worthwhile to add documentation somewhere to let others know about this. I know of at least one case when I was working on JumpThreading that the original code flushed LVI information about a block because it called an external routine that might delete the block (it was at the end of runImpl()). This feature gives others a simple way of calling potentially destructive calls, checking the return code, and cleaning up on success or failure.

Jul 9 2018, 9:04 AM
brzycki added a comment to D48967: [Dominators] Convert existing passes and utils to use the DomTreeUpdater class.

Hello @NutshellySima , comments are inline.

Jul 9 2018, 8:35 AM

Jul 6 2018

brzycki added a comment to D48967: [Dominators] Convert existing passes and utils to use the DomTreeUpdater class.

I ran ninja check-llvm with assertions enabled.

That is definitely not sufficient. The .ll test cases are only a small subset of real-world code and people only add to them to verify a new feature or to prove a bug is fixed. I recommend running test-suite to get a better representation of code out in the world. If you are uncertain how to do so please e-mail me and I'll provide detailed instructions. I'm hoping we can avoid stressful bug report debugging and LLVM code reverts/reapplies.

Jul 6 2018, 10:39 AM
brzycki added a comment to D48967: [Dominators] Convert existing passes and utils to use the DomTreeUpdater class.

Hello @NutshellySima , my comments are inline below. These changes are a good real-world usage of the new DTU class and should help us find any bugs or deficiencies. I'm curious to know what kind of testing you've done on this code: ninja check, test-suite, and/or test-suite for compile time?

Jul 6 2018, 8:30 AM

Jul 5 2018

brzycki added a comment to D48919: [Dominators] Convert existing passes to use DomTreeUpdater - 1.

One thing that is not clear to me is whether it's work to replace plain updates (DT->insert/deleteEdge, DT->applyUpdates) with DTU when it's clear that both are going to be functionally the same. The overhead of constructing DTU is probably negligible, but it's more verbose and I'm not sure what benefit it brings.

It definitely makes sense to use DTU when you are updating both DT and PDT, and when you can use the lazy strategy, or use some other functions that take DTU. Do you think it's worth to replace it everywhere for some other reason?

Jul 5 2018, 8:24 AM

Jul 2 2018

brzycki accepted D48383: [Dominators] Add the DomTreeUpdater class.

+1 LGTM. Thank you @NutshellySima for your dedication to getting this patch ready for adoption!

Jul 2 2018, 7:49 AM

Jun 29 2018

Herald updated subscribers of D48489: [MemDep] Use PhiValuesAnalysis to improve alias analysis results.
Jun 29 2018, 8:02 AM

Jun 28 2018

brzycki added a comment to D48383: [Dominators] Add the DomTreeUpdater class.

@kuhar and @NutshellySima here are a few more small comments.

Jun 28 2018, 4:35 PM

Jun 27 2018

brzycki added a comment to D48383: [Dominators] Add the DomTreeUpdater class.

If we agree that it would be better to have well-behaved updates in most of the places in the future, then having distinct names for these two should make it much easier to grep for. Additionally, it seems like it's not always possible to perform them with the eager strategy, is it?

Good point, I didn't think of the calls being easier to grep. Sounds good to me. And yes, I would prefer all updates to be strict and done after CFG updates, but reality isn't always so forgiving. Having explicit *Relaxed() calls can make them easier to identify and potentially fix as we go.

Jun 27 2018, 4:20 PM
brzycki added a comment to D48383: [Dominators] Add the DomTreeUpdater class.

Thank you for continuing this work @NutshellySima .

Jun 27 2018, 2:36 PM

Jun 26 2018

brzycki added a comment to D48111: [JumpThreading] Don't try to rewrite a use if it's already valid..

do you happen to know why?

The difference compared to the old SSAUpdater is in how we handled uses and defs located in the same block - it did not lead to a problem with the old SSAUpdater, but was unnecessary anyway, since such uses don't need to be rewritten (not in general, but in this particular case when the use is still dominated by its existing def).

Michael

Jun 26 2018, 5:23 PM

Jun 25 2018

brzycki added a comment to D48383: [Dominators] Add the DomTreeUpdater class.

Couple of quick, high level questions, in terms of how we expect the interface to be used. If there is a function that currently looks like this:

bool MergeBlockIntoPredecessor(BasicBlock *BB, DominatorTree *DT = nullptr,
                               LoopInfo *LI = nullptr,
                               MemoryDependenceResults *MemDep = nullptr,
                               DeferredDominance *DDT = nullptr);

.. and we'd like to make it optionally preserve PDT's, do you think it's best to have both an (optional) DT and a DTU? Just the DTU and get DT through it? Would the DTU remain optional or become required? (though possibly empty)

The idea is to have it only take DTU (by reference (non-optional)), such that MergeBlockIntoPredecessor preserves whatever you pass it (possibly no tree).

+1. I'd like to see most, if not all, of the updates to PDT/DT to be done through the DTU interface applyUpdate() call. It simplifies method signatures and provides a consistent interface to analysis and updates.

Jun 25 2018, 8:24 AM

Jun 22 2018

brzycki added a comment to D48181: [JumpThreading] Ignore nil destionation when determining whether a block only goes to a single destination.

Added small fix to suggested code.

Jun 22 2018, 10:14 AM
brzycki added a comment to D48181: [JumpThreading] Ignore nil destionation when determining whether a block only goes to a single destination.

My comments are inline. I'm also curious to know what kind of testing you've done on this (ninja check, ninja check-all, any test-suite runs, etc). I'd like more testing if it's just ninja check: the included tests are a small sampling of real-world code and test-suite is a bit better for this kind of change.

Jun 22 2018, 10:04 AM
brzycki added a comment to D48181: [JumpThreading] Ignore nil destionation when determining whether a block only goes to a single destination.

Hi @trentxintong , I am just starting to review this diff and will get back to you with more meaningful comments soon. While I'm doing that could you tell me if this work is from a bug you discovered or for a new feature you'd like to see implemented. Looking at your .ll changes below makes me believe this is a new feature, but I'd like to be sure to know.

Jun 22 2018, 8:45 AM

Jun 21 2018

brzycki added a comment to D48383: [Dominators] Add the DomTreeUpdater class.

Thank you for working on this @NutshellySima , a unified dominator interface will be most welcome in LLVM. Please see my inline comments.

Jun 21 2018, 11:38 AM
brzycki added a comment to D48111: [JumpThreading] Don't try to rewrite a use if it's already valid..

This problem seems to be aggravated by the new SSAUpdaterBulk() patch pushed on May 12 ( D44282 ) since the PR37745.ll test case does not fail before that date. I don't understand the SSA updater change enough to understand why this problem only happens with the new bulk updates, @mzolotukhin do you happen to know why?

Jun 21 2018, 8:16 AM

Jun 20 2018

brzycki added a comment to D48392: [Dominators] Simplify child lists and make them deterministic.

I always prefer deterministic execution (all other things being equal). Have you done any testing of performance impact to this change? The adding and erasing of Dominator elements may cost more with how the SmallVector type performs the operations.

Jun 20 2018, 1:27 PM

Mar 20 2018

brzycki updated subscribers of D44629: [CloneFunction] Preserve DT in DuplicateInstructionsInSplitBetween..

I'm not a reviewer, but it LGTM. You might want to ask @kuhar to review too.

Mar 20 2018, 8:08 AM

Mar 19 2018

brzycki added inline comments to D40146: [JumpThreading] Preservation of DT and LVI across the pass.
Mar 19 2018, 9:09 AM
brzycki added a comment to D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

Could this lead to problems when jump threading erases an edge?

Mar 19 2018, 8:34 AM