In D36224#1243458, @bjope wrote:

In D36224#1243333, @arsenm wrote:

The tests may pass, but is the output exactly the same? I think this should break the test in the listed example. There can't be a copy in the case it's trying to fix

Sorry, I don't think I understand what you mean. Are you talking about the example in the summary, or the added mir-test?

I think you need to split this into a separate loop before the propagate attributes function instead of adding the recursive call at the same time. This is different from the other attributes because it is inferred top down. You should have a first loop over the CallGraphSCC that adds this. Since the CallGraphSCC should have all of the nodes reachable from each other, this should be some set building / checks from there. You shouldn't need to be looking at the instructions inside the functions and looking for specific call sites

LGTM

Also needs to support/test sub. I’m also concerned it’s not this simple with i1 to use a scalar operation. This needs some tests stressing SIFixSGPRCopies, and with control flow

In D52410#1243396, @uabelho wrote:

In D52410#1243309, @arsenm wrote:

An instruction just needs to def one or the other and the live out would be vcc, since it needs to visit the super register

Hm, you think any of the cases in test/CodeGen/AMDGPU/fold-immediate-operand-shrink.mir could be copied and modifed
into exposing it then? Any idea which one would be most appropriate?

The tests may pass, but is the output exactly the same? I think this should break the test in the listed example. There can't be a copy in the case it's trying to fix

r342879

In D52060#1241470, @mareko wrote:

In D52060#1234370, @arsenm wrote:

Should the instcombine part change also to allow creation of i1 uses?

What do you mean by that? I'm not sure what you mean.

An instruction just needs to def one or the other and the live out would be vcc, since it needs to visit the super register

r342878

r342858

r342857

Needs a comment that it’s a workaround for DA

r342443

LGTM

r342442

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

In D51742#1226311, @scott.linder wrote:

The regression was caught by the expensive_checks build, if that is sufficient for testing? I am actually not sure how to write a lit test for this, because opt -analyze will just re-calculate the DT; I don't know how to just print the DT after the legalize pass runs.

Should the instcombine part change also to allow creation of i1 uses?

Considering emitting traps requires fixing traps first, otherwise a program that should work will incorrectly trap

r342133

r342132

Should include tests running only the IR pass

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.

Should have a special check in the verifier

Needs some edge case tests where the values are uniform but still need to be in a VGPR

r341895

r341894

Remove commented out code

r341806

r341803

r341801

In D51737#1226081, @RKSimon wrote:

Handling INSERT_SUBVECTOR might make sense as well?

r341770