I am not sure whether our approach can be integrated, with this patch. The approaches are completely different. But I can write a summary of our algorithm and we can discuss the algorithm first. We are working on rewriting the code and we can post it when it is ready. Since @eastig has also switched to a similar approach (analyze-then-transform), it will be interesting to have a discussion about different algorithms.

@alexey.zhikhar and I had a look at your patch. Some findings that might be interesting:

Our pipeline has some differences with the default pass pipeline. The problem was exposed when playing with the pipeline. As Jimmy mentioned it is in one of the smaller benchmarks in the test suite. Overall impact is small, but on the other hand, we are not adding any compile time or any other kind of cost. So I don't see an issue. The remaining question is functional stability. The code looks quite correct to me. You have also looked into it in the past so I think on that issue we are fine. There is always a chance that we expose some other bug somewhere else. We have not observed that issue and given limited impact of the patch it is not very likely. Anyways, I think overall this should be fine to be merged. Still I wait a little more to see if any of the reviewers has any concern in the next couple of days.

Is there any concern about this patch? We have reviewed this internally and it looks good to me. It has been also tested extensively. The condition on the load instruction does not seem to have any role in correctness of the transformations and seems to be a completely arbitrary condition. I accept the patch for now, but I will wait and prefer to hear from one of the other reviewers before committing it.

Committed
https://reviews.llvm.org/rG53b95a3cb6a7

committed
https://reviews.llvm.org/rG61e54fd60c43

In D69059#1767016, @c-rhodes wrote:

@mgudim I've created a patch (D70909) also implementing these intrinsics but I was unaware of this patch, sorry about that. Can this be merged now? I can update D70909 to only add the SVE2 intrinsics .

committed
https://reviews.llvm.org/rGe55b536d7d81

Merged the patch https://reviews.llvm.org/rGed7bcb2cb157

LGTM. @sdesmalen @huntergr please let us know if you have any concern

Committed in https://reviews.llvm.org/rG1e9de0215f04

In D68476#1717715, @huntergr wrote:

Hi, thanks for waiting a bit. LGTM.

This looks straighforward. So LGTM. Since this is @mgudim's first patch, I will commit it for him. But I will wait a bit more (before committing) just in case @huntergr has any comments.

Committed.

In D68098#1687669, @huntergr wrote:

LGTM.

You can add the extra multiclass for ftsmul if you wish, but it's not needed until someone implements a matching pattern for that instruction. We only match it against an ACLE intrinsic downstream, not common SDag nodes.

@huntergr Updated according to your comment. Please check. Thanks!

Thanks @huntergr. Will update the patch. Once I am sure we got this one right, we can look into others.

Committed in rL350931

In D55264#1345253, @brzycki wrote:

@amehsan gentle ping. Have you resolved your push access?

Thanks @brzycki for careful review. I need to revive my commit access (not used it for two years now), then I will commit this.

Thanks @brzycki. I believe I have addressed all your comments. Will upload the modified patch shortly.

In D32888#771149, @wenbos wrote:

Hi Hal, can you commit it since I have no commit access?

@echristo explicitly approved this in his last comment, but I think he forgot to change the action box. So I accept just for easier book-keeping. (Eric is in vacation, AFAIK)

https://reviews.llvm.org/rL287872

https://reviews.llvm.org/rL290102

This patch, as is, is incorrect and requires some changes to proceed. Abandoning.

rL289813

Ping

In D25200#610857, @spatel wrote:

In D25200#610847, @amehsan wrote:

You are right (I just tried it :-) If that is the only concern is it possible to approve this and I will remove that line from the switch stmt. (I think it does not hurt to leave the testcases though) and commit.

Yes, it's good to keep the tests to make sure the underlying assumption doesn't break.
LGTM - although I don't think we need a switch for just 2 cases. :)

In D25200#610628, @spatel wrote:

In D25200#610626, @spatel wrote:

Can you just assert that the predicate is not 'ugt' in isAnyBitSet() and simplify that a bit more? Ie, we don't need a switch if we know that eq/ne are the only predicate possibilities for this transform.

"assert" is probably too strong...I've made that mistake before. :)
But I think it's still safe to ignore the 'ugt' case and assume that InstCombine will churn that away sooner or later.

Added the clean up step that we discussed. Also, made some changes in the comments in the code.

Will shortly update the patch.

In D25200#600208, @amehsan wrote:

@spatel @eli.friedman Could one of you please look at this patch? @majnemer said he is busy and will be even busier next week.

@spatel @eli.friedman Could one of you please look at this patch? @majnemer said he is busy and will be even busier next week.

https://reviews.llvm.org/rL287329

https://reviews.llvm.org/rL287334 (fixed the comment in a followup commit though).

Could someone take a look at this?

I actually wanted to check some surrounding guards.

more context?

In D25482#585513, @amehsan wrote:

In D25482#585510, @MatzeB wrote:

In D25482#585491, @amehsan wrote:

In D25482#585490, @MatzeB wrote:

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

Correct :)

So we can remove targetSchedulesPostRAScheduling() and replace it with the empty function overload after adding addPostRASched().

That's a reasonable change for this patch. I'll wait to see Eric's comments well, before updating the patch though. Just to save time.

I put this back in review as Eric wanted to take a look.

now fully tested with test-suite.

Just finished lnt test and there are some failures. I will update when my investigation of the failures is done.

Completed the unit test and code. I decided that extending this for signed comparison is not necessary good, because two zero extends that we generate for unsigned comparison, will be sign extension for signed comparison. That means the signed version will have two more instructions. There might be still more useful cases, but I think they are different enough and we don't need to make them part of this patch.

Thanks David for the review. I have tested this with test-suite. I forgot to do so, after I made first set of changes.

In D25482#585510, @MatzeB wrote:

In D25482#585491, @amehsan wrote:

In D25482#585490, @MatzeB wrote:

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

Correct :)

So we can remove targetSchedulesPostRAScheduling() and replace it with the empty function overload after adding addPostRASched().

In D25482#585490, @MatzeB wrote:

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

I opened a PR for this and copied the last couple of comments there. CC'ed Eric and Matt on the PR.

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Eric, asked for a little more clean up.

Regarding (2): I think we need to make sure InstCombine and target Independent lowering are doing the right thing for these. Fixing as late as possible does not seem correct to me. But I leave to other reviewers to decide.