Page MenuHomePhabricator

nikic (Nikita Popov)
User

Projects

User does not belong to any projects.

User Details

User Since
May 5 2018, 9:37 AM (90 w, 3 d)

Recent Activity

Today

nikic accepted D73575: [InstCombine] canonicalize splat shuffle after cmp.

Does it make sense to extend this for arbitrary shuffles in the future? The mask construction logic in https://github.com/llvm/llvm-project/blob/4aa8cdfeebec115b928e0ccb452551b520d00f0b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp#L1612-L1660 can probably mostly be reused for the icmp case.

Tue, Jan 28, 1:11 PM · Restricted Project
nikic added a comment to D73549: [Analysis] add optional index parameter to isSplatValue() .

I don't fully understand how this API is going to be used. The current API makes sense to me (are all elements in the vector the same?) but the new one is unclear, especially for the case where no shufflevector is involved. If I have a splat constant, which index does the value come from?

Tue, Jan 28, 11:10 AM · Restricted Project
nikic added a comment to D73135: [AArch64][ARM] Always expand ordered vector reductions (PR44600).

Ping

Tue, Jan 28, 12:43 AM · Restricted Project

Yesterday

nikic added inline comments to D73410: [InstCombine] Push negation through multiply (PR44234).
Mon, Jan 27, 2:09 PM · Restricted Project
nikic updated the diff for D73410: [InstCombine] Push negation through multiply (PR44234).

Fix typo

Mon, Jan 27, 2:07 PM · Restricted Project
nikic added a comment to D73398: In SCEV's printer pass, print an estimate of the exit count for each exit from profiling information.

What you're doing here is right for a single exit, but I don't think it holds up for multiple exits. Talking in terms of probabilities, you are given P(E_i | not E_{i-1}), where E_i is the event that exit i is taken. The non-conditional exit probability is P(E_i) = P(E_i | not E_{i-1}) / P(not E_{i-1}). The predicted exit count is (1 / P(E_i)) - 1.

Mon, Jan 27, 1:48 PM · Restricted Project
nikic updated the diff for D73410: [InstCombine] Push negation through multiply (PR44234).

Rebase

Mon, Jan 27, 11:54 AM · Restricted Project
nikic committed rGbcfa0f592f8f: [InstCombine] Move negation handling into freelyNegateValue() (authored by nikic).
[InstCombine] Move negation handling into freelyNegateValue()
Mon, Jan 27, 11:47 AM
nikic closed D73288: [InstCombine] Move negation handling into freelyNegateValue().
Mon, Jan 27, 11:47 AM · Restricted Project
nikic committed rG0957748cb788: [InstCombine] Add more negation tests; NFC (authored by nikic).
[InstCombine] Add more negation tests; NFC
Mon, Jan 27, 11:46 AM
nikic added inline comments to D73288: [InstCombine] Move negation handling into freelyNegateValue().
Mon, Jan 27, 10:18 AM · Restricted Project
nikic updated the diff for D73288: [InstCombine] Move negation handling into freelyNegateValue().

Use switch instead of match chain.

Mon, Jan 27, 10:18 AM · Restricted Project

Sun, Jan 26

nikic added a comment to D72837: [AggressiveInstCombine] Add support for select instructions.

IMHO, there is no added value to splitting to 2 patches, the select changed are too obvious, and the compare is strongly related to the select changes.

Sun, Jan 26, 12:40 PM · Restricted Project
nikic added a comment to D72837: [AggressiveInstCombine] Add support for select instructions.

Is it possible to split this into a patch that adds just the select handling, and another that extends the icmp support? It seems like the select part should be testable independently, as long as you don't derive the select condition from the select operands.

Sun, Jan 26, 1:41 AM · Restricted Project

Sat, Jan 25

nikic added inline comments to D73411: [InstCombine] Process newly inserted instructions in the correct order.
Sat, Jan 25, 2:43 AM · Restricted Project
nikic created D73411: [InstCombine] Process newly inserted instructions in the correct order.
Sat, Jan 25, 2:34 AM · Restricted Project
nikic added a child revision for D73288: [InstCombine] Move negation handling into freelyNegateValue(): D73410: [InstCombine] Push negation through multiply (PR44234).
Sat, Jan 25, 2:08 AM · Restricted Project
nikic added a parent revision for D73410: [InstCombine] Push negation through multiply (PR44234): D73288: [InstCombine] Move negation handling into freelyNegateValue().
Sat, Jan 25, 2:08 AM · Restricted Project
nikic added inline comments to D73410: [InstCombine] Push negation through multiply (PR44234).
Sat, Jan 25, 2:08 AM · Restricted Project
nikic created D73410: [InstCombine] Push negation through multiply (PR44234).
Sat, Jan 25, 1:58 AM · Restricted Project
nikic added inline comments to D73398: In SCEV's printer pass, print an estimate of the exit count for each exit from profiling information.
Sat, Jan 25, 1:32 AM · Restricted Project

Fri, Jan 24

nikic updated the diff for D73288: [InstCombine] Move negation handling into freelyNegateValue().

Also move the ashr/lshr and trunc patterns into freelyNegateValue() and keep existing handling regarding one-use checks.

Fri, Jan 24, 1:50 PM · Restricted Project
nikic added a comment to D73342: Fix EarlyCSE to intersect aliasing metadata..

Hal Finkel provided following example on the mailing list:

int * restrict r = a;
...
int x = noaliasing ? *r : *a;

This shows that the intersection of '*r' and '*a' must be taken.

Fri, Jan 24, 8:59 AM · Restricted Project
nikic added a comment to D73342: Fix EarlyCSE to intersect aliasing metadata..

Can you explain in more detail why the current behavior is incorrect? Why is using the aliasing information in program order wrong? If the first load is noalias, then we should be able to deduplicate to that noalias load. If the second load is noalias, then generally we can't assume the first load is also noalias. Unless there is a must-exec relation between them, as @nlopes mentioned.

Fri, Jan 24, 8:23 AM · Restricted Project

Thu, Jan 23

nikic resigned from D60846: [ValueTracking] Improve isKnowNonZero for Ints.
Thu, Jan 23, 1:06 PM · Restricted Project
nikic created D73288: [InstCombine] Move negation handling into freelyNegateValue().
Thu, Jan 23, 12:38 PM · Restricted Project

Wed, Jan 22

nikic committed rG0b83c5a78fae: [InstCombine] Combine neg of shl of sub (PR44529) (authored by nikic).
[InstCombine] Combine neg of shl of sub (PR44529)
Wed, Jan 22, 2:05 PM
nikic closed D72978: [InstCombine] Combine neg of shl of sub (PR44529).
Wed, Jan 22, 2:04 PM · Restricted Project
nikic committed rG80c34f94acdb: [InstCombine] Add test for PR44529; NFC (authored by nikic).
[InstCombine] Add test for PR44529; NFC
Wed, Jan 22, 2:04 PM
nikic committed rGefba7ed05e50: [PatternMatch] Make m_c_ICmp swap the predicate (PR42801) (authored by nikic).
[PatternMatch] Make m_c_ICmp swap the predicate (PR42801)
Wed, Jan 22, 1:57 PM
nikic closed D72976: [PatternMatch] Make m_c_ICmp swap the predicate (PR42801).
Wed, Jan 22, 1:57 PM · Restricted Project
nikic committed rGed80c86c8854: [PatternMatch] Add m_APInt/m_APFloat matchers accepting undef (authored by nikic).
[PatternMatch] Add m_APInt/m_APFloat matchers accepting undef
Wed, Jan 22, 1:55 PM
nikic closed D72975: [PatternMatch] Add m_APInt/m_APFloat matchers accepting undef.
Wed, Jan 22, 1:55 PM · Restricted Project
nikic added a comment to D72169: [CVP] Simplify cmp of local phi node.

Ping

Wed, Jan 22, 12:06 AM · Restricted Project

Tue, Jan 21

nikic created D73135: [AArch64][ARM] Always expand ordered vector reductions (PR44600).
Tue, Jan 21, 1:00 PM · Restricted Project

Mon, Jan 20

nikic added a comment to D72944: [InstCombine] Fix worklist management when simplifying demanded bits (PR44541).

To fix the infinite loop bug without this change, we could add a clause in canonicalizeMinMaxWithConstant() to avoid creating this:
%cmp = icmp sgt i16 0, %sub

Ie, if the LHS returned by matchSelectPattern() is a constant, swap LHS and RHS because we know we can't create a canonical compare that way. An alternative would be to do that swap within matchSelectPattern() itself. We already have a different LHS/RHS constraint for ABS/NABS on that analysis.

I'm not sure how we can expose that bug with this fix in place though, so might want to make that fix first to be safer?

Mon, Jan 20, 5:30 AM · Restricted Project
nikic accepted D72958: [InstSimplify] fold select of vector constants that include undef elements.

LGTM

Mon, Jan 20, 4:07 AM · Restricted Project

Sun, Jan 19

nikic added inline comments to D72958: [InstSimplify] fold select of vector constants that include undef elements.
Sun, Jan 19, 11:44 AM · Restricted Project

Sat, Jan 18

nikic added inline comments to D72958: [InstSimplify] fold select of vector constants that include undef elements.
Sat, Jan 18, 9:02 AM · Restricted Project
nikic created D72978: [InstCombine] Combine neg of shl of sub (PR44529).
Sat, Jan 18, 8:07 AM · Restricted Project
nikic created D72976: [PatternMatch] Make m_c_ICmp swap the predicate (PR42801).
Sat, Jan 18, 4:14 AM · Restricted Project
nikic created D72975: [PatternMatch] Add m_APInt/m_APFloat matchers accepting undef.
Sat, Jan 18, 3:30 AM · Restricted Project

Fri, Jan 17

nikic added inline comments to D72643: [InstCombine] form copysign from select of FP constants (PR44153).
Fri, Jan 17, 2:24 PM · Restricted Project
nikic added inline comments to D72944: [InstCombine] Fix worklist management when simplifying demanded bits (PR44541).
Fri, Jan 17, 12:09 PM · Restricted Project
nikic created D72944: [InstCombine] Fix worklist management when simplifying demanded bits (PR44541).
Fri, Jan 17, 12:09 PM · Restricted Project
nikic committed rG522c030aa9b1: [InstCombine] Fix worklist management in DSE (PR44552) (authored by nikic).
[InstCombine] Fix worklist management in DSE (PR44552)
Fri, Jan 17, 9:16 AM
nikic closed D72807: [InstCombine] Fix worklist management in DSE (PR44552).
Fri, Jan 17, 9:15 AM · Restricted Project
nikic committed rG77befe54f7d7: [InstCombine] Fix worklist management in return combine (authored by nikic).
[InstCombine] Fix worklist management in return combine
Fri, Jan 17, 9:05 AM
nikic closed D72864: [InstCombine] Fix worklist management in return combine.
Fri, Jan 17, 9:05 AM · Restricted Project
nikic committed rG10d0e2882bbe: [InstCombine] Split assume test in expensive and not; NFC (authored by nikic).
[InstCombine] Split assume test in expensive and not; NFC
Fri, Jan 17, 9:05 AM
nikic committed rG2ca092f32095: [InstCombine] Support disabling expensive combines in opt (authored by nikic).
[InstCombine] Support disabling expensive combines in opt
Fri, Jan 17, 8:57 AM
nikic closed D72861: [InstCombine] Support disabling expensive combines in opt.
Fri, Jan 17, 8:57 AM · Restricted Project
nikic committed rG2d0d4235a282: [InstCombine] Add test for -expensive-combines option; NFC (authored by nikic).
[InstCombine] Add test for -expensive-combines option; NFC
Fri, Jan 17, 8:57 AM

Thu, Jan 16

nikic added a comment to D72861: [InstCombine] Support disabling expensive combines in opt.

@spatel I've reported https://bugs.llvm.org/show_bug.cgi?id=44573 and https://bugs.llvm.org/show_bug.cgi?id=44574 to keep track of this.

Thu, Jan 16, 1:16 PM · Restricted Project
nikic created D72864: [InstCombine] Fix worklist management in return combine.
Thu, Jan 16, 12:47 PM · Restricted Project
nikic created D72861: [InstCombine] Support disabling expensive combines in opt.
Thu, Jan 16, 12:08 PM · Restricted Project
nikic accepted D72784: [IR] fix crash in Constant::isElementWiseEqual() with FP types.

LGTM

Thu, Jan 16, 11:39 AM · Restricted Project

Wed, Jan 15

nikic updated the diff for D72807: [InstCombine] Fix worklist management in DSE (PR44552).

Remove unnecessary iterator increment. Reduce test case to use 100 instead of 1000 iterations.

Wed, Jan 15, 1:56 PM · Restricted Project
nikic added inline comments to D72784: [IR] fix crash in Constant::isElementWiseEqual() with FP types.
Wed, Jan 15, 1:47 PM · Restricted Project
nikic added a comment to D72807: [InstCombine] Fix worklist management in DSE (PR44552).

I've added the complete original test case with -instcombine-infinite-loop-threshold=2. It would also be possible to reduce it (it just wouldn't need the full 1000 iterations that trigger the default threshold), but as the test now runs quickly even on a debug build, I figured keeping the original is fine.

Wed, Jan 15, 1:38 PM · Restricted Project
nikic created D72807: [InstCombine] Fix worklist management in DSE (PR44552).
Wed, Jan 15, 1:29 PM · Restricted Project

Tue, Jan 14

nikic committed rG04e586151e79: [InstCombine] Fix worklist management when removing guard intrinsic (authored by nikic).
[InstCombine] Fix worklist management when removing guard intrinsic
Tue, Jan 14, 12:50 PM
nikic closed D72558: [InstCombine] Fix worklist management when removing guard intrinsic.
Tue, Jan 14, 12:50 PM · Restricted Project
nikic committed rG410331869def: [NewPM] Port MergeFunctions pass (authored by nikic).
[NewPM] Port MergeFunctions pass
Tue, Jan 14, 12:03 PM
nikic closed D72537: [NewPM] Port MergeFunctions pass.
Tue, Jan 14, 12:02 PM · Restricted Project
nikic committed rG65c0805be523: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms (authored by nikic).
[InstCombine] Fix infinite loop due to bitcast <-> phi transforms
Tue, Jan 14, 11:53 AM
nikic committed rGb4dd928ffbb8: [InstCombine] Make combineLoadToNewType a method; NFC (authored by nikic).
[InstCombine] Make combineLoadToNewType a method; NFC
Tue, Jan 14, 11:53 AM
nikic closed D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.
Tue, Jan 14, 11:53 AM · Restricted Project
nikic committed rG652cd7c1007a: [InstCombine] Fix user iterator invalidation in bitcast of phi transform (authored by nikic).
[InstCombine] Fix user iterator invalidation in bitcast of phi transform
Tue, Jan 14, 11:43 AM
nikic committed rGfa632340938c: [InstCombine] Add test for iterator invalidation bug; NFC (authored by nikic).
[InstCombine] Add test for iterator invalidation bug; NFC
Tue, Jan 14, 11:43 AM
nikic closed D72657: [InstCombine] Fix user iterator invalidation in bitcast of phi transform.
Tue, Jan 14, 11:43 AM · Restricted Project
nikic added inline comments to D72537: [NewPM] Port MergeFunctions pass.
Tue, Jan 14, 11:32 AM · Restricted Project
nikic planned changes to D72519: [LoopInfo] Support multi edge in getLoopLatch().

https://github.com/llvm/llvm-project/blob/ab72db7fc85266f094cc6b9452dd01f175c04cab/llvm/lib/Analysis/ScalarEvolutionExpander.cpp#L1300-L1325 => This piece of code in SCEVExpander breaks in practice (creating separate values for the same predecessor block).

Tue, Jan 14, 11:23 AM · Restricted Project
nikic requested review of D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

I've now verified that this passes test-suite when applied together with D72657.

Tue, Jan 14, 1:04 AM · Restricted Project
nikic added a child revision for D72657: [InstCombine] Fix user iterator invalidation in bitcast of phi transform: D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.
Tue, Jan 14, 12:55 AM · Restricted Project
nikic added a parent revision for D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms: D72657: [InstCombine] Fix user iterator invalidation in bitcast of phi transform.
Tue, Jan 14, 12:55 AM · Restricted Project

Mon, Jan 13

nikic created D72657: [InstCombine] Fix user iterator invalidation in bitcast of phi transform.
Mon, Jan 13, 2:58 PM · Restricted Project
nikic added a comment to D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

I was able to create a reproducer with uselistorder:

Mon, Jan 13, 2:39 PM · Restricted Project
nikic added a comment to D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

Okay, I think I got it. I don't have a standalone reproducer for this, but what seems to be happening is that we're iterating over OldPN->users() in https://github.com/llvm/llvm-project/blob/a506f7f9105eec4baac296d21c922457d6f4b52a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L2347, but https://github.com/llvm/llvm-project/blob/a506f7f9105eec4baac296d21c922457d6f4b52a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L2353 is going to modify the users. That makes us skip over a user (specifically, the original bitcast), which then refers to the old phi node.

Mon, Jan 13, 2:22 PM · Restricted Project
nikic added a comment to D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

I'm a bit stumped here. From the trace I see that previously instcombine produced

Mon, Jan 13, 1:15 PM · Restricted Project
nikic planned changes to D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

Did you verify that test-suite now passes?

Mon, Jan 13, 11:50 AM · Restricted Project
nikic added inline comments to D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.
Mon, Jan 13, 10:07 AM · Restricted Project
nikic updated the diff for D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

Add comment, split out NFC change.

Mon, Jan 13, 10:07 AM · Restricted Project
nikic added inline comments to rGef5debac4302: [SelectionDAG] ComputeKnownBits add getValidMinimumShiftAmountConstant() ISD….
Mon, Jan 13, 7:53 AM
nikic added inline comments to rGef5debac4302: [SelectionDAG] ComputeKnownBits add getValidMinimumShiftAmountConstant() ISD….
Mon, Jan 13, 6:54 AM
nikic added a comment to D72423: [DemandedBits] Improve accuracy of Add propagator.

@rrika Performing these kinds of tests on IR is both quite inefficient and unnecessary complex. It would be better to extract the code logic (which just operates on APInts) into a separate file, which will allow you to easily test it, and also share the logic with other places doing demanded bits analysis, such as InstCombine and SelectionDAG.

Are you suggesting factoring out the logic for addition/subtraction only or all opcodes? Where should this new function live?

Mon, Jan 13, 5:29 AM · Restricted Project
nikic added a comment to D72169: [CVP] Simplify cmp of local phi node.

Ping

Mon, Jan 13, 12:52 AM · Restricted Project
nikic updated the summary of D72169: [CVP] Simplify cmp of local phi node.
Mon, Jan 13, 12:52 AM · Restricted Project

Sun, Jan 12

nikic added a comment to D72423: [DemandedBits] Improve accuracy of Add propagator.

@rrika Performing these kinds of tests on IR is both quite inefficient and unnecessary complex. It would be better to extract the code logic (which just operates on APInts) into a separate file, which will allow you to easily test it, and also share the logic with other places doing demanded bits analysis, such as InstCombine and SelectionDAG.

Sun, Jan 12, 5:43 AM · Restricted Project

Sat, Jan 11

nikic added a comment to D72519: [LoopInfo] Support multi edge in getLoopLatch().

I'm open to being convinced that the change in canonical form to allow multiple identical backedges is reasonable, but I'd expect to see an argument to that form, not simple a code change.

Sat, Jan 11, 1:11 PM · Restricted Project
nikic created D72558: [InstCombine] Fix worklist management when removing guard intrinsic.
Sat, Jan 11, 9:42 AM · Restricted Project
nikic added a comment to D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

I've updated the patch to directly perform the load combine now. I believe the store case shouldn't exhibit the same issue, as the bitcast is cannot be a phi operand there. I've also added the new looping testcase and -instcombine-infinite-loop-threshold=2 to make sure that the combine runs through in one iteration.

Sat, Jan 11, 5:34 AM · Restricted Project
nikic updated the diff for D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

Explicitly perform load combine.

Sat, Jan 11, 5:34 AM · Restricted Project
nikic added a comment to D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

As far as I can see this is basically the same issue as what this patch tries to solve. Just this time it's not the extra use in the old phi causing the loop, but a change in worklist order caused by this change.

Sat, Jan 11, 4:18 AM · Restricted Project
nikic added a comment to D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

Mostly reduced testcase triggering an infinite loop with this patch:

Sat, Jan 11, 3:58 AM · Restricted Project
nikic committed rG142ba7d76af4: [LoopRotate] Add tests for rotate with switch; NFC (authored by nikic).
[LoopRotate] Add tests for rotate with switch; NFC
Sat, Jan 11, 2:06 AM
nikic committed rGad36d29eaed6: [LoopSimplify] Regenerate test checks; NFC (authored by nikic).
[LoopSimplify] Regenerate test checks; NFC
Sat, Jan 11, 2:06 AM
nikic committed rG0e322c8a1f20: [InstCombine] Preserve nuw on sub of geps (PR44419) (authored by nikic).
[InstCombine] Preserve nuw on sub of geps (PR44419)
Sat, Jan 11, 2:05 AM
nikic closed D72048: [InstCombine] Preserve nuw on sub of geps (PR44419).
Sat, Jan 11, 2:05 AM · Restricted Project
nikic accepted D72436: [SCEV] get a more accurate range for AddRecExpr with nsw flag.

LGTM

Sat, Jan 11, 1:46 AM · Restricted Project