ping

Yes. Please review proposed fix https://reviews.llvm.org/D93803. Evgeniy

Thanks for letting me know. Looking now...

Updated test case due to https://reviews.llvm.org/D79485 has landed.

Rebase

Rebase

Addressed comments

Added test case.

Addressed comments

I had already tried to measure performance with the test-suite previously with out success. This time again I observe big variation. I'm using dedicated performance machine which runs only my process. I've build test-suite as follows:

Initial update

Updated comments

Rebase

Rebase

Rebase

Rebase

Rebase

LGTM

LGTM. But let chance for others to comment. Thanks.

In D90371#2363420, @asbirlea wrote:

This LGTM.
I'd like to understand better what you mean by not calling partialOverwrite. If the functionality in that call would be included in the isOverwrite call, note that there are call-paths that would do more work than they do now, and I thought there was a previous refactoring done to avoid this.

This currently seems to lead to a bit less stores removed overall on MultiSource/SPEC2000/SPEC2006 with -O3 -flto, but that might be related to some of the limits needing adjustments.

I found PartialLimit preventing DSE to optimize one of my simple cases. Here is the fix https://reviews.llvm.org/D90371 which I believe is beneficial regardless.

Thanks for giving it a try!

Rename DefLoc to ResDefLoc to fix name conflict.

Yeah there have been some substantial changes to the way we find the candidates to remove, but I think the same idea should still apply. It would be ideal to update & rebase the patch, but if you won't have time to do so it would be great if you could share the older version regardless.

Rebased and uploaded here https://reviews.llvm.org/D90095

LGTM with a nit.

I think it makes sense to introduce the proposed heuristic since there is no easy way to estimate cost of the code generated by SCEVExpander at the moment.

Fixed according to Ayal's request.

Does this look fine?

Sanitize MaxIC upfront.

ping

It turned out that in the targeted case there is the following expression min(a,b) = min(min(a,c), b) where rhs min is reassociated. Due to this foldICmpWithMinMax can't handle it. I've uploaded a series of 3 patched for NaryReassociate (https://reviews.llvm.org/D88285, https://reviews.llvm.org/D88286, https://reviews.llvm.org/D88287) which converts that to min(a,b) = min(min(a,b), c). With that in place foldICmpWithMinMax does its job.

ping

In D87679#2296877, @fhahn wrote:

LGTM, thanks. Suggestion inline, but quite subjective, so feel free to ignore.

@fhahn ping

Hi @ebrevnov,

I just wanted to briefly check in to see if you made any progress on this or if you think it is worth adding a bug report to keep track of the improvement?

Minor change to test case

Addressed comments

ping

ping

I won't have time to look into this in the near future unfortunately, as I am mainly focused on getting things in good shape to enable DSE & MemorySSA by default. It might be good to file a bug report so this opportunity does not drop off our radar. If anyone is interested in giving implementing it a try, even better :)

@fhahn is this patch supposed to support the following case?

In D79485#2231383, @hiraditya wrote:

Is the test failure related to this patch?

In D79485#2218349, @hiraditya wrote:

..anyway I can try to describe how thing work using some example. Is this what you are looking for?

yes. Thank you.

In D79485#2218350, @hiraditya wrote:

please run clang-format.

Added description of the algorithm to the header + formatting.

I think making 3 transformation passes immune to optnone ruins the whole idea. You really should have mechanism to set very specific and well documented overrides to the default.

I think this should be put on hold until D85781 reworked and relanded again.

In D85781#2220627, @xbolva00 wrote:

In D85781#2220576, @ebrevnov wrote:

How your last commit differs from the approved one? Why you have so many additional changes in the tests?

Patch affects many codegen tests so I updated them mostly with updater scripts.

How your last commit differs from the approved one? Why you have so many additional changes in the tests?

In D79485#2217052, @hiraditya wrote:

Is this work based on any paper/implementation?

I don't propose anything completely new here. New algorithm does essentially the same as existing one but

1. fixes several correctness issues
2. extends the approach to handle loops and invokes in a universal way