Fix further review comments by Stas.

Fix review comments by Stas and Matt.

Fix review comments (by Matt), by materializing custom lowering of
scalar_to_vector via build_vector since build_vector already specially
handles v8i16/v8f16 types.

Fix review comments.

This patch helped me to understand - what is going on with custom lowering of few vector operations, but, otherwise, the change within this patch itself for INSERT_SUBVECTOR does not any make sense. Hence abandoning it. We be coming up with update patch(es).

Fix minor comments by Matt and CD.

In D123973#3459961, @rampitec wrote:

It does not seem to move tests but rather dropping them.

Rebase to D123973 and update lit tests accordingly.

Within spill-vgpr.ll, remove 2>&1 from RUN line for gfx900.

Usage of AGPR in reject-agpr-usage-before-gfx908.ll should also fail for gfx906.

Rebase to latest trunk and to D123809.

Restore spill-vgpr-to-agpr.ll and spill-agpr.ll.

Move code related to shifting of reserved VGPR to a lower range within FrameLowering.

Fix assert message.

Minor update to member initialization.

Fix few review comments by CD.

In D123525#3446104, @rampitec wrote:

Let's begin with the question: what problem are you trying to solve?

Here is the update where we always reserve highest avaialble VGPR irrespective of the
register constraint.

Refine the logic within indirectCopyToAGPR() while chosing lowest available
VGPR for AGPR copy. Basically, find out the total avaialble unused free
VGPRs (after RegAlloc), and call scavenger (only) that many times to find
the lowest available unused free VGPR.

In D122367#3407538, @rampitec wrote:

What's the reason behind this? You are not even using it.

Fix review comments by Stas.

If I understand this patch correctly, the main intention of this patch is to check if the entry point function makes any call(s) to device functions, and if so, create and allocate module lds. Otherwise don't.

Fix review comments by Jay.

In D122286#3401911, @foad wrote:

Can you simplify the patch? I think all it is doing is (a) adding a new test and (b) simplifying the "if" on line 609 because the condition is always true. All the other changes make it harder to see what the patch is really doing.

hasGFX90AInsts() is true for both GFX90A and GFX940 subtargets.

Rebase and remove newly added confusing TODO comments.

Fix further review comments by Matt.

Fix review comments by Matt.

LGTM.

In D110257#3133934, @JonChesterfield wrote:

In D110257#3133895, @hsmhsm wrote:

In D110257#3133879, @JonChesterfield wrote:

In D110257#3133866, @hsmhsm wrote:

This is not something specific to AMDGPU backend, but AMDGPU backend at present requires this canonical form.

Undocumented and not checked by the IR verifier. Canonical form seems to be overstating it until at least one of those is addressed.

We already discussed that this canonical form is not something that IR verifier can verify, but it is good enough for better code transformation/optimization. Please refer llvm-dev email discussion w.r.t it.

If the new invariant is that all alloca must be adjacent to one another, that's a trivial thing for the verifier to check. So I guess it's something else? Please write down what this new invariant is intended to be, preferably in the documentation, perhaps of the alloca instruction.

In D109870#3133898, @JonChesterfield wrote:

I personally think that you just want to avoid this patch from getting approved, though I do not know the real reason

I'm mistrusting of this patch because it has been landed repeatedly and broke amdgpu openmp every time. That's the 'real reason', every time you land this is breaks openmp and wastes a bunch of my time picking up the pieces.

In D110257#3133879, @JonChesterfield wrote:

In D110257#3133866, @hsmhsm wrote:

This is not something specific to AMDGPU backend, but AMDGPU backend at present requires this canonical form.

Undocumented and not checked by the IR verifier. Canonical form seems to be overstating it until at least one of those is addressed.

I do not think this patch makes any progress. Enough is enough, I am abandoning it.

In D109870#3133844, @JonChesterfield wrote:

Hi @JonChesterfield and @jdoerfert, you both had rejected this patch initially asking for the changes. Your review and acceptance of this patch is must, otherwise this patch will not land even if some other reviewer accept it. So, kind request to take a look at this patch.

I haven't seen someone remove reviewers from their patch when said reviewers marked it 'requested changes' before. Can you indicate why you did so? Reinstating my requested changes flag in the mean time.

In D110257#3133838, @JonChesterfield wrote:

Please change the commit message to say why this change is necessary / an improvement on what we have now.

My recollection is that the amdgpu backend crashes on some IR and this decreases the probability of that IR pattern occuring, which still sounds like fixing the wrong place to me. Was this the one where hoisting static size alloca into the entry block near the backend would the problem?

I think this patch is missing a documentation update adding the new constraint that allocas must be contiguous in IR. That would help to answer questions about which alloca must be contiguous and which can occur separated by instructions, as currently none of them need to be adjacent. Also, is this only intended to constrain the entry basic block?

Rebase.

Minor changes to comment.

Hi @arsenm, The pre-requisite front-end patch (https://reviews.llvm.org/D110257) has landed to llvm trunk. Now, this patch needs an action to be taken. Please have a look at this patch during your free time.

Rebase.

Thanks @rnk and @yaxunl .

LGTM

This patch is waiting for an action for a long time. I am expecting at-least anyone of the reviewers to either say "yes" or "no" or "further required improvements" on this patch so that I can take further action on this patch. If you say "no" to this patch with a strong valid reason, then I will abandon this patch and move-on instead of uncertainly waiting.

Rebase.