ae25a397e9de833ffbd5d8e3b480086404625cb7

Added two more tests to address the review comment.

LGTM

Ping

Ping

Ping

ping

Not sure I can have one at this point.
There is a follow-up patch I am making to enable it for AMDGPU. I will have some tests along with that.

@qcolombet can you review this patch?

Ping

rebase + elaborated the comment in Target.td

Ping

Added testcases.

Addressed the suggestion by @foad to include the Dereferenceable check on MMO.

Made the alignment check to 8-byte boundary instead of 16-byte.

Used -DAG to fix the test.

Ping

Used the FI directly in MI if available during ISel. Fixed an unhandled case during eliminateFrameIndex.
Also fixed the assertions to make sure the soffset field is relevant.

At global-isel, it has already been taken care of.
The asserts - the cases where it is expecting a register is specifically for sp relative accesses and won't cause a problem after this patch.

rebase + ping

Ping @stoklund @MatzeB or anyone interested in this patch.
Appreciate it if this gets reviewed soon.

Ping

Addressed the review comments.

Can we have this patch reviewed soon?
Appreciate any response.

It has been merged with https://reviews.llvm.org/rG207cd5f68fabbf760c7e66dc6c49e833f3f6f953.

Expanded the comment.

Incorporated the suggestions.

We are trying to have the kill intrinsic handling early during the instruction selection.
I can try to incorporate it all entirely there including the unify block code (if can't find a better place now to have it).
I will also update D77544 if this patch goes upstream.

Ping

Rebase + added lit test

LGTM

Ping

Rebase + Suggestions.

Fixed the testcase.

Ping

Incorporated the suggestions.

Divided the VGPRs into equal number of CSRs and scratch registers. Also, added a test case for VGPR tuple allocation.

Incorporated the suggestions.

Added a small description of the test.

In D76356#1942097, @rampitec wrote:

In D76356#1942070, @arsenm wrote:

In D76356#1939683, @rampitec wrote:

In D76356#1939646, @arsenm wrote:

In D76356#1939580, @rampitec wrote:

In D76356#1939493, @cdevadas wrote:

Thank you all for the comments.
I can see that there are concerns with the current split boundary (4 VGPRs together), considering the fact that we have wide VGPR uses in certain scenarios (image instructions).
But, like Matt mentioned, how frequently such scenarios occur?
Changing the split boundary to a large value would probably take away the whole purpose of this patch - reduce the CSR spills & try to ensure a better occupancy.

What will happen if you need to pass VReg_1024 into a function? It might not be a common case, but will it even work?

It works, but that isn't changed by this patch. This is not changing the argument registers which are all still in v0-v31. I believe the largest argument type we pass in registers is <8 x i32>, and force <32 x i32> to be stack passed anyway

OK. What if such a register needs to be preserved by a caller? I guess there is no safe window for it, so it will be forced to spill. Then we will spill a whole huge register, not just a part of it, because we do not use sublane spilling (except to AGPRs), right?

Yes, that's what I would expect

Then probably interleave 4 is not a best choice. We may also need to adjust cost of tuples to make aligned allocation more likely.

A cost for VGPR registers has been handled with https://reviews.llvm.org/D76417. This will ensure a balanced allocation of scratch registers & CSRs at every split (once the current patch is in the upstream).

Thank you all for the comments.
I can see that there are concerns with the current split boundary (4 VGPRs together), considering the fact that we have wide VGPR uses in certain scenarios (image instructions).
But, like Matt mentioned, how frequently such scenarios occur?
Changing the split boundary to a large value would probably take away the whole purpose of this patch - reduce the CSR spills & try to ensure a better occupancy.

Abandoning this review.
This optimization should be handled late after Basic Block Placement. A new review will be opened by handling it in a late pass.

Added a brief description of the register split (AMDGPUUsage.rst).

rebase

Reverted the changes made on the CostPerUse value.
It will go in a follow-up commit after this review.

incorporated the suggestion.