ping

Matt - are you now happy for me to submit this? (It is tagged as approved, but since you've made some extra comments I'm waiting for you to agree with the latest changes).

Made suggested changes - is this more in line with what you were thinking Matt?

Updates in light of review comments

Made some changes based on review

GFX10 support has gone in since this change was approved - gfx10 allows 2 sgprs
on the constant bus. Implementation updated to allow for this.

Managed to get the fmac test to keep using fmac
Also updated the test to use non-anonymous values

Changed test to use fma intrinsic

+Stas to comment on the v_fmac_f16 test change.
Is it acceptable to change the result to look for v_pk_fma_f16 rather than 2 v_fmac_f16 instructions? If not, any suggestions on how to get the compiler to generate 2 x fmac instead?

I might revisit this one - setting cumode seems messy to enable driver control of the WGP setting, but seems the most pragmatic at the moment.

ping

LGTM

ping

Modified test in line with review comments

Not really an area I'm 100% sure about - but looks ok to me. One of the other reviewers will have to sign off too.
Minor niggle on the comment (if my understanding is correct).

LGTM

ping

ping

LGTM - but probably need approval from one of the other reviewers as well

In D51925#1309875, @arsenm wrote:

In D51925#1309692, @dstuttard wrote:

ping

What happens if you just drop the optimization entirely?

ping

ping

ping

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