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.

Yeah, when I was implementing some of the P9 instructions that naturally fit the BUILD_VECTOR nodes, I realized that we produce poor code for some patterns. I wanted to see how bad this situation is so I wrote the C test case that is added as a comment to build-vector-tests.ll and examined the assembly. Then I picked off the patterns that produce poor code and fixed them one by one.

In D26066#581855, @nemanjai wrote:

Here are some examples of terrible code gen currently (rather than providing code, I'm just illustrating these with pseudo-SDAG nodes):

Thanks Nemanja for comments on the testcase.

@MatzeB
Do you have further comments on this?

There has been many comments here, but only a couple of minor issues has to be addressed. (check indentation, remove whitespace, change some variable names). If you don't mind to approve it, I will fix those before committing.

There is one comment not yet addressed. Size of latest legal integer is currently hardcoded. As I responded to that comment I will fix it. I want to look at this a bit more to see if it makes sense to add some extensions to it.

In D23614#580123, @kparzysz wrote:

Could you move the testcase into the right directory?

Committed rL284995

Commited 284983