Page MenuHomePhabricator

kuhar (Jakub Kuderski)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 15 2015, 7:49 AM (183 w, 4 d)

Recent Activity

Mon, Jan 7

kuhar added a comment to D56284: [UnrollRuntime] Fix domTree failure in multiexit unrolling.

I've got a naive question about the DTU: Does the automatic updater handle the ImmediateDominator "automatically" for the remaining nodes in the dom tree once identify that one of the nodes in the DT has a change in IDom?

Exactly. Given a set of CFG updates, if figures out how to change the DomTree and PostDomTree such that they match the CFG.

Mon, Jan 7, 1:25 PM
kuhar added a comment to D56284: [UnrollRuntime] Fix domTree failure in multiexit unrolling.

the same bug can happen with:

  1. latch exit
  2. non-immediate successors of latchexit/otherexit.

    I have simplified test cases for each of these and we need a more general fix. Working on this.
Mon, Jan 7, 12:16 PM

Thu, Jan 3

kuhar added a comment to D56284: [UnrollRuntime] Fix domTree failure in multiexit unrolling.

@anna Can this be rewritten to use the DomTreeUpdater utility?

@kuhar, I have not looked at the DTU uttility yet, but I'd think it can be rewritten (unless there are some limitations for the DomTreeUpdater utility). However, that would be a large enough change and I think it's better to do it as a separate change at a later point rather than as part of fixing this bug. Also, there are enough DT updates through out this code that it will take a while to get through the whole process of porting over for runtime unrolling.

Thu, Jan 3, 3:19 PM
kuhar added a comment to D56284: [UnrollRuntime] Fix domTree failure in multiexit unrolling.

@anna Can this be rewritten to use the DomTreeUpdater utility? We have large number of examples in the codebase already, so the question is whether this affects the performance in this case. Historically, we had tons of bugs in manual DomTree updates and I'd rather we replaces as much as possible using the incremental updater.

Thu, Jan 3, 3:00 PM

Nov 20 2018

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

Fundamentally, does this have to be an expensive operation? It seems like there should be some way to keep a "local" operation like this from propagating across the entire tree.

Nov 20 2018, 2:06 PM
kuhar added a comment to D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor..

Though I don't have bitcode samples to reproduce your findings, I guess three times faster is caused by the DomTree is consuming a lot of time in DeleteUnreachable().

Maybe we can put some high level APIs that have these best practices inside DTU such as something like updateAfterSpliceBlocks and updateAfterChangePredecessorTo or just add a sort after running updates legalization. :)

Nov 20 2018, 11:45 AM
kuhar added inline comments to D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor..
Nov 20 2018, 11:34 AM
kuhar accepted D54732: [LoopUnroll] Don't verify domtree by default with +Asserts..
Nov 20 2018, 11:11 AM

Nov 19 2018

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

We experimented with different ordering of updates before, including scheduling deletions before insertions, but didn't discover any promising strategies. Because of that, updates are performed in the exact order they were scheduled (except optimizing out trivially redundant ones).

Nov 19 2018, 3:12 PM
kuhar added a reviewer for D54730: [DomTree] Fix order of domtree updates in MergeBlockIntoPredecessor.: NutshellySima.
Nov 19 2018, 3:08 PM

Nov 13 2018

kuhar added inline comments to D54312: [CSP, Cloning] Update DuplicateInstructionsInSplitBetween to use DomTreeUpdater..
Nov 13 2018, 8:33 AM

Nov 9 2018

kuhar added inline comments to D54312: [CSP, Cloning] Update DuplicateInstructionsInSplitBetween to use DomTreeUpdater..
Nov 9 2018, 7:15 AM

Nov 8 2018

kuhar accepted D47259: [IPSCCP,PM] Preserve DT in the new pass manager..

Looks good to me now, thanks for the fixes! You may want for somebody else to take a look at it, though.

Nov 8 2018, 10:04 AM
kuhar added a reviewer for D47259: [IPSCCP,PM] Preserve DT in the new pass manager.: NutshellySima.
Nov 8 2018, 7:21 AM

Oct 25 2018

kuhar 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?

Oct 25 2018, 11:26 PM
kuhar accepted D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929).

LGTM. Thanks for looking into this!

Oct 25 2018, 10:23 AM

Oct 24 2018

kuhar 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 .

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.

Oct 24 2018, 8:18 AM
kuhar 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:07 AM

Oct 16 2018

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

@kuhar , here is the statistics for k*α>n:

Awesome, thanks for the numbers. It's much easier to reason about it this way. But I'm a bit confused by why you measured it like this. Can we have statistics with respect to nodes visited during DFS? That should be highly correlated and give us more confidence in the measured times.

I think maybe "40" is a better choice?

30 seems best to me :)

Oct 16 2018, 8:34 AM

Oct 15 2018

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

Do you think for the unittests of the incremental updater, the updater should directly apply updates on a small k (don't do recalculation with few updates) or n (don't do recalculation on a small graph)?

I think that for small n it shouldn't recalculate when k < n.

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

I have tested heuristic k>n/α with α=10/40/65/70/75/80/100/300/1000 on clang.bcs. (But I didn't save them to a file :\) 10/1000 produce very bad result. ("10" is better than the current implementation). 300 produces better result than 1000. 40/100 produce better ones than 10/300/1000. And 65-80 don't make much difference and produce the best results among all αs I selected.

I'd be very interested to see the numbers for these constants.

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

For k>n you mentioned, I think you mean k>n/α (α is a constant) because for the reproducer in 37929, the minimum value of α to make the time consumed by clang acceptable is about 3 (α should definitely be larger than 3). But selecting a higher value like "75" can also boost other inputs more (about 2x~4.5x on clang*.bcs).

OK, makes sense if that the case in the bug report.

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

Interesting. Nice improvements. What about small trees? It would seem that any tree less that 75 nodes would always be recalculated. Do the timings you ran include things to show that this is better? Or was that just looking at larger trees at the time?

I haven't conducted dedicated tests on graphs with the number of vertices less than 75, but among all the domtrees recalculated in the two clang-*.bc I mentioned above, there are approximately 63% of them has an n smaller than 75.

I'd be much more comfortable if this change didn't always recalculate for graphs smaller than 75. I think there is a lot of value in making sure the incremental updater is behaving correctly on small examples (e.g., existing unit tests, most regression tests, etc.). If for some reason it stops working, it will be much easier to detect and fix it on these small examples.

Oct 15 2018, 7:07 AM

Oct 13 2018

kuhar added a comment to D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929).
  • For the constant "650", I used the 2nd reproducer in bug PR37929 and collected some statistics on the size of the updates after legalization and the time used by applying updates. The statistics shows that the time complexity is about Θ(k^2.4) (let k be the size of the update vector after legalization) [I have just checked the original paper, it is O(m min{n, k} + kn), so about Ω(k*(k+n)) in the case of the reproducer] and it seems that at least for a CFG size no larger than the one in PR37929, "650" is a reasonable value that the time used by recalculating is sure to be less than applying updates directly. (I think there sure should be some better heuristics deciding this.)

Thanks for the detailed explanation. This is what I expected. I don't it's enough to use 650 in the code without any explanation or justification -- even if it works in practise, I think it deserves a comment.

  • It is definitely better to get it related to the tree size but I'm lack of inputs with different CFG sizes to experiment on.

You can take intermediate bitcode files when doing full-LTO built using lld. I think the command that does the trick is --save-temps=DIR or similar.

  • I acknowledged this as an issue of the incremental updater not handling a huge update vector correctly; It definitely should fallback to recalculating if it is much faster. I believe this constant is related to the size of updates after legalization so I put it in the incremental updater. [Currently, there is a similar functionality in DTU which is Ω(n^2) and I believe it is slow and (redundant if JT submits updates normally) so I am considering removing it from DTU, then it is impossible to know the size of the legalized update vector]. Besides, it can also benefit other places not using the DomTreeUpdater.

I see. If we expect to see many redundant updates, then it makes sense to check that after legalization.

Oct 13 2018, 2:39 PM
kuhar added a comment to D53245: Teach the DominatorTree fallback to recalculation when applying updates to speedup JT (PR37929).

Thanks for looking into this. A couple of high-level questions:

  • how was the constant "650" picked? Is it just playing with different values and choosing something that seemed reasonable?
  • why is the threshold independent of the tree size? Intuitively the policy could be to recalculate if the number of updates is greater than some multiple of size. Maybe we can refer to the average-case complexity from Lucuas' paper?
  • why do we want to put it in the incremental updater instead of the DomTreeUpdater class?
Oct 13 2018, 8:35 AM

Sep 24 2018

kuhar added inline comments to D51664: [IR] Lazily number instructions for local dominance queries.
Sep 24 2018, 10:24 AM
kuhar added inline comments to D51664: [IR] Lazily number instructions for local dominance queries.
Sep 24 2018, 10:24 AM

Sep 21 2018

kuhar added inline comments to D51664: [IR] Lazily number instructions for local dominance queries.
Sep 21 2018, 1:08 PM

Sep 4 2018

kuhar added a comment to D51200: Introduce per-callsite inline intrinsics.

Thank you for the comments, Richard!

Sep 4 2018, 1:33 PM · Restricted Project
kuhar added a comment to D51200: Introduce per-callsite inline intrinsics.

+rjmccall for CodeGen changes

Sep 4 2018, 1:28 PM · Restricted Project

Aug 24 2018

kuhar updated the diff for D51200: Introduce per-callsite inline intrinsics.
Aug 24 2018, 5:54 PM · Restricted Project
kuhar updated the diff for D51200: Introduce per-callsite inline intrinsics.

Rename missed CallInlineKind parameters to CIK

Aug 24 2018, 5:50 PM · Restricted Project
kuhar edited reviewers for D51200: Introduce per-callsite inline intrinsics, added: Quuxplusone, amharc; removed: grosser.
Aug 24 2018, 4:08 PM · Restricted Project
kuhar added inline comments to D51200: Introduce per-callsite inline intrinsics.
Aug 24 2018, 4:02 PM · Restricted Project
kuhar updated the diff for D51200: Introduce per-callsite inline intrinsics.

Fix naming and add more tests.

Aug 24 2018, 4:02 PM · Restricted Project
kuhar added a comment to D51200: Introduce per-callsite inline intrinsics.

One extra question: do you think this deserves a separate RFC?

Aug 24 2018, 8:58 AM · Restricted Project

Aug 23 2018

kuhar added a comment to D51200: Introduce per-callsite inline intrinsics.

Disclaimer: I'm a Clang newbie and I'm not sure if that's a good way to implement these intrinsics. I'm not sure about the following things:

  • The new enum CallInlineKind may not be in the right place
  • Not sure if adding the extra parameter to EmitSomething methods is the cleanest thing possible -- some functions have it now, some don't, and it makes argument lists longer
  • Not sure if just adding an extra attribute at each callsite is enough -- I'm not sure what is supposed to happen when there are conflicting inline attributes at call sites and function declarations (e.g. __builtin_always_inline and __attribute__ ((always_inline)))
  • I don't know how to handle constexpr functions
  • I don't know how to implement it for constructor calls
Aug 23 2018, 5:34 PM · Restricted Project
kuhar created D51200: Introduce per-callsite inline intrinsics.
Aug 23 2018, 5:29 PM · Restricted Project

Aug 20 2018

kuhar added inline comments to D45299: API to update MemorySSA for cloned blocks..
Aug 20 2018, 1:54 PM

Aug 17 2018

kuhar accepted D50675: [IDF] Teach Iterated Dominance Frontier to use a snapshot CFG based on a GraphDiff..

Seems fine

Aug 17 2018, 10:11 AM

Aug 16 2018

kuhar accepted D50671: [DomTree] Add constructor to create a new DT based on current DT/CFG and a set of Updates..
Aug 16 2018, 2:19 PM

Aug 14 2018

kuhar accepted D50717: [SimplifyCFG] Remove pointer from SmallPtrSet before deletion.
Aug 14 2018, 10:21 AM
kuhar added a comment to D50717: [SimplifyCFG] Remove pointer from SmallPtrSet before deletion.

I'm no expert, but I thought that SmallPtrSet->erase(BB) would treat BB as an opaque value that it removes from it's set, not accessing it's content in any way. You may be right about the eraseFromParent invalidating things though.

It's certainly much better this new way :)

Aug 14 2018, 10:18 AM
kuhar added a comment to D50717: [SimplifyCFG] Remove pointer from SmallPtrSet before deletion.

I guess because it's only erasing it from a SmallPtrSet, it's not actually _using_ it, but this looks like a very sensible change.

Aug 14 2018, 9:55 AM

Aug 13 2018

kuhar added a comment to D50671: [DomTree] Add constructor to create a new DT based on current DT/CFG and a set of Updates..

Looks fine.

Aug 13 2018, 4:58 PM
kuhar accepted D50669: [DomTree] Cleanup Update and LegalizeUpdate API moved to Support header..

LGTM

Aug 13 2018, 4:39 PM
kuhar added inline comments to D50671: [DomTree] Add constructor to create a new DT based on current DT/CFG and a set of Updates..
Aug 13 2018, 4:37 PM
kuhar added inline comments to D50675: [IDF] Teach Iterated Dominance Frontier to use a snapshot CFG based on a GraphDiff..
Aug 13 2018, 4:33 PM
kuhar accepted D50479: Expose CFG Update struct. Define GraphTraits to get children given a snapshot CFG..

Thanks for the changes. LGTM.

Aug 13 2018, 10:23 AM

Aug 10 2018

kuhar added inline comments to D50479: Expose CFG Update struct. Define GraphTraits to get children given a snapshot CFG..
Aug 10 2018, 1:47 PM
kuhar added inline comments to D50479: Expose CFG Update struct. Define GraphTraits to get children given a snapshot CFG..
Aug 10 2018, 12:05 PM

Aug 9 2018

kuhar added a comment to D50479: Expose CFG Update struct. Define GraphTraits to get children given a snapshot CFG..

I agree that long term this can and should be reused in DomTree, so I'm trying to make it have the right functionality.
Another possibly related issue, is that I'm not reversing the children, as in DomTree, not sure the reason behind that one.

Sure. The children are being reversed to preserve the successor order in DFS, which uses stack and reverses them (again) internally. But this reverse can be moved somewhere else and you don't have to worry about it.

Aug 9 2018, 1:07 PM
kuhar added a comment to D50479: Expose CFG Update struct. Define GraphTraits to get children given a snapshot CFG..

Another issue with the above traits: is there a way to avoid some of the code duplication, since the only differences are the bool arguments to GraphDiff and succ/pred iterators?

Aug 9 2018, 1:05 PM

Aug 8 2018

kuhar updated subscribers of D50479: Expose CFG Update struct. Define GraphTraits to get children given a snapshot CFG..

One thing that is not clear to me is how you decide whether to apply the updates on top of the current CFG or to reverse-apply them. DomTreeConstruction needs the latter, but I don't know what makes sense in your use-case.

Aug 8 2018, 5:06 PM

Aug 2 2018

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

Thanks for the changes!

Aug 2 2018, 12:50 PM
kuhar added inline comments to D49982: [TailCallElim] Preserve DT and PDT.
Aug 2 2018, 11:52 AM
kuhar accepted D49738: [Dominators] Make RemoveUnreachableBlocks return false if the BasicBlock is already awaiting deletion.
Aug 2 2018, 9:40 AM
kuhar accepted D49988: [ADCE] Remove the need of DomTree.
Aug 2 2018, 9:05 AM
kuhar accepted D50173: [Dominators] Refine the logic of recalculate() in the DomTreeUpdater.

Looks good, thanks for the patch!

Aug 2 2018, 9:01 AM
kuhar added inline comments to D49988: [ADCE] Remove the need of DomTree.
Aug 2 2018, 8:57 AM
kuhar added inline comments to D49982: [TailCallElim] Preserve DT and PDT.
Aug 2 2018, 8:51 AM
kuhar accepted D49747: [Dominators] Remove the DeferredDominance class.

Nit: It's removal and not deprecation.
Looks good otherwise.

Aug 2 2018, 8:42 AM
kuhar added a comment to D49738: [Dominators] Make RemoveUnreachableBlocks return false if the BasicBlock is already awaiting deletion.

Looks fine except for the test IR.

Aug 2 2018, 8:36 AM

Jul 31 2018

kuhar committed rL338396: [Dominators] Make slow walks shorter.
[Dominators] Make slow walks shorter
Jul 31 2018, 8:53 AM
kuhar closed D49955: [Dominators] Make slow walks shorter.
Jul 31 2018, 8:53 AM

Jul 28 2018

kuhar created D49955: [Dominators] Make slow walks shorter.
Jul 28 2018, 1:13 AM

Jul 27 2018

kuhar committed rL338184: [Dominators] Make applyUpdate's documentation less confusing [NFC].
[Dominators] Make applyUpdate's documentation less confusing [NFC]
Jul 27 2018, 5:54 PM
kuhar closed D49944: [Dominators] Make applyUpdate's documentation less confusing [NFC].
Jul 27 2018, 5:54 PM
kuhar retitled D49944: [Dominators] Make applyUpdate's documentation less confusing [NFC] from [Dominators] Make applyUpdate's documentation less confusing to [Dominators] Make applyUpdate's documentation less confusing [NFC].
Jul 27 2018, 3:21 PM
kuhar added a comment to D49925: [SimpleLoopUnswitch] Fix DT updates for trivial branch unswitching..

@chandlerc OK, now I finally understand why it's possible to get confused here. I put a patch with modified documentation here: D49944.

Jul 27 2018, 3:20 PM
kuhar created D49944: [Dominators] Make applyUpdate's documentation less confusing [NFC].
Jul 27 2018, 3:20 PM
kuhar updated subscribers of D49925: [SimpleLoopUnswitch] Fix DT updates for trivial branch unswitching..

@chandlerc FYI, all existing uses of the 'raw' api (DT.insert/deleteEdge, DT.applyUpdates) are going to be replaced with the new unified API after branching to 8.0. @NutshellySima is working on this.
The plan is also to hide insert/deleteEdge completely, so that we won't have bugs like this in the future.

Jul 27 2018, 2:22 PM
kuhar accepted D49925: [SimpleLoopUnswitch] Fix DT updates for trivial branch unswitching..

This case requires batch updater. It's always surprising that these kinds of bugs can stay undetected for months.

Jul 27 2018, 1:44 PM

Jul 25 2018

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

Thanks for the changes, Chijun!

Jul 25 2018, 11:29 AM

Jul 24 2018

kuhar accepted D49731: [Dominators] Assert if there is modification to DelBB while it is awaiting deletion.

Thanks for the comment!
LGTM.

Jul 24 2018, 10:01 AM
kuhar added inline comments to D49731: [Dominators] Assert if there is modification to DelBB while it is awaiting deletion.
Jul 24 2018, 8:44 AM

Jul 11 2018

kuhar accepted D48974: [DomTreeUpdater] Ignore updates when both DT and PDT are nullptrs.

LGTM, but please wait to see if @brzycki has some comments before submitting the patch

Jul 11 2018, 10:20 AM
kuhar accepted D49056: [Dominators] Add isUpdateLazy() method to the DomTreeUpdater.

Looks like a nice improvement!

Jul 11 2018, 8:51 AM

Jul 9 2018

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

A general high-level question/suggestion: would it be possible to split this patch such that we don't change all of the uses of DDT and plain updates at once? I'm afraid this can subtly break in many places, and the patch will have to be reverted a couple of times before you get them right. If you do it with more granularity, then it should be easier to land each part and test it in isolation.
Is it possible, or do you have a dependency on the utility functions from Local.cpp and other helper files?

Jul 9 2018, 9:01 AM
kuhar added a comment to D48974: [DomTreeUpdater] Ignore updates when both DT and PDT are nullptrs.

Have you considered detecting that both trees are null and overriding the update strategy to Eager in that case? I think that should functionally be equivalent, although more confusing when the strategy you get can be different from what someone just set :P I'm just wondering what pros/cons there are.

Jul 9 2018, 8:40 AM
kuhar 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, 8:29 AM

Jul 4 2018

kuhar committed rL336294: [Dominators] Add DomTreeUpdater constructor from DT* and PDT*.
[Dominators] Add DomTreeUpdater constructor from DT* and PDT*
Jul 4 2018, 11:42 AM
kuhar closed D48923: [Dominators] Add DomTreeUpdater constructor from DT* and PDT*.
Jul 4 2018, 11:42 AM
kuhar 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.

Jul 4 2018, 11:15 AM
kuhar accepted D48923: [Dominators] Add DomTreeUpdater constructor from DT* and PDT*.

LGTM. Should make life easier.

Jul 4 2018, 10:51 AM

Jul 2 2018

kuhar committed rL336163: Reappl "[Dominators] Add the DomTreeUpdater class".
Reappl "[Dominators] Add the DomTreeUpdater class"
Jul 2 2018, 7:11 PM
kuhar closed D48383: [Dominators] Add the DomTreeUpdater class.
Jul 2 2018, 7:11 PM
kuhar committed rL336117: Revert "[Dominators] Add the DomTreeUpdater class".
Revert "[Dominators] Add the DomTreeUpdater class"
Jul 2 2018, 9:15 AM
kuhar committed rL336114: [Dominators] Add the DomTreeUpdater class.
[Dominators] Add the DomTreeUpdater class
Jul 2 2018, 8:42 AM
kuhar closed D48383: [Dominators] Add the DomTreeUpdater class.
Jul 2 2018, 8:42 AM

Jul 1 2018

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

Looks good to me now. Thanks for making all the fixes!

Jul 1 2018, 6:21 PM
kuhar accepted D47103: Implement strip.invariant.group.
Jul 1 2018, 3:47 PM

Jun 29 2018

kuhar added inline comments to D48383: [Dominators] Add the DomTreeUpdater class.
Jun 29 2018, 11:27 AM
kuhar added inline comments to D48383: [Dominators] Add the DomTreeUpdater class.
Jun 29 2018, 10:20 AM

Jun 28 2018

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

Looks almost ready to me. Found just a couple of nits.

Jun 28 2018, 8:55 AM

Jun 27 2018

kuhar added a comment to D48383: [Dominators] Add the DomTreeUpdater class.
  1. Add insertEdgeRelaxed() and deleteEdgeRelaxed() to cover the behavior of the original DeferredDominance's insertEdge()/deleteEdge().

Maybe it'd be better to have a default boolean third variable?
deleteEdge(BasicBlock *From, BasicBlock *To, bool Relaxed = false) { ...

Jun 27 2018, 2:41 PM
kuhar abandoned D43216: [Dominators] Ensure that every PostDomTree root is connected to the virtual node.
Jun 27 2018, 10:00 AM
kuhar committed rL335751: [AliasSet] Fix UnknownInstructions printing.
[AliasSet] Fix UnknownInstructions printing
Jun 27 2018, 9:39 AM
kuhar closed D48609: [AliasSet] Fix UnknownInstructions printing.
Jun 27 2018, 9:39 AM