Page MenuHomePhabricator

nikic (Nikita Popov)
User

Projects

User does not belong to any projects.

User Details

User Since
May 5 2018, 9:37 AM (134 w, 4 d)

Recent Activity

Yesterday

nikic added a comment to D92486: Set option default for enabling memory ssa for new pass manager loop sink pass to true..

Compile-time: https://llvm-compile-time-tracker.com/compare.php?from=54eab293f523956bdc4b1a98b6cf5abc0bd1ef3f&to=b023c8adc9eb545ec5b8aa1a8d6f77a46d622006&stat=instructions

The problem here is that LoopSink is only relevant for PGO, but MemorySSA gets computed unconditionally in the legacy PM. This problem does not affect the new pass manager, where it's possible to compute MemorySSA only if useful. Supporting this properly in LegacyPM would require something like LazyMemorySSA.

I think in this instance it might be best to do this change only for NewPM. It's best to keep both pass managers in sync, but here there is a technical reason for different treatment.

Perhaps it would also be possible to directly construct MemorySSA in the pass, if PGO is enabled with the legacy PM?

Wed, Dec 2, 9:51 AM · Restricted Project
nikic added a comment to D92401: [BasicAA] Handle two unknown sizes for GEPs.

This kind of logic should always be valid, regardless if V1 is a GEP or not, right? Is there a way to do this check early or late for any query?

The general idea is valid, but the way it is applied depends on the operation. For GEPs we just strip to the base pointer. For phis and selects, we recurse over the phi/select operands (with special casing for the case of identical control dependence).

Hm, I'm confused(, but also not deep enough in the BasicAA code to argue). I assumed that this new logic which is put in a function that has a precondition (isa<GEP>(V1)) could be placed in a function without that precondition. The logic does not utilize the fact that V1 isa GEP. Anyway, it was just a thought.

Wed, Dec 2, 9:49 AM · Restricted Project
nikic added a comment to D92486: Set option default for enabling memory ssa for new pass manager loop sink pass to true..

The problem here is that LoopSink is only relevant for PGO, but MemorySSA gets computed unconditionally in the legacy PM. This problem does not affect the new pass manager, where it's possible to compute MemorySSA only if useful. Supporting this properly in LegacyPM would require something like LazyMemorySSA.

Wed, Dec 2, 9:26 AM · Restricted Project

Tue, Dec 1

nikic added a comment to D92072: [CMake][NewPM] Move ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER into llvm/.

It looks like this change has also enabled the new pass manager by default.

Tue, Dec 1, 1:03 PM · Restricted Project, Restricted Project
nikic added a comment to D92401: [BasicAA] Handle two unknown sizes for GEPs.

This kind of logic should always be valid, regardless if V1 is a GEP or not, right? Is there a way to do this check early or late for any query?

Tue, Dec 1, 12:58 PM · Restricted Project
nikic added a comment to D92402: [InstSimplify] Allow isUndefValue(V) to return true if V is poison.

I don't think this change makes a lot of sense. I expect that in nearly all cases we'll be able to produce a better result by having a separate PoisonValue check beforehand (we'll want to return poison rather than undef or some other value). And if we do that, this change to the isUndefValue() API just makes things more confusing.

Tue, Dec 1, 11:24 AM · Restricted Project
nikic accepted D92411: [Intrinsics] Re-remove experimental_vector_reduce intrinsics.

LGTM

Tue, Dec 1, 11:20 AM · Restricted Project
nikic requested review of D92401: [BasicAA] Handle two unknown sizes for GEPs.
Tue, Dec 1, 9:43 AM · Restricted Project
nikic committed rG54eab293f523: [BasicAA] Add test for suboptimal result with unknown sizes (NFC) (authored by nikic).
[BasicAA] Add test for suboptimal result with unknown sizes (NFC)
Tue, Dec 1, 9:21 AM
nikic committed rG624af932a808: [MemCpyOpt] Port to MemorySSA (authored by nikic).
[MemCpyOpt] Port to MemorySSA
Tue, Dec 1, 8:58 AM
nikic closed D89207: [MemCpyOpt] Port to MemorySSA.
Tue, Dec 1, 8:57 AM · Restricted Project
nikic added a reviewer for D92367: [SCEV] Fix incorrect exit count calculated in error scope: mkazantsev.
Tue, Dec 1, 12:46 AM · Restricted Project

Mon, Nov 30

nikic added inline comments to D92045: [DSE] Consider out-of-bound writes in isOverwrite..
Mon, Nov 30, 1:50 PM · Restricted Project
nikic committed rGb5f23189fb05: [DL] Inline getAlignmentInfo() implementation (NFC) (authored by nikic).
[DL] Inline getAlignmentInfo() implementation (NFC)
Mon, Nov 30, 11:56 AM
nikic added inline comments to D86844: [LoopDeletion] Allows deletion of possibly infinite side-effect free loops.
Mon, Nov 30, 1:01 AM · Restricted Project, Restricted Project

Sun, Nov 29

nikic added a comment to D89207: [MemCpyOpt] Port to MemorySSA.

@asbirlea Is this ready to land in the current form?

Sun, Nov 29, 2:43 PM · Restricted Project
nikic committed rG891170e8636b: [DL] Optimize address space zero lookup (NFC) (authored by nikic).
[DL] Optimize address space zero lookup (NFC)
Sun, Nov 29, 1:52 PM
nikic accepted D92207: [IndVars] ICmpInst should not prevent IV widening.

LGTM

Sun, Nov 29, 4:58 AM · Restricted Project
nikic accepted D92270: [ConstantFold] Fold more operations to poison.

LGTM

Sun, Nov 29, 2:08 AM · Restricted Project, Restricted Project
nikic planned changes to D91936: [BasicAA] Fix BatchAA results for phi-phi assumptions.

This will clearly need something more sophisticated. After thinking this over, I also think that the current approach of inserting assumptions at phis is not optimal, as the tests added in https://github.com/llvm/llvm-project/commit/b5e8de9c7903d458b098a8af03939384270c1a5e show. I expect this can also result in BatchAA returning different results, depending on whether a gep or a phi is visited first. We should be inserting everything as NoAlias into the cache in the first place and tracking whether that assumption gets used or not.

Sun, Nov 29, 1:38 AM · Restricted Project
nikic committed rGe987fbdd85d6: [BasicAA] Generalize recursive phi alias analysis (authored by nikic).
[BasicAA] Generalize recursive phi alias analysis
Sun, Nov 29, 1:25 AM
nikic closed D91914: [BasicAA] Generalize recursive phi alias analysis.
Sun, Nov 29, 1:25 AM · Restricted Project

Sat, Nov 28

nikic committed rGb5e8de9c7903: [BasicAA] Add tests for suboptimal speculation results (NFC) (authored by nikic).
[BasicAA] Add tests for suboptimal speculation results (NFC)
Sat, Nov 28, 10:17 AM
nikic added inline comments to D92203: [ConstantFold] Fold operations to poison if possible.
Sat, Nov 28, 9:17 AM · Restricted Project
nikic accepted D92203: [ConstantFold] Fold operations to poison if possible.

There seem to be still quite a few places where UndefValue can be replaced with PoisonValue in ConstantFold, do you plan to follow up on that?

Sat, Nov 28, 9:10 AM · Restricted Project
nikic added inline comments to D92203: [ConstantFold] Fold operations to poison if possible.
Sat, Nov 28, 1:42 AM · Restricted Project
nikic committed rG1dea8ed8b7dd: [BasicAA] Remove unnecessary known size requirement (authored by nikic).
[BasicAA] Remove unnecessary known size requirement
Sat, Nov 28, 1:18 AM
nikic closed D91482: [BasicAA] Remove unnecessary known size requirement.
Sat, Nov 28, 1:17 AM · Restricted Project

Fri, Nov 27

nikic reopened D91936: [BasicAA] Fix BatchAA results for phi-phi assumptions.

@mstorsjo Thanks for the report & revert.

Fri, Nov 27, 1:35 PM · Restricted Project
nikic committed rG8351f9b5ce7e: [ValueTracking] Fix assert on shufflevector of pointers (authored by nikic).
[ValueTracking] Fix assert on shufflevector of pointers
Fri, Nov 27, 12:20 PM

Thu, Nov 26

nikic updated the diff for D90094: [BasicAA] Handle recursive queries more efficiently (NFCI).

Rebase

Thu, Nov 26, 12:56 PM · Restricted Project
nikic committed rG8166ed1a7a26: [BasicAA] Fix BatchAA results for phi-phi assumptions (authored by nikic).
[BasicAA] Fix BatchAA results for phi-phi assumptions
Thu, Nov 26, 12:44 PM
nikic closed D91936: [BasicAA] Fix BatchAA results for phi-phi assumptions.
Thu, Nov 26, 12:44 PM · Restricted Project
nikic accepted D92162: [LangRef] Add poison constant.

LGTM

Thu, Nov 26, 11:35 AM · Restricted Project
nikic committed rG4df8efce80e3: [AA] Split up LocationSize::unknown() (authored by nikic).
[AA] Split up LocationSize::unknown()
Thu, Nov 26, 9:40 AM
nikic closed D91649: [AA] Split up LocationSize::unknown().
Thu, Nov 26, 9:40 AM · Restricted Project
nikic added a comment to D92152: [SCEV] Use isKnownPredicateAt in isLoopBackedgeGuardedByCond.

Looks like this change has a large negative compile-time impact: https://llvm-compile-time-tracker.com/compare.php?from=2254e014a9019bf17c3f5cb27c1dc40ca0f2ffce&to=14f2ad0e3cc54d5eb254b545a469e8ffdb62b119&stat=instructions 2% on average, 6% on mafft.

Thu, Nov 26, 1:28 AM · Restricted Project

Tue, Nov 24

nikic added inline comments to D92045: [DSE] Consider out-of-bound writes in isOverwrite..
Tue, Nov 24, 12:57 PM · Restricted Project
nikic added inline comments to D91711: SCEV add function to see if SCEVUnknown is null.
Tue, Nov 24, 11:26 AM · Restricted Project
nikic added inline comments to D92045: [DSE] Consider out-of-bound writes in isOverwrite..
Tue, Nov 24, 11:24 AM · Restricted Project
nikic added inline comments to D92045: [DSE] Consider out-of-bound writes in isOverwrite..
Tue, Nov 24, 11:19 AM · Restricted Project
nikic added inline comments to D91649: [AA] Split up LocationSize::unknown().
Tue, Nov 24, 12:31 AM · Restricted Project

Mon, Nov 23

nikic added a comment to D91576: [MemCpyOpt] Correctly merge alias scopes during call slot optimization.

Unless there's something that makes this issue specific to the call slot optimization, wouldn't it be better to adjust the logic in MDNode::getMostGenericAliasScope() to ensure that any code performing alias scope merging is covered?

Mon, Nov 23, 4:12 AM · Restricted Project
nikic added a comment to D90341: Use deref facts derived from minimum object size of allocations.

The logic here looks sound to me. Structurally, I would suggest integrating this with the getPointerDereferenceableBytes() check above, as you are currently duplicating the logic around it. Basically extract the current getPointerDereferenceableBytes() call into a static function/lambda, and add the handling for the allocated object size in there, the remaining logic (including the non-null check and the alignment check) will be shared.

Mon, Nov 23, 3:23 AM · Restricted Project
nikic accepted D91942: [SCEV] Fix incorrect treatment of max taken count. PR48225.

LGTM

Mon, Nov 23, 1:15 AM · Restricted Project

Sun, Nov 22

nikic updated the diff for D91936: [BasicAA] Fix BatchAA results for phi-phi assumptions.

Rebase. I've committed the not-really-related change that lead to the compile-time improvement. With that gone, the new numbers are: http://localhost:8000/compare.php?from=6f5ef648a57aed3274118dcbd6467e8ac4f6ed1f&to=50d73a31620111c4298cc336ac4682ef7585ed6b&stat=instructions Which does make this a mild compile-time regression, but not too bad.

Sun, Nov 22, 11:34 AM · Restricted Project
nikic committed rG6f5ef648a57a: [BasicAA] Avoid unnecessary cache update (NFC) (authored by nikic).
[BasicAA] Avoid unnecessary cache update (NFC)
Sun, Nov 22, 11:11 AM
nikic requested review of D91936: [BasicAA] Fix BatchAA results for phi-phi assumptions.
Sun, Nov 22, 10:58 AM · Restricted Project
nikic committed rG221c2b8862b1: [BasicAA] Add more phi-phi tests (NFC) (authored by nikic).
[BasicAA] Add more phi-phi tests (NFC)
Sun, Nov 22, 7:53 AM

Sat, Nov 21

nikic committed rGded592886625: [BasicAA] Remove unnecessary sextOrSelf (NFC) (authored by nikic).
[BasicAA] Remove unnecessary sextOrSelf (NFC)
Sat, Nov 21, 12:33 PM
nikic committed rG0d114f56d709: [BasicAA] Return DecomposedGEP (NFC) (authored by nikic).
[BasicAA] Return DecomposedGEP (NFC)
Sat, Nov 21, 12:07 PM
nikic added a comment to D90648: [SCEV] Fix nsw flags for GEP expressions.

This patch does not have a new test case added to demonstrate what this patch is trying to fix or reference the bug number showing the bug end-to-end.

Sat, Nov 21, 11:58 AM · Restricted Project
nikic committed rGf4412c5ae4ee: [BasicAA] Remove some intermediate variables (NFC) (authored by nikic).
[BasicAA] Remove some intermediate variables (NFC)
Sat, Nov 21, 11:36 AM
nikic committed rG913a99c47439: [BasicAA] Remove stale FIXME (NFC) (authored by nikic).
[BasicAA] Remove stale FIXME (NFC)
Sat, Nov 21, 11:07 AM
nikic requested review of D91914: [BasicAA] Generalize recursive phi alias analysis.
Sat, Nov 21, 9:04 AM · Restricted Project
nikic committed rG072ddff3f207: [BasicAA] Add recphi test with dynamic offset (NFC) (authored by nikic).
[BasicAA] Add recphi test with dynamic offset (NFC)
Sat, Nov 21, 8:52 AM
nikic accepted D91864: [NFC] Reduce code duplication in binop processing in computeExitLimitFromCondCached.

LGTM

Sat, Nov 21, 8:51 AM · Restricted Project

Fri, Nov 20

nikic added a comment to D89207: [MemCpyOpt] Port to MemorySSA.

Since this is using MSSA walker API with a MemoryLocation (which does not cache), could this become costly in a pathologically constructed example? If so, could we have a (reasonably large) upper bound on the number of getClobbering (Access, Location) calls to avoid such cases?

Fri, Nov 20, 1:57 PM · Restricted Project
nikic updated the diff for D89207: [MemCpyOpt] Port to MemorySSA.

Rebase.

Fri, Nov 20, 1:49 PM · Restricted Project
nikic updated the diff for D91482: [BasicAA] Remove unnecessary known size requirement.

Rebase over D91649, after which this correctness of this change should be more obvious. At this point in BasicAA, all LocationSizes only allow access after the base pointer.

Fri, Nov 20, 9:58 AM · Restricted Project
nikic updated the diff for D91649: [AA] Split up LocationSize::unknown().

Accept AATags in MemoryLocation::getAfter/getBeforeOrAfter helpers, which makes them usable in more cases.

Fri, Nov 20, 9:50 AM · Restricted Project

Thu, Nov 19

nikic updated the diff for D91649: [AA] Split up LocationSize::unknown().

Rename the MemoryLocation helpers as well. With the new LocationSize naming, it makes sense to call these MemoryLocation::getAfter() and MemoryLocation::getBeforeOrAfter().

Thu, Nov 19, 2:58 PM · Restricted Project
nikic updated the diff for D91649: [AA] Split up LocationSize::unknown().

Rename to LocationSize::afterPointer() and LocationSize::beforeOrAfterPointer().

Thu, Nov 19, 2:13 PM · Restricted Project
nikic added inline comments to D91649: [AA] Split up LocationSize::unknown().
Thu, Nov 19, 1:53 PM · Restricted Project
nikic updated the diff for D91649: [AA] Split up LocationSize::unknown().

Rebase, address review.

Thu, Nov 19, 1:48 PM · Restricted Project
nikic committed rGe8dc6e9a3242: [MemLoc] Use hasValue() method more (NFC) (authored by nikic).
[MemLoc] Use hasValue() method more (NFC)
Thu, Nov 19, 1:30 PM
nikic committed rG7de7c40898a8: [MemLoc] Use hasValue() method (NFC) (authored by nikic).
[MemLoc] Use hasValue() method (NFC)
Thu, Nov 19, 12:55 PM
nikic committed rGd8eb99810dc3: [MemLoc] Specify LocationSize in unit test (authored by nikic).
[MemLoc] Specify LocationSize in unit test
Thu, Nov 19, 12:53 PM
nikic committed rG393b9e9db31a: [MemLoc] Require LocationSize argument (NFC) (authored by nikic).
[MemLoc] Require LocationSize argument (NFC)
Thu, Nov 19, 12:46 PM
nikic committed rG22ec72f803d6: [Lint] Use MemoryLocation (authored by nikic).
[Lint] Use MemoryLocation
Thu, Nov 19, 11:55 AM
nikic committed rGeb995e933216: [Polly] Use LocationSize::unknown() (NFC) (authored by nikic).
[Polly] Use LocationSize::unknown() (NFC)
Thu, Nov 19, 11:27 AM
nikic added a comment to D72420: [LoopRotate] Add support for rotating loops with switch exit.

I'm not sure. It might make more sense to make SimplifyCFG be less aggressive about forming switches in this case. Pretty sure that converting if (a == C || B == C2) into a switch is pretty much a strict regression in optimization power, because a lot more passes support branches than switches.

Thu, Nov 19, 9:29 AM · Restricted Project

Wed, Nov 18

nikic added a comment to D90249: Add options to enable memoryssa for loopsink, expand loopsink testing and fix exposed bug in LICM.

Looking over the patch, the compile-time regression is likely not related to MemorySSA, but the fact that you moved the AST construction before the !Preheader->getParent()->hasProfileData() check, and the profitability heuristic. This means that expensive AST construction now happens every time, instead of only when useful.

Wed, Nov 18, 12:53 PM · Restricted Project
nikic committed rGcd3c22c47e4b: [BasicAA] Generalize base offset modulus handling (authored by nikic).
[BasicAA] Generalize base offset modulus handling
Wed, Nov 18, 12:49 PM
nikic closed D91027: [BasicAA] Generalize base offset modulus handling.
Wed, Nov 18, 12:49 PM · Restricted Project
nikic added a comment to D90249: Add options to enable memoryssa for loopsink, expand loopsink testing and fix exposed bug in LICM.

This change showed up as a significant compile-time regression (before it was reverted): https://llvm-compile-time-tracker.com/compare.php?from=85ccdcaa502ee2c478f2d0ba2b1e217117b69032&to=d4ba28bddc89a14885218b9eaa4fbf6654c2a5bd&stat=instructions

Wed, Nov 18, 12:31 PM · Restricted Project
nikic committed rGf4a3969bffce: [Inline] Fix incorrectly dropped noalias metadata (authored by nikic).
[Inline] Fix incorrectly dropped noalias metadata
Wed, Nov 18, 12:23 PM
nikic committed rG0dca0b703428: [Inline] Expand test to show dropped metadata (NFC) (authored by nikic).
[Inline] Expand test to show dropped metadata (NFC)
Wed, Nov 18, 12:23 PM
nikic committed rG23aeadb89df3: [Inline] Fix incorrect noalias metadata application (PR48209) (authored by nikic).
[Inline] Fix incorrect noalias metadata application (PR48209)
Wed, Nov 18, 11:53 AM
nikic committed rGb51c290663cf: [Inline] Add test for PR48209 (NFC) (authored by nikic).
[Inline] Add test for PR48209 (NFC)
Wed, Nov 18, 11:53 AM
nikic added a comment to D91383: [BasicAA] Make alias GEP positive offset handling symmetric.

@uabelho Thanks for the report! I have reverted the assert in https://github.com/llvm/llvm-project/commit/85ccdcaa502ee2c478f2d0ba2b1e217117b69032. We'll have to fix this asymmetric in phi-phi aliasing before re-enabling it.

Wed, Nov 18, 11:07 AM · Restricted Project
nikic committed rG85ccdcaa502e: [BasicAA] Remove assert in AA evaluator (authored by nikic).
[BasicAA] Remove assert in AA evaluator
Wed, Nov 18, 11:05 AM
nikic added a comment to D91576: [MemCpyOpt] Correctly merge alias scopes during call slot optimization.
In D91576#2401162, @hoy wrote:
Wed, Nov 18, 12:52 AM · Restricted Project
nikic added a comment to D91649: [AA] Split up LocationSize::unknown().
  • LocationSize::unknownMaybeNegative() seems somewhat ambiguous in isolation; unless I see the other name or have the context from this patch. I'm either keep LocationSize::unknown or make it even more verbose LocationSize::unknownPositiveOrNegative().
Wed, Nov 18, 12:40 AM · Restricted Project

Tue, Nov 17

nikic added a comment to D91250: Support intrinsic overloading on unnamed types.

Can't really comment on whether this makes sense approach wise. For testing, I'd suggest to test that this is correctly handled in bitcode serialization/loading as well, and to drop the current hack in PredicateInfo and see if it works fine for that use case.

Tue, Nov 17, 12:01 PM · Restricted Project
nikic added a comment to D91562: [LSR] Drop poison flags for post-inc IVs (PR46943).

The approach here doesn't look right to me. Poison flags have no relation to the position where a certain instruction is executed. What's relevant is where the instruction is used.

Tue, Nov 17, 11:53 AM · Restricted Project
nikic retitled D91562: [LSR] Drop poison flags for post-inc IVs (PR46943) from Fix for LLVM bug 46943. to [LSR] Drop poison flags for post-inc IVs (PR46943).
Tue, Nov 17, 11:48 AM · Restricted Project
nikic added reviewers for D91576: [MemCpyOpt] Correctly merge alias scopes during call slot optimization: jdoerfert, jeroen.dobbelaere.
Tue, Nov 17, 11:44 AM · Restricted Project
nikic added a comment to D91576: [MemCpyOpt] Correctly merge alias scopes during call slot optimization.

I don't get this fix. Adding extra scopes to alias.scope should always be conservatively correct. I think there may be some confusion here due to the malformed noalias metadata you're using.

Tue, Nov 17, 11:42 AM · Restricted Project
nikic requested review of D91649: [AA] Split up LocationSize::unknown().
Tue, Nov 17, 11:20 AM · Restricted Project
nikic updated the diff for D90094: [BasicAA] Handle recursive queries more efficiently (NFCI).

Strip pointer casts in GlobalsModRef. This *might* be the cause for observed differences, as previously the call from BasicAA would have been performed with casts stripped.

Tue, Nov 17, 9:27 AM · Restricted Project
nikic committed rGcb4fc25c9189: [BasicAA] Make alias GEP positive offset handling symmetric (authored by nikic).
[BasicAA] Make alias GEP positive offset handling symmetric
Tue, Nov 17, 9:06 AM
nikic closed D91383: [BasicAA] Make alias GEP positive offset handling symmetric.
Tue, Nov 17, 9:05 AM · Restricted Project
nikic added a reviewer for D91576: [MemCpyOpt] Correctly merge alias scopes during call slot optimization: nikic.
Tue, Nov 17, 12:33 AM · Restricted Project

Sun, Nov 15

nikic planned changes to D91482: [BasicAA] Remove unnecessary known size requirement.

While I believe this change is correct, there are some incorrect AA uses that should be addressed first. There's one in MemorySSA wrt phi translation over decreasing cycles (https://reviews.llvm.org/D87778#inline-852179), and another in ParallelDSP (see test failure).

Sun, Nov 15, 11:53 AM · Restricted Project
nikic added inline comments to D87778: [MemorySSA] Be more conservative when traversing MemoryPhis..
Sun, Nov 15, 11:48 AM · Restricted Project
nikic committed rG3b7f84d97fa5: [AA] Add missing AAQI parameter (authored by nikic).
[AA] Add missing AAQI parameter
Sun, Nov 15, 11:32 AM
nikic added a reverting change for rG1ec6e1eb8a08: [SCEV] Factor out part of wrap flag detection logic [NFC-ish]: rG9ace4b337fe2: Revert "[SCEV] Factor out part of wrap flag detection logic [NFC-ish]".
Sun, Nov 15, 1:28 AM
nikic committed rG9ace4b337fe2: Revert "[SCEV] Factor out part of wrap flag detection logic [NFC-ish]" (authored by nikic).
Revert "[SCEV] Factor out part of wrap flag detection logic [NFC-ish]"
Sun, Nov 15, 1:28 AM

Sat, Nov 14

nikic requested review of D91482: [BasicAA] Remove unnecessary known size requirement.
Sat, Nov 14, 10:13 AM · Restricted Project