mssimpso (Matthew Simpson)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 19 2014, 12:23 PM (160 w, 5 d)

Recent Activity

Mon, Jan 15

mssimpso updated the diff for D39869: [Inliner] Inline through indirect call sites having !callees metadata.

Addressed David's comments.

Mon, Jan 15, 3:11 PM

Fri, Jan 12

mssimpso added a comment to D42007: [SimplifyCFG] Try to change store operation type when sinking.

I might consider holding my nose, if this restores something that was there.
I think the other two passes we have for doing sinking aren't currently enabled (GVNSink and Sink.cpp), although unless somebody puts effort in them this will always be a chicken-egg problem.
I'm not sure I'll have the time to review this patch carefully, I don't want to put you on the hook, but if you can consider an alternative, that would be great.
If you look at the original review you'll notice that I was fundamentally opposed to the change, see e.g. https://reviews.llvm.org/D38566#926530
(FWIW, it doesn't matter where we move sinking/hoisting there will be always some case that we can't get properly. We, of course should prioritize for the common case, at least IMHO).

Fri, Jan 12, 3:19 PM
mssimpso abandoned D42007: [SimplifyCFG] Try to change store operation type when sinking.
Fri, Jan 12, 2:51 PM
mssimpso added a comment to D42007: [SimplifyCFG] Try to change store operation type when sinking.

Thanks for the quick feedback Danny/Davide. I definitely appreciate the point that SimplifyCFG may not be the best place for this kind of transformation. Davide, I assume the dedicated pass you're referring to is GVNSink? I don't think that pass is enabled yet (I haven't been closely following the progress, so I'm not sure what's holding it up at this point), but it's possible GVNSink would indeed catch the cases this patch does. I haven't tested that yet.

Fri, Jan 12, 2:49 PM
mssimpso added a comment to D41948: [SLP] Fix vectorization for tree with trunc to minimum required bit width..

Thanks for cleaning this up.

Fri, Jan 12, 2:14 PM
mssimpso created D42007: [SimplifyCFG] Try to change store operation type when sinking.
Fri, Jan 12, 12:24 PM

Thu, Jan 11

mssimpso added inline comments to D41913: [LV] Don't call recordVectorLoopValueForInductionCast for newly-created IV from a trunc..
Thu, Jan 11, 1:56 PM
mssimpso accepted D41766: [MachineCombiner] Add check for optimal pattern order..

TBH I was hoping for a more complete approach to testing this, but I thought I share this relatively straight forward check.

Thu, Jan 11, 12:56 PM
mssimpso added a comment to D41913: [LV] Don't call recordVectorLoopValueForInductionCast for newly-created IV from a trunc..

Hi Dimitry,

Thu, Jan 11, 12:42 PM

Mon, Jan 8

mssimpso added a comment to D41766: [MachineCombiner] Add check for optimal pattern order..

Hi Florian,

Mon, Jan 8, 11:58 AM
mssimpso added a comment to D39869: [Inliner] Inline through indirect call sites having !callees metadata.

I think 'tryToPromote' method should be moved to the indirect call promotion pass so that the callback to inline cost/benefit is exposed there (a more general cost/benefit model needs to be developed for indirect call promotion). Initially, I think we can limit the use of the getInlineCost callback to cases where profile data is not available, this will achieve what this patch is doing without affecting existing promotion heuristics.

Mon, Jan 8, 11:26 AM

Thu, Jan 4

mssimpso added a comment to D39869: [Inliner] Inline through indirect call sites having !callees metadata.

Ping.

Thu, Jan 4, 4:38 PM
mssimpso added a comment to D39339: [CallGraph] Refine call graph for indirect calls with !callees metadata.

Ping.

Thu, Jan 4, 4:37 PM

Wed, Dec 27

mssimpso committed rL321491: [AArch64] Change order of candidate FMLS patterns.
[AArch64] Change order of candidate FMLS patterns
Wed, Dec 27, 7:26 AM
mssimpso closed D41587: [AArch64] Change order of candidate FMLS patterns.
Wed, Dec 27, 7:25 AM

Tue, Dec 26

mssimpso added a comment to D41587: [AArch64] Change order of candidate FMLS patterns.

Thanks for spotting this!
The reordering looks good to me, give the current behavior of the MachineCombiner it makes sense to move up the more profitable patterns.

However, the order in which we add the patterns to the list of candidates determines the transformation that takes place, since only the first pattern that matches will be used.

This behavior is not ideal though, especially as some order of patterns might be good for one micro architecture, whereas a different order is better for a different micro architecture.

I'll try to look into this. Maybe it's worth to change the MachineCombiner to try all available patterns.

Tue, Dec 26, 12:45 PM
mssimpso created D41587: [AArch64] Change order of candidate FMLS patterns.
Tue, Dec 26, 12:15 PM

Wed, Dec 20

mssimpso updated the diff for D39869: [Inliner] Inline through indirect call sites having !callees metadata.

Rebased.

Wed, Dec 20, 12:01 PM
mssimpso committed rL321210: [ICP] Expose unconditional call promotion interface.
[ICP] Expose unconditional call promotion interface
Wed, Dec 20, 11:27 AM
mssimpso closed D40751: [ICP] Expose unconditional call promotion interface.
Wed, Dec 20, 11:27 AM

Tue, Dec 19

mssimpso added a comment to D40751: [ICP] Expose unconditional call promotion interface.

Thanks David! I'll make those changes before committing.

Tue, Dec 19, 12:08 PM

Dec 14 2017

mssimpso updated the diff for D40751: [ICP] Expose unconditional call promotion interface.

Addressed David's comments. Thanks again!

Dec 14 2017, 2:48 PM
mssimpso added a comment to D40751: [ICP] Expose unconditional call promotion interface.

Thanks again for the comments, David. I'll update the patch shortly.

Dec 14 2017, 11:50 AM

Dec 13 2017

mssimpso added a comment to D39339: [CallGraph] Refine call graph for indirect calls with !callees metadata.

Ping.

Dec 13 2017, 2:44 PM

Dec 12 2017

mssimpso added inline comments to D40751: [ICP] Expose unconditional call promotion interface.
Dec 12 2017, 10:09 AM
mssimpso updated the diff for D39869: [Inliner] Inline through indirect call sites having !callees metadata.

Moved demoteCall, previously in D40751, to this patch.

Dec 12 2017, 10:08 AM
mssimpso updated the diff for D40751: [ICP] Expose unconditional call promotion interface.

Addressed David's comments. Thanks for the feedback!

Dec 12 2017, 10:08 AM

Dec 11 2017

mssimpso added a comment to D40751: [ICP] Expose unconditional call promotion interface.

Ping.

Dec 11 2017, 3:00 PM

Dec 6 2017

mssimpso updated the diff for D40751: [ICP] Expose unconditional call promotion interface.

Rebased. Thanks!

Dec 6 2017, 1:45 PM
mssimpso committed rL319963: [PGO] Make indirect call promotion a utility.
[PGO] Make indirect call promotion a utility
Dec 6 2017, 1:23 PM
mssimpso closed D40658: [PGO] Make indirect call promotion a utility by committing rL319963: [PGO] Make indirect call promotion a utility.
Dec 6 2017, 1:23 PM

Dec 1 2017

mssimpso edited dependencies for D39869: [Inliner] Inline through indirect call sites having !callees metadata, added: 1; removed: 1.
Dec 1 2017, 1:26 PM
mssimpso removed a dependent revision for D40658: [PGO] Make indirect call promotion a utility: D39869: [Inliner] Inline through indirect call sites having !callees metadata.
Dec 1 2017, 1:26 PM
mssimpso added a dependent revision for D40751: [ICP] Expose unconditional call promotion interface: D39869: [Inliner] Inline through indirect call sites having !callees metadata.
Dec 1 2017, 1:26 PM
mssimpso added a dependent revision for D40658: [PGO] Make indirect call promotion a utility: D40751: [ICP] Expose unconditional call promotion interface.
Dec 1 2017, 1:25 PM
mssimpso added a dependency for D40751: [ICP] Expose unconditional call promotion interface: D40658: [PGO] Make indirect call promotion a utility.
Dec 1 2017, 1:25 PM
mssimpso created D40751: [ICP] Expose unconditional call promotion interface.
Dec 1 2017, 1:25 PM
mssimpso updated the diff for D40658: [PGO] Make indirect call promotion a utility.

Made this patch NFC and split out the functional changes.

Dec 1 2017, 1:25 PM
mssimpso added a comment to D39556: [ConstantFold] Support vector index when factoring out GEP index into preceding dimensions.

One minor comment. Otherwise, this looks fine to me.

Dec 1 2017, 12:23 PM

Nov 30 2017

mssimpso added a comment to D40658: [PGO] Make indirect call promotion a utility.

Can you make this patch a pure NFC one and split out the enhancement part into another patch?

Nov 30 2017, 12:36 PM
mssimpso added a dependency for D39869: [Inliner] Inline through indirect call sites having !callees metadata: D40658: [PGO] Make indirect call promotion a utility.
Nov 30 2017, 9:51 AM
mssimpso added a dependent revision for D40658: [PGO] Make indirect call promotion a utility: D39869: [Inliner] Inline through indirect call sites having !callees metadata.
Nov 30 2017, 9:51 AM
mssimpso updated the diff for D39869: [Inliner] Inline through indirect call sites having !callees metadata.

Rebased on top of D40658. We now use a common indirect call promotion interface.

Nov 30 2017, 9:51 AM
mssimpso created D40658: [PGO] Make indirect call promotion a utility.
Nov 30 2017, 9:50 AM

Nov 27 2017

mssimpso added a comment to D39869: [Inliner] Inline through indirect call sites having !callees metadata.

Yes, I think it is good to do the necessary refactoring work first.

Nov 27 2017, 11:09 AM
mssimpso added a comment to D39869: [Inliner] Inline through indirect call sites having !callees metadata.

A high level comment: I think the right direction is to share the implementation with the existing IndirectCallPromotion.cpp pass. The analyzer can synthesize the MD_prof profile data for the indirect targets, though the actual target count can be synthesized with some heuristics.

Nov 27 2017, 11:04 AM
mssimpso added a comment to D39869: [Inliner] Inline through indirect call sites having !callees metadata.

Ping.

Nov 27 2017, 8:42 AM

Nov 17 2017

mssimpso added inline comments to D39556: [ConstantFold] Support vector index when factoring out GEP index into preceding dimensions.
Nov 17 2017, 11:09 AM

Nov 9 2017

mssimpso added a dependent revision for D39339: [CallGraph] Refine call graph for indirect calls with !callees metadata: D39869: [Inliner] Inline through indirect call sites having !callees metadata.
Nov 9 2017, 1:53 PM
mssimpso added a dependency for D39869: [Inliner] Inline through indirect call sites having !callees metadata: D39339: [CallGraph] Refine call graph for indirect calls with !callees metadata.
Nov 9 2017, 1:53 PM
mssimpso created D39869: [Inliner] Inline through indirect call sites having !callees metadata.
Nov 9 2017, 1:53 PM
mssimpso updated the diff for D39339: [CallGraph] Refine call graph for indirect calls with !callees metadata.
  • Incorporated the change into the lazy call graph as well. The metadata is represented as ref edges there.
  • Added a helper in CallSite to get the functions listed in the metadata.
Nov 9 2017, 1:52 PM

Oct 31 2017

mssimpso committed rL316994: [InstCombine] Simplify selects that test cmpxchg instructions.
[InstCombine] Simplify selects that test cmpxchg instructions
Oct 31 2017, 5:34 AM
mssimpso closed D39383: [InstCombine] Simplify selects that test cmpxchg instructions by committing rL316994: [InstCombine] Simplify selects that test cmpxchg instructions.
Oct 31 2017, 5:34 AM

Oct 27 2017

mssimpso created D39383: [InstCombine] Simplify selects that test cmpxchg instructions.
Oct 27 2017, 12:50 PM
mssimpso accepted D38677: [ConstantFold] Fix a crash when folding a GEP that has vector index.

Thank you, Matt.

As you pointed out, supporting vector previous index is not difficult. I choosed to bail out at first because

  1. We already bail out when the current index is a vector.
  2. I initially planned to fix the crash in this patch to unblock the release and add the vector index support for both current and previous index.
  3. I think this piece of algorithm work better if we iterate from the last index to the first. So, we don't need to recursively call this function in the following case.

    ` @block = global [8 x [64 x [8192 x i8]]] zeroinitializer, align 1

    define i8* @vectorindex() { %1 = getelementptr inbounds [8 x [64 x [8192 x i8]]], [8 x [64 x [8192 x i8]]]* @block, i64 0, i64 0, i64 63, i64 8192 ret i8* %1 } `
Oct 27 2017, 5:54 AM

Oct 26 2017

mssimpso added a comment to D38677: [ConstantFold] Fix a crash when folding a GEP that has vector index.

How hard do you think it would be to update this transformation to reason about vector GEPs rather than bailing out? (I've left some inline comments if you want to give it a try). Having said that, I'm fine with that patch as is. But you should at least add a comment explaining why we're giving up.

Oct 26 2017, 11:54 AM
mssimpso created D39339: [CallGraph] Refine call graph for indirect calls with !callees metadata.
Oct 26 2017, 11:35 AM

Oct 25 2017

mssimpso committed rL316625: Attempt to unbreak the expensive-checks-win bot.
Attempt to unbreak the expensive-checks-win bot
Oct 25 2017, 3:47 PM
mssimpso abandoned D36432: [IPSCCP] Add function specialization ability.

I'm abandoning this for now to close the review. Davide/Danny, please feel free to revive this patch if you want. To summarize the main points in the review, we need to work on the cost model more to enable something like this by default.

Oct 25 2017, 1:21 PM
mssimpso committed rL316576: Add CalledValuePropagation pass.
Add CalledValuePropagation pass
Oct 25 2017, 6:40 AM
mssimpso closed D37355: Add CalledValuePropagation pass by committing rL316576: Add CalledValuePropagation pass.
Oct 25 2017, 6:40 AM

Oct 16 2017

mssimpso committed rL315944: Add !callees metadata.
Add !callees metadata
Oct 16 2017, 3:22 PM
mssimpso closed D37354: Add !callees metadata by committing rL315944: Add !callees metadata.
Oct 16 2017, 3:22 PM
mssimpso updated the diff for D37355: Add CalledValuePropagation pass.

I updated this patch following the most recent changes to the generic solver, which allow clients to define custom LatticeKeys. I'll email llvm-dev before committing.

Oct 16 2017, 1:37 PM
mssimpso committed rL315919: [SparsePropagation] Enable interprocedural analysis.
[SparsePropagation] Enable interprocedural analysis
Oct 16 2017, 10:44 AM
mssimpso closed D37353: [SparsePropagation] Enable interprocedural analysis by committing rL315919: [SparsePropagation] Enable interprocedural analysis.
Oct 16 2017, 10:44 AM
mssimpso added a comment to D37353: [SparsePropagation] Enable interprocedural analysis.

Thanks very much for the reviews!

Oct 16 2017, 10:21 AM

Oct 13 2017

mssimpso committed rL315719: [IPSCCP] Move common functions to ValueLatticeUtils (NFC).
[IPSCCP] Move common functions to ValueLatticeUtils (NFC)
Oct 13 2017, 10:54 AM
mssimpso closed D37638: [IPSCCP] Move common functions to IPOUtils (NFC) by committing rL315719: [IPSCCP] Move common functions to ValueLatticeUtils (NFC).
Oct 13 2017, 10:54 AM

Oct 12 2017

mssimpso updated the diff for D37353: [SparsePropagation] Enable interprocedural analysis.

Implement the previously discussed approach using generic LatticeKeys. A few things to note about this version of the patch:

Oct 12 2017, 12:44 PM

Oct 10 2017

mssimpso committed rL315375: [SparsePropagation] Let the Instruction work list hold Values..
[SparsePropagation] Let the Instruction work list hold Values.
Oct 10 2017, 3:14 PM
mssimpso committed rL315373: [SparsePropagation] Use SmallVector for work lists.
[SparsePropagation] Use SmallVector for work lists
Oct 10 2017, 2:33 PM
mssimpso committed rL315356: [SparseSolver] Rename getLatticeState to getExistingValueState (NFC).
[SparseSolver] Rename getLatticeState to getExistingValueState (NFC)
Oct 10 2017, 1:19 PM
mssimpso updated the diff for D37638: [IPSCCP] Move common functions to IPOUtils (NFC).

Addressed Danny's comments.

Oct 10 2017, 12:03 PM
mssimpso added a comment to D37353: [SparsePropagation] Enable interprocedural analysis.

I agree, getStateMapForInstruction is a bit strange. I've been thinking about what to do here. And one of the simplest things I came up with was to just define a custom LatticeKey type like we do a LatticeVal. This would allow the solver to maintain a single map (meaning we no longer need something like getStateMapForInstruction), and give the client the flexibility to separate the kinds of values it cares about.

Oct 10 2017, 9:08 AM

Oct 9 2017

mssimpso added a comment to D37355: Add CalledValuePropagation pass.

Yes, I'm fine with the dependent patches.
As this is a large-ish change (and involves a fair amount of refactoring), if I was you, I'd send a mail to llvm-dev to avoid surprises explaining what's your plan (so that if there are people relying on bits being in a particular position for their out-of-tree passes/analyses they at least have an heads-up of what's going on)

Oct 9 2017, 1:38 PM
mssimpso added a comment to D37355: Add CalledValuePropagation pass.

Thanks very much for all of the reviews! Just to clarify, are you also OK with the dependent patches (the changes in D37353 to make the generic solver interprocedural and the NFC in D37638 that pulls out the IPO utilities)?

Oct 9 2017, 1:13 PM
mssimpso updated the diff for D37355: Add CalledValuePropagation pass.

Addressed Danny's comments.

Oct 9 2017, 11:17 AM

Oct 5 2017

mssimpso updated the diff for D37353: [SparsePropagation] Enable interprocedural analysis.

Rebased.

Oct 5 2017, 11:12 AM
mssimpso committed rL314996: [SparsePropagation] Move member definitions to header (NFC).
[SparsePropagation] Move member definitions to header (NFC)
Oct 5 2017, 11:05 AM
mssimpso closed D38561: [SparsePropagation] Move member definitions to header (NFC) by committing rL314996: [SparsePropagation] Move member definitions to header (NFC).
Oct 5 2017, 11:05 AM

Oct 4 2017

mssimpso added a dependent revision for D38561: [SparsePropagation] Move member definitions to header (NFC): D37353: [SparsePropagation] Enable interprocedural analysis.
Oct 4 2017, 2:05 PM
mssimpso added a dependency for D37353: [SparsePropagation] Enable interprocedural analysis: D38561: [SparsePropagation] Move member definitions to header (NFC).
Oct 4 2017, 2:05 PM
mssimpso updated the diff for D37355: Add CalledValuePropagation pass.

Addressed comments from Danny and Davide.

Oct 4 2017, 2:04 PM
mssimpso updated the diff for D37353: [SparsePropagation] Enable interprocedural analysis.

Addressed comments form Danny.

Oct 4 2017, 2:04 PM
mssimpso created D38561: [SparsePropagation] Move member definitions to header (NFC).
Oct 4 2017, 2:03 PM

Oct 3 2017

mssimpso added a comment to D38476: Template the sparse propagation solver instead of using void pointers.

Hi Danny,

Oct 3 2017, 12:05 PM
mssimpso added a comment to D37353: [SparsePropagation] Enable interprocedural analysis.

FYI: I'm about to rename getOrInitValueState to getValueState.
That is what SCCP calls it, and IMHO, we should be consistent.

I think you should do roughly the same (IE name the return-only ones as getExisting or getLattice, and not add "orInit" to the initializing ones)

Oct 3 2017, 7:55 AM

Oct 2 2017

mssimpso accepted D38476: Template the sparse propagation solver instead of using void pointers.

Thanks!

Oct 2 2017, 1:45 PM
mssimpso added a comment to D37353: [SparsePropagation] Enable interprocedural analysis.

It's worth noting: You can make propagation much faster by keeping a separate worklist for overdefined , and prioritizing it over the normal one.
This will move things to overdefined as quickly as possible.
(Because anything meet overdefined is just about always overdefined).

This is what SCCP does.
Again, just recording it here, i can do it in a followup.

Oct 2 2017, 12:39 PM
mssimpso added a comment to D37355: Add CalledValuePropagation pass.

Generally I feel like it's getting there. Some small comments, but I expect this to be ready to be committed soon'ish

Oct 2 2017, 12:26 PM
mssimpso added a comment to D37355: Add CalledValuePropagation pass.

I see this review has been hanging around a while, so i'll take a stab at reviewing it.

(Random note: I know you didn't do this, but getOrInitValueState is very verbose for a thing that everything is probably going to use by default.
IMHO, the API should be getOrInitValueState is renamed to getValueState, getValueState is renamed to getExistingValueState.
Similarly with the other calls.

Since there are no other users, this may be a good time to change it in a followup)

Generally looks very very good. I don't think i have any algorithmic complaints here at all.
If you agree with the templating vs void ptrs, i'm happy to go commit that patch.

Oct 2 2017, 12:24 PM
mssimpso added a comment to D37355: Add CalledValuePropagation pass.

Ping.

Oct 2 2017, 7:39 AM

Sep 29 2017

mssimpso committed rL314542: [LV] Use correct insertion point when type shrinking reductions.
[LV] Use correct insertion point when type shrinking reductions
Sep 29 2017, 11:09 AM

Sep 25 2017

mssimpso added a comment to D37355: Add CalledValuePropagation pass.

Hi everyone,

Sep 25 2017, 8:18 AM

Sep 18 2017

mssimpso added a comment to D37738: [SLPVectorizer] Generalize vectorizeStores to support loads as well NFC. .

Hi Florian,

Sep 18 2017, 2:33 PM

Sep 15 2017

mssimpso updated the diff for D37355: Add CalledValuePropagation pass.

Made untracked values overdefined.

Sep 15 2017, 11:55 AM
mssimpso updated the diff for D37638: [IPSCCP] Move common functions to IPOUtils (NFC).

Some updates.

Sep 15 2017, 9:57 AM

Sep 14 2017

mssimpso added a comment to D37353: [SparsePropagation] Enable interprocedural analysis.

Ideally we should consider moving to a model where the resolver doesn't really exist.
There are lots of subtleties in the way solver and resolver work in lockstep.
I'm pretty sure the only reason why SCCP didn't fall apart as GVN (the old one) or many other passes in tree is because Zhendong didn't fire up his fuzzer engine on it :)
I didn't have time to work on this (I had a couple of prototypes) and it's unlikely things will change in the near future (1-2 months).
So, for now I'm fine to leave the constant solver where it is but I'd still like to point out you just uncovered the tip of the iceberg.

Yes, and I just found your post about this very issue on llvm-dev from many months ago. Thanks!

Sep 14 2017, 2:40 PM