Page MenuHomePhabricator

kuhar (Jakub Kuderski)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Tue, Sep 8

kuhar accepted D87319: [DomTree] Use SmallVector<DomTreeNodeBase *, 4> instead of std::vector..

Thanks for the numbers. LGTM.

Tue, Sep 8, 12:39 PM · Restricted Project
kuhar accepted D81558: [NewPM] Introduce PreserveCFG check.

Thanks for the table, this looks good to me now, modulo the two remaining nits. I'd be more comfortable if somebody familiar with the new pass manager reviewed this too.

Tue, Sep 8, 7:05 AM · Restricted Project

Thu, Sep 3

kuhar added inline comments to D81558: [NewPM] Introduce PreserveCFG check.
Thu, Sep 3, 9:43 PM · Restricted Project

Tue, Sep 1

kuhar added inline comments to D81558: [NewPM] Introduce PreserveCFG check.
Tue, Sep 1, 11:19 AM · Restricted Project
kuhar added inline comments to D81558: [NewPM] Introduce PreserveCFG check.
Tue, Sep 1, 10:01 AM · Restricted Project
kuhar added a comment to D81558: [NewPM] Introduce PreserveCFG check.

This looks very useful. Does it currently report any errors when enabled?

Tue, Sep 1, 9:02 AM · Restricted Project

Aug 21 2020

kuhar accepted D85472: [DomTree] Extend update API to allow a post CFG view..
Aug 21 2020, 2:29 PM · Restricted Project

Aug 19 2020

kuhar added a comment to D83088: Introduce CfgTraits abstraction.

Hi Nicoali,

Aug 19 2020, 2:21 PM · Restricted Project, Restricted Project, Restricted Project

Aug 17 2020

kuhar committed rGeed6476a8744: Reset PAL metadata when AMDGPU traget stream finishes (authored by s-perron).
Reset PAL metadata when AMDGPU traget stream finishes
Aug 17 2020, 8:00 AM
kuhar closed D85667: Reset PAL metadata when AMDGPU traget stream finishes.
Aug 17 2020, 8:00 AM · Restricted Project

Aug 11 2020

kuhar added a comment to D85472: [DomTree] Extend update API to allow a post CFG view..

This makes sense to me. I think the only missing piece is some documentation which also explains how to interpret when PostView is missing (PostView == nullptr ===> Current CFG is the implicit PostView?)

Aug 11 2020, 9:24 AM · Restricted Project

Aug 5 2020

kuhar added inline comments to D81558: [NewPM] Introduce PreserveCFG check.
Aug 5 2020, 7:59 AM · Restricted Project

Aug 4 2020

kuhar requested changes to D81558: [NewPM] Introduce PreserveCFG check.

I think this deserves some extra documentation that explains what it means to preserve CFG in plain English. For example, should we also care about preserving the number of return blocks?

Aug 4 2020, 11:40 AM · Restricted Project
kuhar accepted D84763: DomTree: Make PostDomTree immune to block successors swap.
Aug 4 2020, 11:30 AM · Restricted Project

Jul 31 2020

kuhar added a comment to D84763: DomTree: Make PostDomTree immune to block successors swap.

The code looks fine to me. Have you checked if compilation times are affected by this patch?

Jul 31 2020, 9:02 AM · Restricted Project

Jul 29 2020

kuhar resigned from D84279: AMDGPU/GlobalISel: Handle llvm.amdgcn.reloc.constant.

The patch looks fine to me, but I'm not familiar enough with the backend and GlobalIsel to review it.

Jul 29 2020, 10:02 AM · Restricted Project

Jul 28 2020

kuhar added a comment to D84763: DomTree: Make PostDomTree immune to block successors swap.

One more thing: should this be backported for the upcoming 11.0 release?

Jul 28 2020, 11:01 PM · Restricted Project
kuhar added a comment to D84763: DomTree: Make PostDomTree immune to block successors swap.

Taking a few steps back, I'd love to see a cfg preservation check like D81558 ...

I'm about to update the patches.

While this makes root finding insensitive to successors' order, I think it still may be possible to run into tree instability by changing the order of tree children (also plain domtree),...

I feel I do not understand. Any example?

and in turn changing DFS numbers used by other analyses. ...

I agree that DFS numbers are changed by successors swap, but can we make an exclusion for DFS numbers? If not, then this patch is useless because successors swap change CFG by definition (DFS numbers will be changes if we recalculate them).

AFAIK tree updates would still be based on some Cfg/Graph traits, which may give you unstable order.

As I see successors swap cannot be encoded for DomTreeUpdater. So this kind of change are not tracked.

Jul 28 2020, 11:00 PM · Restricted Project
kuhar accepted D84713: [DominatorTree] Simplify ChildrenGetter..
Jul 28 2020, 3:34 PM · Restricted Project, Restricted Project
kuhar added a comment to D84763: DomTree: Make PostDomTree immune to block successors swap.

I think this is a good direction.

Jul 28 2020, 11:58 AM · Restricted Project
kuhar added a comment to D84763: DomTree: Make PostDomTree immune to block successors swap.

Given that you want to fix InstCombine not to swap successors and move this to SimplifyCFG, does this patch improve anything in the current pass pipeline?

I don't believe that is the correct fix, because it literally says "nope, there is no way to update PDT after swapping successors other than full domtree recomputation", that is why i'm pushing for fixing PDT.

Jul 28 2020, 11:48 AM · Restricted Project
kuhar added a comment to D84493: [InstCombine] Disable branch predicate canonicalization.

<..> you have to chose some arbitrary way of selecting a fake-entry node from nodes in an scc. Right now this order is whatever children or inverse_children return, but could be based on the order of blocks in a function instead.

Ok. Let us try now. But why did not they do it a few years ago then?

Jul 28 2020, 11:43 AM · Restricted Project
kuhar added a comment to D84492: [SimplifyCFG] Enable branch predicate canonicalization by default.

Would it make sense to enable the canonicalization in SimplifyCFG and disable the one in InstCombine as a single patch?

Jul 28 2020, 11:38 AM · Restricted Project
kuhar added inline comments to D84491: [SimplifyCFG] Canonicalize branch predicates.
Jul 28 2020, 11:36 AM · Restricted Project
kuhar requested changes to D84763: DomTree: Make PostDomTree immune to block successors swap.

Thanks for pushing on this.

Jul 28 2020, 11:21 AM · Restricted Project
kuhar added reviewers for D84763: DomTree: Make PostDomTree immune to block successors swap: kuhar, asbirlea.
Jul 28 2020, 10:35 AM · Restricted Project
kuhar accepted D83093: DomTree: Define GraphTraits for GenericDomTreeBase.
Jul 28 2020, 7:51 AM · Restricted Project

Jul 27 2020

kuhar accepted D83092: DomTree: Add climbLhsUntilSiblings helper.
Jul 27 2020, 5:33 PM · Restricted Project
kuhar accepted D84713: [DominatorTree] Simplify ChildrenGetter..

One tiny nit: the function name ChildrenGet sounds kind of backwards to me, but it seems like the other direction is already taken.

Jul 27 2020, 5:28 PM · Restricted Project, Restricted Project

Jul 8 2020

kuhar accepted D83089: DomTree: Extract (mostly) read-only logic into type-erased base classes.

Thanks for checking this. I dug up some old whole-program bitcode and uploaded it here in case it helps with future code reviews: https://drive.google.com/drive/folders/1VJpym19cW-8BVgdtl2MsD3zB4CoEQ93O?usp=sharing

Jul 8 2020, 12:00 PM · Restricted Project
kuhar added a comment to D83087: DomTree: remove explicit use of DomTreeNodeBase::iterator.

modulo accidental formatting changes.

I'm not aware of any. Some line breaks changed because "const_iterator" is longer than "iterator".

Jul 8 2020, 8:40 AM · Restricted Project, Restricted Project, Restricted Project

Jul 6 2020

kuhar accepted D83087: DomTree: remove explicit use of DomTreeNodeBase::iterator.

LGTM modulo accidental formatting changes.

Jul 6 2020, 8:11 PM · Restricted Project, Restricted Project, Restricted Project
kuhar accepted D83090: DomTree: Add TreeNode type alias.

LGTM

Jul 6 2020, 7:55 PM · Restricted Project
kuhar requested changes to D83092: DomTree: Add climbLhsUntilSiblings helper.
Jul 6 2020, 7:52 PM · Restricted Project
kuhar added a comment to D83089: DomTree: Extract (mostly) read-only logic into type-erased base classes.

Overall, this seems like a good idea to me. The amount of templated code started growing out of hand some time ago, to the point where it's really hard to make logically changes, especially in the generic updater code.

Jul 6 2020, 7:43 PM · Restricted Project

Jul 4 2020

kuhar added a comment to D83160: [InstCombine] Lower infinite combine loop detection thresholds.

Overall, I think this is a good direction, but I'd like to understand better why InstCombine needs more than a few iterations and how 'fix' these cases without bailing out after a fixed number of iterations.
I'd be interested to find out if we can make an IR pattern generator that forces InstCombine to run N iterations. Are there any algorithmic guarantees of the current implementation which we could use to show that InstCombine doesn't go into an infinite loop?

Jul 4 2020, 10:46 AM · Restricted Project

Jul 2 2020

kuhar added a reviewer for D83088: Introduce CfgTraits abstraction: asbirlea.
Jul 2 2020, 7:25 PM · Restricted Project, Restricted Project, Restricted Project
kuhar added reviewers for D83089: DomTree: Extract (mostly) read-only logic into type-erased base classes: asbirlea, brzycki, kuhar.
Jul 2 2020, 7:25 PM · Restricted Project
kuhar added a reviewer for D83088: Introduce CfgTraits abstraction: brzycki.
Jul 2 2020, 7:25 PM · Restricted Project, Restricted Project, Restricted Project

Jun 11 2020

kuhar added a comment to D81089: Mark InstCombine as not preserving CFG.

If we define that changing successor order constitutes a CFG change, then these optimizations need to be dropped from InstCombine, and moved into SimplifyCFG.

As said before, InstCombine is supposed to be CFG-preserving -- if it isn't because the definition of what "CFG-preserving" means was unclear, then we need to fix InstCombine to be in line with we new definition.

As the pipeline diffs show, not preserving CFG has a very real cost (five new domtree calculations at least). We should try to avoid that :)

Jun 11 2020, 11:00 AM · Restricted Project
kuhar added a comment to D81089: Mark InstCombine as not preserving CFG.

Even if we agree the way the PostDominatorTree computes roots in cases involving infinite loops shouldn't depend on the ordering of successors, I'm not sure it's correct to say that instcombine "preserves the CFG". Even for the regular DominatorTree, I think reordering the successors invalidates DFS numbering (in the sense that it changes the result of getDFSNumIn()).

You can always preserve domtrees by marking dfsnumbers as invalid.

Jun 11 2020, 11:00 AM · Restricted Project

Jun 3 2020

kuhar added a comment to D81089: Mark InstCombine as not preserving CFG.

As a quick fix, maybe we could try to detect changes in reverse-unreachable code and update postdominators when that's detected? Not sure how much overhead that would be; InstCombine is already very expensive w.r.t. compilation times.

Jun 3 2020, 3:29 PM · Restricted Project

May 12 2020

kuhar accepted D78881: [FlattenCFG] Fix `MergeIfRegion` in case then-path is empty.

Rename Invert2 variable to InvertCond2.
Hope it makes more sense.

May 12 2020, 8:34 AM · Restricted Project

May 11 2020

kuhar accepted D78881: [FlattenCFG] Fix `MergeIfRegion` in case then-path is empty.

I followed the new tests and the patch seems fine to me, but I'd rather someone else took a look too.

May 11 2020, 6:56 AM · Restricted Project

Apr 18 2020

kuhar accepted D77967: [Dominators] Facilitate updates to MachinePostDominatorTree.

While this is fine, I'm I don't understand how splitting critical edges will interact with domtree updates when you call getBase in MachineDominators.h. But we can discuss that in another patch.

Apr 18 2020, 3:38 PM · Restricted Project

Apr 12 2020

kuhar added a comment to D77967: [Dominators] Facilitate updates to MachinePostDominatorTree.

Could you add some basic unit tests and a separate set of unit tests where critical edges get split?

Apr 12 2020, 12:16 PM · Restricted Project

Apr 8 2020

kuhar accepted D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff..

Looks good to me overall. I don't want to block it over the cosmetic issues like allocating the empty GD object.

Apr 8 2020, 7:57 PM · Restricted Project, Restricted Project

Apr 5 2020

kuhar added a comment to D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff..

Address comments.

Apr 5 2020, 7:45 PM · Restricted Project, Restricted Project
kuhar added inline comments to D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff..
Apr 5 2020, 7:45 PM · Restricted Project, Restricted Project

Apr 2 2020

kuhar added inline comments to D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff..
Apr 2 2020, 8:04 PM · Restricted Project, Restricted Project
kuhar added inline comments to D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff..
Apr 2 2020, 8:04 PM · Restricted Project, Restricted Project

Mar 31 2020

kuhar accepted D77167: [GraphDiff] Extend GraphDiff to track a list of updates..

LGTM

Mar 31 2020, 2:17 PM · Restricted Project

Mar 30 2020

kuhar committed rG77ce2e21a877: [AMDGPU] Add Relocation Constant Support (authored by kuhar).
[AMDGPU] Add Relocation Constant Support
Mar 30 2020, 10:51 AM
kuhar closed D76440: [AMDGPU] Add Relocation Constant Support.
Mar 30 2020, 10:51 AM · Restricted Project

Mar 25 2020

kuhar accepted D76813: Remove overly verbose debug from GenericDomTreeConstruction.
Mar 25 2020, 5:53 PM · Restricted Project

Mar 18 2020

kuhar committed rG1e4ee0bfc529: [Dominators] Fixup comments in GenericDominatorTreeConstruction. NFC. (authored by kuhar).
[Dominators] Fixup comments in GenericDominatorTreeConstruction. NFC.
Mar 18 2020, 11:25 AM
kuhar closed D76340: [Dominators] Fixup comments in GenericDominatorTreeConstruction. NFC..
Mar 18 2020, 11:25 AM · Restricted Project
kuhar added inline comments to D76340: [Dominators] Fixup comments in GenericDominatorTreeConstruction. NFC..
Mar 18 2020, 11:24 AM · Restricted Project

Mar 17 2020

kuhar created D76340: [Dominators] Fixup comments in GenericDominatorTreeConstruction. NFC..
Mar 17 2020, 7:59 PM · Restricted Project

Feb 29 2020

kuhar added inline comments to D74931: [Dominators] Use Instruction::comesBefore for block-local queries, NFC.
Feb 29 2020, 2:47 PM · Restricted Project

Feb 20 2020

kuhar added inline comments to D74931: [Dominators] Use Instruction::comesBefore for block-local queries, NFC.
Feb 20 2020, 7:46 PM · Restricted Project
kuhar added inline comments to D74931: [Dominators] Use Instruction::comesBefore for block-local queries, NFC.
Feb 20 2020, 3:25 PM · Restricted Project
kuhar added inline comments to D74931: [Dominators] Use Instruction::comesBefore for block-local queries, NFC.
Feb 20 2020, 2:39 PM · Restricted Project
kuhar accepted D74931: [Dominators] Use Instruction::comesBefore for block-local queries, NFC.

LGTM

Feb 20 2020, 2:30 PM · Restricted Project

Dec 20 2019

kuhar committed rGc431c407ebcb: [InstCombine] Improve infinite loop detection (authored by kuhar).
[InstCombine] Improve infinite loop detection
Dec 20 2019, 1:27 PM
kuhar closed D71673: [InstCombine] Improve infinite loop detection.
Dec 20 2019, 1:26 PM · Restricted Project

Dec 18 2019

kuhar created D71673: [InstCombine] Improve infinite loop detection.
Dec 18 2019, 1:03 PM · Restricted Project
kuhar committed rG3d29c41ad59e: [InstCombine] Insert instructions before adding them to worklist (authored by kuhar).
[InstCombine] Insert instructions before adding them to worklist
Dec 18 2019, 12:05 PM
kuhar closed D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 18 2019, 12:05 PM · Restricted Project
kuhar committed rG406b6019cd2b: [InstCombine] Allow to limit the max number of iterations (authored by kuhar).
[InstCombine] Allow to limit the max number of iterations
Dec 18 2019, 10:56 AM
kuhar closed D71145: [InstCombine] Allow to limit the max number of iterations.
Dec 18 2019, 10:56 AM · Restricted Project
kuhar updated the diff for D71145: [InstCombine] Allow to limit the max number of iterations.

Replace UINT_MAX with InstCombineDefaultMaxIterations

Dec 18 2019, 10:45 AM · Restricted Project
kuhar updated the diff for D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 18 2019, 10:45 AM · Restricted Project
kuhar updated the diff for D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 18 2019, 10:45 AM · Restricted Project
kuhar updated the diff for D71093: [InstCombine] Insert instructions before adding them to worklist.

Fix instruction insertion in InstCombineCasts for zext-or-icmp.

Dec 18 2019, 10:35 AM · Restricted Project
kuhar added inline comments to D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 18 2019, 10:35 AM · Restricted Project

Dec 17 2019

kuhar added a comment to D71145: [InstCombine] Allow to limit the max number of iterations.

I think it may be worthwhile to separate the limits for "stop after N iterations" and "assert after N iterations". In particular what I have in mind is that we can use -instcombine-max-iterations=2 in tests to ensure that correct worklist management allows everything to be folded in a single iteration (with the second establishing the fixed point). However, for this purpose -instcombine-max-iterations would actually need to assert. Possibly there's room for two separate options?

Dec 17 2019, 11:40 AM · Restricted Project
kuhar added inline comments to D71145: [InstCombine] Allow to limit the max number of iterations.
Dec 17 2019, 10:35 AM · Restricted Project
kuhar added a comment to D71145: [InstCombine] Allow to limit the max number of iterations.

@lebedev.ri @spatel @craig.topper
Do you have any other feedback or questions/concerns about the patch?

Dec 17 2019, 8:08 AM · Restricted Project
kuhar added inline comments to D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 17 2019, 7:58 AM · Restricted Project

Dec 13 2019

kuhar added inline comments to D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 13 2019, 10:42 AM · Restricted Project
kuhar added a comment to D71093: [InstCombine] Insert instructions before adding them to worklist.

I'm sorry, i'm not following the patch's logic anymore.
First it was about relying on Add() printing ADD: line, and now
all the printing stuff seems to be gone and the patch is adding some
invariants on when the instruction should be inserted into worklist?

Since the beginning, the patch was about ensuring that the instructions added to the worklist are properly inserted. One manifestation of instructions not being correctly inserted that was easy to observe was debug ADD lines with <badrefs> being printed.
One confusing thing that happened was that I followed a suggestion of reducing the amount of debug output printed and removing the line containing NEW that directly preceded ADD. But this change is no more and the amount of the debug output is the same.

Dec 13 2019, 10:33 AM · Restricted Project
kuhar added a comment to D71093: [InstCombine] Insert instructions before adding them to worklist.

Do you have a small IR example with before/after output that we can put here in the review? That way there's at least a record of what the improvement looks like.

Dec 13 2019, 9:38 AM · Restricted Project
kuhar updated the summary of D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 13 2019, 9:29 AM · Restricted Project
kuhar added a comment to D71093: [InstCombine] Insert instructions before adding them to worklist.

Do you have any other feedback? @spatel @lebedev.ri

Dec 13 2019, 8:15 AM · Restricted Project

Dec 11 2019

kuhar added a comment to D71145: [InstCombine] Allow to limit the max number of iterations.

In my experience extra iterations are often caused by bad worklist management when updating nodes sometimes. For example https://reviews.llvm.org/D50990 was supposed to fix one case of that, but I never got around to finding the original test where I observed the issue. If that patch still applies does it have any effect on any of the extra iterations in the table?

Dec 11 2019, 1:23 PM · Restricted Project

Dec 10 2019

kuhar added inline comments to D71145: [InstCombine] Allow to limit the max number of iterations.
Dec 10 2019, 2:03 PM · Restricted Project
kuhar added inline comments to D71145: [InstCombine] Allow to limit the max number of iterations.
Dec 10 2019, 9:41 AM · Restricted Project
kuhar added inline comments to D71145: [InstCombine] Allow to limit the max number of iterations.
Dec 10 2019, 8:01 AM · Restricted Project
kuhar updated the diff for D71145: [InstCombine] Allow to limit the max number of iterations.

Change the default iteration limit to UINT_MAX - 1.

Dec 10 2019, 7:56 AM · Restricted Project
kuhar added a comment to D71145: [InstCombine] Allow to limit the max number of iterations.

I ran some experiments to see how many iterations of InstCombine are necessary on real-world C++ applications. I paste the results below.
The numbers were collected by running InstCombine in a Full-LTO-like scenario: first I compiled all the code with -O3 with *optimizations disabled*, then I linked it together (using gllvm), and gave it to opt with -O3.

Dec 10 2019, 6:29 AM · Restricted Project

Dec 9 2019

kuhar added inline comments to D71145: [InstCombine] Allow to limit the max number of iterations.
Dec 9 2019, 10:43 AM · Restricted Project
kuhar updated the diff for D71145: [InstCombine] Allow to limit the max number of iterations.

Thanks for the suggestions.

Dec 9 2019, 9:56 AM · Restricted Project

Dec 6 2019

kuhar created D71145: [InstCombine] Allow to limit the max number of iterations.
Dec 6 2019, 12:43 PM · Restricted Project
kuhar updated the diff for D71093: [InstCombine] Insert instructions before adding them to worklist.

I added an assertion checking if the instructions added have a parent (block).
Also restored the previous debug message.

Dec 6 2019, 10:31 AM · Restricted Project
kuhar updated the diff for D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 6 2019, 8:20 AM · Restricted Project
kuhar added a comment to D71093: [InstCombine] Insert instructions before adding them to worklist.

This looks fine, I wonder whether it's worth testing [how easy it's to test, that is]

Dec 6 2019, 8:20 AM · Restricted Project

Dec 5 2019

kuhar created D71093: [InstCombine] Insert instructions before adding them to worklist.
Dec 5 2019, 2:54 PM · Restricted Project

Oct 8 2019

kuhar accepted D68460: [MachineSink] Don't preserve MachineLoopInfo.
Oct 8 2019, 7:38 AM · Restricted Project

Oct 4 2019

kuhar added a comment to D68460: [MachineSink] Don't preserve MachineLoopInfo.

Would it be possible to add a verifyAnalysis function to MLI and check if a freshly calculated one matches the 'preserved' one?

Oct 4 2019, 2:04 PM · Restricted Project