Page MenuHomePhabricator

brzycki (Brian Rzycki)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Mon, Dec 10

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

Awesome, thanks for doing this @amehsan .

Mon, Dec 10, 8:11 AM

Thu, Dec 6

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.

Thu, Dec 6, 10:10 AM

Thu, Nov 29

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

Thu, Nov 29, 7:54 AM

Mon, Nov 26

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
Mon, Nov 26, 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
Mon, Nov 26, 12:44 PM

Tue, Nov 20

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?

Tue, Nov 20, 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

Mar 16 2018

brzycki committed rL327713: [JumpThreading] Track unreachable BBs to avoid processing.
[JumpThreading] Track unreachable BBs to avoid processing
Mar 16 2018, 8:16 AM
brzycki closed D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.
Mar 16 2018, 8:16 AM
brzycki updated the diff for D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

Comment cleanup and rebase. NFC.

Mar 16 2018, 7:41 AM

Mar 15 2018

brzycki added a comment to D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

Thanks guys. I found a few spelling mistakes in the comments that I'm fixing and testing now. I'll upload the diff and push the change shortly if all goes well.

Mar 15 2018, 7:57 AM
brzycki added a comment to D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

Ping. @sebpop, @kuhar, or @dberlin any objections to pushing this patch as-is?

Mar 15 2018, 7:38 AM

Mar 13 2018

brzycki committed rL327432: [LazyValueInfo] PR33357 prevent infinite recursion on BinaryOperator.
[LazyValueInfo] PR33357 prevent infinite recursion on BinaryOperator
Mar 13 2018, 11:16 AM
brzycki closed D34135: [JumpThreading] Use DT to avoid processing dead blocks.
Mar 13 2018, 11:16 AM

Mar 12 2018

brzycki added a comment to D34135: [JumpThreading] Use DT to avoid processing dead blocks.

I'm a little worried about the duplicated code here. I feel like you are having to patch way too many points in the same function, and it will be easy to miss one in the future if someone changes the logicc slightly.

Is there really no better place to put this/way to refactor it?

If not, i'll approve, but just wanted to check.

Mar 12 2018, 11:00 AM
brzycki added a comment to D34135: [JumpThreading] Use DT to avoid processing dead blocks.

Gentle ping. Are there any objections to this patch? Should I open another Differential for this patch in preparation to commiting it?

Mar 12 2018, 7:38 AM

Mar 8 2018

brzycki updated the diff for D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

Addressed @kuhar 's comments.

Mar 8 2018, 3:45 PM
brzycki added a comment to D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

@kuhar I attempted to use a ranged iterator for (auto &BB : F) loop and it causes crashes in test-suite.

FAILED: Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/driver.cpp.o
/work/brzycki/test-suite/build/tools/timeit --summary Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/driver.cpp.o.time /work/brzycki/upstream/install/bin/clang++  -DNDEBUG -I../Bitcode/Benchmarks/Halide/bilateral_grid -IBitcode/Benchmarks/Halide/bilateral_grid -O3   -O3 -DNDEBUG   -w -Werror=date-time -std=c++11 -MD -MT Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/driver.cpp.o -MF Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/driver.cpp.o.d -o Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/driver.cpp.o -c ../Bitcode/Benchmarks/Halide/bilateral_grid/driver.cpp
While deleting: label %lpad142
Mar 8 2018, 10:58 AM

Mar 7 2018

brzycki added a comment to D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

Hi @kuhar , thanks for the review. As a general comment I tried to minimize changes to JumpThreading to remove removeUnreachableBlocks.

Mar 7 2018, 2:47 PM
brzycki updated the diff for D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

Renamed InvalidBlocks to UnreachableBlocks, minor tweaks to comments.

Mar 7 2018, 8:29 AM
brzycki added inline comments to D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.
Mar 7 2018, 8:28 AM
brzycki updated the summary of D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.
Mar 7 2018, 8:11 AM

Mar 6 2018

brzycki added a comment to D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.

Here are 4 ctmark runs on x86_64 of compile_time results. The tip compiler is llvm-project SHA 7a365bd12dd84be1e7c9ca6d9a2268b80f604ae9. The upstream compiler is the same as tip with this patch applied.

Mar 6 2018, 4:15 PM
brzycki created D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions.
Mar 6 2018, 3:57 PM
brzycki added a comment to D34135: [JumpThreading] Use DT to avoid processing dead blocks.

I have analyzed this failure and have what I believe (in @dberlin 's words) to be the smallest hammer fix:

$ git diff lib/Analysis/LazyValueInfo.cpp
diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
index 65fd007dc0b..36e66f7f6ef 100644
--- a/llvm/lib/Analysis/LazyValueInfo.cpp
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp
@@ -1145,9 +1145,17 @@ getValueFromConditionImpl(Value *Val, Value *Cond, bool isTrueDest,
              (!isTrueDest && BO->getOpcode() != BinaryOperator::Or))
     return ValueLatticeElement::getOverdefined();
Mar 6 2018, 2:18 PM

Feb 26 2018

brzycki accepted D41298: [Dominators] Remove verifyDomTree and add some verifying for Post Dom Trees.

+1 LGTM as well.

Feb 26 2018, 8:04 AM

Feb 21 2018

brzycki closed D37575: [BasicBlock] add new function removeEdge().

This method is too expensive as it is designed today.

Feb 21 2018, 8:07 AM

Feb 19 2018

brzycki added a comment to D43450: [GSoC] Dominators project proposal.

+1 from me. I'd love to see a single interface for all Dominator-related analysis or updates. This idea reminds me (a bit) of a comment @dberlin made in D43173 about an interface similar to getBestSimplifyQuery to reduce the number of functions that need explicit pointers to a DT, DDT, PDT or other analysis object. I don't quite understand how this would work but I wanted to bring it up in case the work could be done in a way to anticipate such an interface to reduce re-work.

Feb 19 2018, 1:15 PM

Feb 16 2018

brzycki committed rL325356: [JumpThreading] PR36133 enable/disable DominatorTree for LVI analysis.
[JumpThreading] PR36133 enable/disable DominatorTree for LVI analysis
Feb 16 2018, 8:38 AM
brzycki closed D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).
Feb 16 2018, 8:38 AM

Feb 15 2018

brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

It's looks ok for me but I have not contributed enough to give lgtms.

Feb 15 2018, 11:00 AM

Feb 14 2018

brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

@a.elovikov and @dberlin does this look good to commit?

Feb 14 2018, 11:29 AM
brzycki updated the diff for D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

Added asserts to enableDT / disableDT to verify both DT and DisabledDT are never both valid pointers. Added a final LVI->enabledDT() at the very end of runImpl to allow subsequent passes to use DT with the cached LVI object.

Feb 14 2018, 8:35 AM
brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

I might be missing something, but why don't we enable DT back in LVI after the JT pass finishes and flushes the DDT? Is it possible that we end up with disabled DT *after* the JT?

Feb 14 2018, 8:31 AM

Feb 13 2018

brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

IMHO, if you were going to go that route, LVI checks whether DT is null, and so you could wrap this all in a "UpdatedDTOrNull" object that, when cast to a DominatorTree *, returned the tree if it was up to date, or a null pointer if it was not.

It's a bit more subtle than that @dberlin due to the way LazyValueInfo is defined. Because the LazyValueInfoImpl object is cached and re-used it would store a pointer to DT (or nullptr) at the instance of creating the first cachced LVI instance. I ran into this problem with my first attempt to disable DT within LVI and is another reason I decided on the new external methods.

Feb 13 2018, 3:22 PM
brzycki updated the diff for D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

Use std::swap() in enableDT/DisableDT methods.

Feb 13 2018, 3:18 PM
brzycki updated the diff for D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

Rebased to tip, switched to always enabling or disabling before calling LVI analysis. Prevents LVI from being accidentally enabled.

Feb 13 2018, 2:12 PM
brzycki added inline comments to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).
Feb 13 2018, 8:21 AM

Feb 12 2018

brzycki added a comment to D43216: [Dominators] Ensure that every PostDomTree root is connected to the virtual node.

Hi @kuhar , is there a testcase that causes this to fail?

Feb 12 2018, 6:32 PM
brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

I used clang -O3 -fno-exceptions -ftime-report -c tramp3d-v4.cpp -o /tmp/foo.o with an upstream (non-patched) compiler. Here are the hottest areas of pass execution according to the reports. These lines are displayed in the same order:

Feb 12 2018, 4:34 PM
brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

Six runs of tramp3d-v4 with this patch applied show comperable variance and similar low/high bounds:

Feb 12 2018, 2:09 PM
brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

The variance of tramp3d-v4 is considerable, as much as 18.5%. I ran CTMark on the pre-DDT compiler and here are the results:

Feb 12 2018, 1:27 PM
brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

I'm seeing considerable variance on the tramp3d-v4 compile time test before and after the preservation of DT on JumpThreading patch:

Feb 12 2018, 11:56 AM
brzycki added inline comments to D43173: [CallSiteSplitting] Preserve DominatorTreeAnalysis..
Feb 12 2018, 9:39 AM
brzycki added a comment to D43140: [Dominators] Always recalculate postdominators when update yields different roots.

I haven't checked, but from my previous experiments I doubt it does -- forming or to infinite loops are very rare, and this piece of code was only executed a few times when compiling clang or sqlite. I don't have numbers for it, but could get them later this week if someone's really interested.

Feb 12 2018, 7:53 AM
brzycki accepted D43140: [Dominators] Always recalculate postdominators when update yields different roots.

LGTM. Out of curiosity does this change impact compile time?

Feb 12 2018, 7:43 AM

Feb 9 2018

brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

@dberlin thank you for the suggestion and I have implemented the change. The good news is sqlite is back to our original performance. The not-so-great news is this causes a regression for tramp3d-v4:

Feb 9 2018, 9:10 AM
brzycki updated the diff for D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

Added a feature to LVI to enable or disable the use of the DominatorTree for analysis. Updated JumpThreading to disable the use of DT for LVI analysis when there are pending updates in the DDT.

Feb 9 2018, 9:05 AM

Feb 7 2018

brzycki added a comment to D42717: [JumpThreading] sync DT for LVI analysis (PR 36133).

@kuhar and @dberlin I would appreciate any guidance you two may have. I spoke with @sebpop about this problem and we don't see a way around the performance impact to preserve DT for LVI analysis. As it stands now there are cases where the deferred updates change the LVI analysis sufficiently to cause a miscompile. We do think there may be ways to mitigate this loss in the future: one way is to prevent pointless threads where a BB is threaded only to be discarded in a subsequent loop iteration within JumpThreading.

Feb 7 2018, 1:33 PM