[PATCH] D20907: [IfConversion] Bugfix: don't add Undef flag on use if reg is live.

[Quentin Colombet via llvm-commits]

Hi Jonas,

Let us step back a little bit. I actually did not read any code when commenting the first time, I was clarifying the intended behavior of the undef flag wrt liveness :).
Now, I have read the relevant part of IfConversion and I can give more appropriate guidance.

> On Jun 21, 2016, at 1:15 AM, Jonas Paulsson <paulsson at linux.vnet.ibm.com> wrote:
> 
> jonpa added a comment.
> 
> Quentin:
> LivePhysRegs was not assuming that an undef use means the register is dead, it rather misses the fact that the register was indeed live due to the (erroneous) undef flag.
> 
> Looking at the example code: in order for LivePhysRegs to discover that %R4L is live, there must be a use of %R4L on the third instruction (using stepBackward()). That can't be an undef use, since that does not signal liveness. Therfore, it must be a use op without the undef flag, which is exactly what the patch achieves.

At this point, two questions remain:
A. IfConversion is adding undef flag whereas it should not?
B. LivePhysReg does not interpret undef properly?


Taken separately, LivePhysReg does the right thing with what it sees, i.e., the register is live only if it will be used later on and has been redefined, and the undef flag on LOCR is appropriate because you do not want to depend on the input of that register while telling the liveness that the value is live.

However, taken together, the problem is that LivePhysReg does not know that the definition of R4L is predicated and the undef flag use indeed does not help liveness.

That is pretty much your analysis, but rephrased :).

> I am a bit confused about the meaning/purpose of such implicit undef-use operands? Why add the implicit operand in UpdatePredRedefs() if it is undef? Instruction order would be maintained without it due to the register definitions, right? And it does not help liveness tracking.


The implicit use is here to keep the liveness going up to the redefinition, as you figured out, and I believe the undef was added as well to avoid false dependencies between two unrelated instructions. Like we saw this does not work because like you said, this does not help the liveness.

Given that we want the liveness to remain out of the business of the predication, I believe the proper fix is to set implicit use for all live variables. I.e., move your test on the setting of the implicit use, not in the undef flag.
Cheers,
-Quentin


> 
> Krzysztof:
> My concern for using tied operands was merely about having a better model of the instruction which could be verified. I am thinking that for a predicated instruction, the input operand *must* be present and *must not* be undef if reg is live, as it is the value of the instruction if it becomes a noop. Othwerwise, the liveness is not properly modelled. Therefore, it would be preferred to model this operand explicitly, as a tied op. Just a thought...
> 
> 
> http://reviews.llvm.org/D20907
> 
> 
>