In D54498#1305662, @jdoerfert wrote:

In D54498#1305504, @fhahn wrote:

FWIW TransitiveCallSite seems like a slightly better name to me than AbstractCallSite, as the CallSite class is already an abstraction over different kinds of calls. As for the location of the class, if it is tied to an analysis that gathers the transitive call sites, having it in lib/Analysis/ might make sense initially as well.

There are a few things to consider but at the end I will not fight anybody for a certain name or source location. Things that led me to
do this design included:

• Abstract call sites, as implemented here, are not *only* transitive call sites but can be transitive or direct call sites.
• The call site abstraction is going away soon [0]. Afterwards, the only abstraction will be this one on top of the new CallBase class.
• The analysis to identify callback functions doesn't exist yet. When it does, I would probably only annotate the IR and use these annotations in the constructor of AbstractCallSite. This way we can easily pass uses to the constructor and do not need to a dedicated pass that provides both direct and transitive calls (separately or unified through the AbstractCallSite abstraction).

In D54498#1298538, @jdoerfert wrote:

Thanks for the comments!

In D54498#1298137, @fhahn wrote:

I think using this in IPSCCP would be a great showcase too, given that it is more powerful (and enabled in the default pipeline). lib/Transforms/Scalar/SCCP.cpp contains the solver that is used for both per function SCCP and IPSCCP.

SCCP is certainly on my list, but one of the IPOs I haven't looked into yet.

Constant propagation is an obvious use case for this. What other transformations do you envision using this?

I have however a version of argument promotion (which turns out to be really important wrt parallel codes).
I also have extended the attribute deduction pass in various ways with an older version of the abstract/transitive/callback call sites, see here and here.
Initial results from the prototype implementations can be found in the talk and the paper (see commit message). Basically, they are as good as the dedicated transformations we implemented and evaluated here.

ping.

Committed as rL346857, thanks for all the comments! I will create a few tickets for follow up work.

I think using this in IPSCCP would be a great showcase too, given that it is more powerful (and enabled in the default pipeline). lib/Transforms/Scalar/SCCP.cpp contains the solver that is used for both per function SCCP and IPSCCP.

submitted as rL346769

Thanks, I'll update the comments before I commit this.

I am abandoning this for now, as I updated D51431 to avoid adding nonnull, because even with freeze it would be too restrictive.

Thanks for the comments. For now, I added a custom DenseMapInfo to D49491, but I hope I can revisit this once I have more time!

Address comments: renamed CSP functions, add assertion ensuring there is a single edge between PredBB and BB. I hope this addresses all comments

In D49489#1296800, @rengolin wrote:

So, IIUC, the way you only get the interleave info on instructions and map to VPlan is because we don't yet have scalar evolution in VPlan, so we need to do that in Instruction and then map to VPInstruction.

In the future, once we can (perhaps using the same template technique) have SCEV on VPlan, we won't need the Old2New map anymore, and then the analysis will be independent of the underlying type.

Di I get that right?

Thanks for all the review!

Updated to traverse regions.

Rebased, updated the use auto * for cast<>.

Thanks for adding the include, I just saw the commit after I submitted the forward declaration.

Thanks, I'll address the comments on Monday.

updated to use DTU.getDomTree(), thanks!

I've updated the test to explicitly use function(require<domtree>,require<postdomtree> before and after ipsccp to make sure the analyses are preserved. I couldn't find a flag to verify the PDT unfortunately.

In D49491#1291871, @ABataev wrote:

I have no more comments.

Thanks, reverted in rL346492. Looks like this exposed a bug in preserving the DT in CallSiteSplitting. I'll have a look and recommit with a fix.

Do you need someone to commit this?

In D47259#1292001, @chandlerc wrote:

FWIW, still LGTM and I think you can go ahead and submit.

Thank you very much for all the reviews, Eli!

Thank you very much for having a look! I updated the patch to use deleteBB, added a comment in lib/Transform/IPO/SCCP.cpp on why nullptr is passed in as DT for the legacy pass manager and renamed AnalysisForFn to AnalysisResultsForFn.

In D49491#1291680, @ABataev wrote:

Just one more question: how do you estimate that the vectorized code is more effective than the scalar one? What's the criterion?

Fixed test case (new pm needs -passes='ipsccp,function(verify<domtree>) to verify the DTs afterwards), also pass DTU to other changeToUnreachable and ConstantFoldTerminator calls.

Rebased after DTUpdater changes landed.

@trentxintong I just updated D44627, which stops backtracking once we hit IDom(call site), because conditions further up won't be impacted by splitting. Any chance you could try this patch to see if it improves things on your test case?

Resurrecting the patch. The cases we loose (propagating %a == null in test_cond_no_effect) is now handled by ipsccp, which is a better place IMO. @junbuml what do you think?

I stripped adding nonnull for now. Even with freeze, we have to be far too conservative about the merging rules. This patch is now more an enabler to make it easier to use ValueLAtticeElement, with range support. What do you think?

Turned check into assertion, added a comment explaining the if (!InnerLoop) {} block and another comment to findInductionAndReductions.

Is this still an issue? If so, please add a test case. Requesting changes for now, to clear up my queue.

instcombine now propagates nonnull in the cases mentioned here. I'll resign to clear up my pipeline.

Thank you very much Davide :) In case you are interested in reviewing another small NewGVN patch, there would be D51595.

use unsigned instead of auto

Address comments and add checks for simple loads/stores in areVectorizable().

ping. Eli it would be great if you could have another look now that the patch removing support for inner loop only reductions is committed and this patch is simpler.

In D52930#1267561, @mkazantsev wrote:

Added notion of this option being slow. It can be later be turned on in EXPENSIVE_CHECKS mode along with the other two. I tried turning it on now and currently all three turned on fail 2 tests. Apparently a bug in there.

Abandoning for now

If interleaving in LoopVectorize is the problem, maybe setting ForceTargetMaxInterleaveFactor to 1 would be worth a try to just disable interleaving in LV.

Address comments, thanks!

In D51168#1283117, @efriedma wrote:

I don't really know the relevant code well enough, and I don't think I'll have time to study it in the near future.

You might want to ask on llvmdev to see if there's anyone interested in reviewing NewGVN patches.

Thanks Eli!

Updated after removing support for inner loop only reductions in D53027. The code is now slightly simpler. What do you think?

In D53027#1277836, @efriedma wrote:

If you aren't finding real loops where this matters, simply rejecting loops with a reduction like this seems fine.

ping

Committed, thank you very much for the patch!

I can commit it tomorrow.

ping

Thank you very much Eli! I'll take a look at the other metadata kinds as well.

Thanks!

ping

In D53562#1273812, @samparker wrote:

Hey Florian,

It was brought to my attention that the scheduler still wasn't enabled because of the missing feature. I've now added this and the geomean improvement is 2.23%. I will shortly add a couple of tests too.

In D53027#1268066, @efriedma wrote:

This analysis seems very fragile.

LGTM. AFAIK cortex-a57 and cortex-a72 are close enough for this to be beneficial. AArch64 re-uses the A57 model for A72 too.

Thanks! I'll submit it in a few days, with DoNotOptimize.