Thanks for the review - made all the suggested changes

In D54301#1296006, @scott.linder wrote:

I don't quite understand what distinguishes the two versions of MCSubtargetInfo (getSTI() vs STM), but it appears a patch from Konstantin earlier this year changed this specific instance from STM.getFeatureBits() to getSTI(). Changing this back LGTM but I don't know what the rationale was in the first place, if it wasn't a typo, so I don't want to sign off without asking.

Scott - you made some changes here most recently so adding you as the reviewer.

Added Neil as a reviewer as I've made some changes to some of his a16 tests. I'm pretty certain that the modifications are correct, but wanted to get feedback on that as well.

Modified based on review feedback from Nicolai

This patch has been superseded by D49097 - so I'm abandoning this one.

Minor changes made.

Made minor code changes suggested in review

@arsenm Matt, any more comments? Would you be happy with a clarification comment as per the last suggestion from me?

@arsenm Matt - good to go?

Covered all the requested changes (I think). Also implemented a test to make sure that simplifyDemanded doesn't run when TFE/LWE is enabled.

Changed the implementation of the intrinsic return type to be an aggregate type

In D51925#1233093, @arsenm wrote:

In D51925#1230429, @dstuttard wrote:

Looking again at the code - you're correct that it attempts to only do this transformation if the high bits are zero.
However, the code that checks this has the following telling comment:

// (i32 zext (i16 (bitcast f16:$src))) -> fp16_zext $src
// FIXME: It is not universally true that the high bits are zeroed on gfx9.
if (Src.getOpcode() == ISD::BITCAST) {
  SDValue BCSrc = Src.getOperand(0);
  if (BCSrc.getValueType() == MVT::f16 &&
      fp16SrcZerosHighBits(BCSrc.getOpcode()))
    return DCI.DAG.getNode(AMDGPUISD::FP16_ZEXT, SDLoc(N), VT, BCSrc);
}

In this particular case the BCSrc operation was an fptrunc which passes the fp16SrcZerosHighBits test - but that eventually ends up as v_mad_mixlo_f16 which doesn't ensure that the high bits are zero.

Any suggestions on how to proceed? I agree that it seems a shame to have to insert the extra AND operation blindly.

I guess you could check the subtarget in fp16SrcZerosHighBits. However that's pretty risky since it's depending on things we can't guarantee. Something could transform any other instruction into something else that won't preserve this. Overall I'm very unhappy this hardware change happened and it's a lot of work to handle all of this properly. I think what we really need is to drop this combine/node, and a separate machine instruction for every operation that preserves the high bits (with a tied source operand) vs. zeros them, and then have a machine pass that tries to clean up the extra ands while dropping this combine. We'll have to do extra work because we will have missed out on combines that this was enabling.

In D51932#1236779, @arsenm wrote:

I don't actually understand why this code is where it is? Why is SIFixSGPRCopies doing this? To clarify is this just an optimization? My initial reaction was that it was a fix, but looking at it again it seems like an optimization to me

Updated a mov to a copy as per review comment

Moved foldToImm into SIInstrInfo as suggested
Implemented check in verifyInstruction and checked that it worked when the fix was removed

Added missing break

De-duplicated 1/2PI constant

Made suggested change

Made suggested changes

Folded in most of the changes highlighted in the review

Looking again at the code - you're correct that it attempts to only do this transformation if the high bits are zero.
However, the code that checks this has the following telling comment:

LGTM - but you might want further reviews from others not so involved in implementation.

@nhaehnle - just added you as reviewer at the moment.

D48555 supersedes this change.

Yes, this change looks better based on what you say about not requiring live-in's for undef sub-ranges.

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