[PATCH] [MachineSinking] Conservatively clear kill flags after coalescing.

Hi Quentin,

Jonas looked further into the problem below, and asked me to submit his patch. Note the we have our own out-of-tree target, and we have not been able to reproduce this problem on an in-tree target. /Patrik Hägglund

[MachineSinking] Conservatively clear kill flags after coalescing.

This solves the problem of having a kill flag inside a loop

with a definition of the register prior to the loop:

%vreg368 …

Inside loop:

%vreg520 = COPY %vreg368

%vreg568<def,tied1> = add %vreg341, %vreg520

=> was coalesced into =>

%vreg568<def,tied1> = add %vreg341, %vreg368

MachineVerifier then complained:

*** Bad machine code: Virtual register killed in block, but needed live out. ***

The kill flag for %vreg368 is incorrect, and is cleared by this patch.

This is similar to the clearing done at the end of

MachineSinking::SinkInstruction().

0001-MachineSinking-Conservatively-clear-kill-flags-after.patch (1.48 KB)

Hi Patrik,

LGTM.

Thanks,
-Quentin

clearKillFlags seems a little "overkill" to me. In this case you could just simply transfer the value of the kill flag from the SrcReg to the DstReg.

-Juergen

clearKillFlags seems a little "overkill" to me. In this case you could just simply transfer the value of the kill flag from the SrcReg to the DstReg.

We are extending the live-range of SrcReg. I do not see how you could relate that to the kill flag of DstReg.

Therefore, I still think, this is the right fix.

-Quentin

clearKillFlags seems a little "overkill" to me. In this case you could just simply transfer the value of the kill flag from the SrcReg to the DstReg.

We are extending the live-range of SrcReg. I do not see how you could relate that to the kill flag of DstReg.

I mean, we *may* extend, but we do not really know.

When the SrcReg has the kill flag set, we don't want to propagate that and just keeping the original DstReg kill flag at its last use. Basically there is no work to be done when SrcReg has the kill flag set.
When the kill flag is not set we have to first clear the kill flag from all DstReg uses and then perform the replacement with the SrcReg. If we would do the clearing of the kill flag afterwards we would clear more kill flags than necessary and perform more work too.

if (!MI->getOperand(1).isKill())
MRI->clearKillFlags(DstReg);
MRI->replaceRegWith(DstReg, SrcReg);

DstReg<def> = COPY SrcReg<kill> DstReg<def> = COPY SrcReg

... = ... DstReg ... = ... DstReg
... = ... DstReg<kill> ... = ... DstReg<kill>
... = ... SrcReg<kill>

----->

... = ... SrcReg ... = ... SrcReg
... = ... SrcReg<kill> ... = ... SrcReg
... = ... SrcReg<kill> <-- this would get cleared too if we clear the kill flags for SrcReg.

-Juergen

When the SrcReg has the kill flag set, we don't want to propagate that and just keeping the original DstReg kill flag at its last use. Basically there is no work to be done when SrcReg has the kill flag set.
When the kill flag is not set we have to first clear the kill flag from all DstReg uses and then perform the replacement with the SrcReg. If we would do the clearing of the kill flag afterwards we would clear more kill flags than necessary and perform more work too.

if (!MI->getOperand(1).isKill())
  MRI->clearKillFlags(DstReg);
MRI->replaceRegWith(DstReg, SrcReg);

DstReg<def> = COPY SrcReg<kill> DstReg<def> = COPY SrcReg

... = ... DstReg ... = ... DstReg
... = ... DstReg<kill> ... = ... DstReg<kill>
                                    ... = ... SrcReg<kill>

----->

... = ... SrcReg ... = ... SrcReg
... = ... SrcReg<kill> ... = ... SrcReg
                                    ... = ... SrcReg<kill> <-- this would get cleared too if we clear the kill flags for SrcReg.

Yes, but if you consider a different example where SrcReg is killed before DstReg, then it is wrong:

DstReg = COPY SrcReg

… = … SrcReg<kill>

… = … DstReg<kill or not>

——>

… = … SrcReg<kill> <— this isn’t cleared whereas it should.

… = … SrcReg<kill or not>

We could indeed do some tricks to preserve the kill flags, but in my opinion, this is error prone and is not worth the risk.

Cheers,
-Quentin

You are right. It isn't worth the risk and effort.

Cheers,
Juergen

Thanks! Committed as r217427.

/Patrik Hägglund