Removed test as being overkill for the missed state clear being added (2 other
tests still remain)

In D45372#1068792, @alex-t wrote:

In D45372#1068728, @dstuttard wrote:

2. Remove the test altogether since the VirtReg2Value.clear() is definitely required and was originally missed as an oversight - and the test might be overkill.

Sounds reasonable. It really was an oversight :) It's funny that the change was reviewed by lots of people for 2 months but nobody noticed not clearing state.

In D45372#1062947, @alex-t wrote:

In general LGTM.
I also concerned about the magic test that has no checks and has no visible side effects on shared data but is necessary to reproduce buggy behavior.
Could you please clarify what do you need it for?

Folded in triple to run line

LGTM

Any comment on this change? If not I'll land in the next few days.

Any comment on this change? If not I'll land in the next few days.

Any more comments on this change? If not I'll land this in the next few days.

Removing a debug llvm_unreachable

A better fix for this problem.

The intent of this code is really to allow anyone doing debug to insert a check anywhere in the code. In particular, for the problems I was looking at I needed to check the live intervals after each change made in order to pinpoint where an error was being introduced. As implemented here (with the check after coalescing) it looks redundant, so having that extra check is probably useless.
Running a verification phase after coalescing is sometimes too late, and indeed may well have asserted before this point.

Removing debug code

I'm going to take another look at this bug.
I've also uploaded a couple of other fixes for SubRange join failures. See D40300 and D40308. I've also uploaded a verifier method that is helpful in tracking these problems down. See D40297.

Removed a blank line causing unnecessary differences

I've got a couple of new SubRange join fixes that I'm about to upload for review. This routine was useful in tracking those down so I thought it would be useful to add as a generic aid for future problems.

In D35241#891440, @RKSimon wrote:

@dstuttard Are you ready to commit this?

Test now reduced further - thanks @RKSimon - confirmed that it still exhibits the problem and that the fix still fixes it.
I managed to remove a couple more lines from the cut-down version you provided.

Cutting down the test size and rebasing

In D35241#871886, @RKSimon wrote:

@dstuttard Are you still looking at this?

In D35241#827823, @arsenm wrote:

Can you reduce the test any further?

Agreed - @cwabbott your changes provide something much more complete than this change, plus remove the need for it. Thanks.

In D34889#815492, @MatzeB wrote:

Testcase? It seems that your change wants to allow the following:

vreg4:sub0<def, dead> = ...
...
  = use vreg4:sub1

This is not legal! The dead modifier is about the full vreg not just about the sub0 part defined. (-verify-machineinstrs should also lead to such inputs getting rejected).

Implemented a slightly different approach to resolving the issue.

In D35073#815414, @MatzeB wrote:

I'm confused by this change (and the testcase doesn't really help understanding what is going on). You are probably onto a real bug here where we cannot assume the same lanemasks work for the input/output register class of a copy. However that should be independent of the fact that a full or partial copy is present.

ping

ping

@arsenm - any comments?

It's already bugpoint reduced - I tried trimming it further by hand but it hid the issue (unsurprisingly).

Not sure who is an appropriate reviewer for this, adding @RKSimon as he seems to have made several changes in the same area.

Adding verify-machineinstr flag to llc invocation in the test

I do have a reproducer for this (a .ll test). I've attempted to turn this into a .mir test, but it is too fragile to allow me to change the target specific intrinsics to something generic (required in order to print out the .mir).
Hopefully, the check is an obvious omission and an easy one to approve?
If not I'll see if I can create a smaller regression for it - but I've already spent quite a lot of time attempting to do this.

Updating the test to a .mir test
Replacing the buffer.load intrinsics with load volatile worked

@qcolombet - any further comments or are you happy for this to go in?

In D34391#791480, @MatzeB wrote:

The code fix LGTM, but please wait for @qcolombet/@arsenm before committing.

Making a .mir test seems indeed hard at the moment, as the printer already fails; manually stripping memory operands only works after printing I presume?

I think we will require a more extensive change to properly support WQM and all it can do. However, this is a minor extension to an existing intrinsic that allows implementation of functions required in some shader implementations.
This can be removed if necessary if more fully featured support is implemented.

Updating in line with comments from reviewers

Added in mising check on RmValNo for null

In D34391#786792, @npjdesres wrote:

FYI I tried this patch in my out-of-tree backend (hoping to resolve http://llvm.org/PR32773). I observed a segfault SR.removeValNo(RmValNo) because RmValNo may be null.

I don't know yet whether this is specific to my backend, but I thought I'd mention it in case it indicates a more general problem.

Updating the test as per review comments

Inadvertently included extra changes in last diff

Corrected 80 column formatting and variable names

Removed the commented out debug statement

Removing debug comment
Also adding the test case from the bugzilla as a new test case (I realised it is
a bit annoying to have to go elsewhere to get hold of the reproducer)
I'll update it as a test if this change looks promising

In D34391#785251, @meadori wrote:

Test case?

I've added this change specifically to address a problem seen in the associated PR (http://llvm.org/PR33524), however I'm not sure that this is necessarily the right way to go about fixing this issue.

Adding MatzeB as reviewer - you've made some recent changes in the same area.

Added llvm-commits

Removed the bulk of the legacy implementation and now lower to the new form earlier

Made suggested changes prior to submission

Made changes to comments
Changed address spaces in tests and added some checks

See responses to your individual comments - it could be that I've missed something in the way this could be done.

Forgot to update comment about float as an option for overloading

Data can now be float or int
I decided that since the load variant would support this, so should the store. This meant re-jigging
the implementation slightly to use an approach more similar to load to enable the use of any for the
store as well (unless you can see a more efficient way to do it).

Any chance of another review of this change?
Does it look good to go now?
Thanks

Any chance of a final review on this change?

ping

ping

As suggested by Tom Stellard (due to potential issues with range checking) , I've changed the
intrinsics to have explicit operands for vindex, voffset, soffset and offset. The backend no longer
attempts to optimise by folding in any offsets it can spot in preceding instructions.