[llvm] r288189 - [LiveRangeEdit] Handle instructions with no defs correctly.

[Geoff Berry via llvm-commits]

On 12/7/2016 8:31 AM, Hal Finkel wrote:
> ----- Original Message -----
>> From: "Geoff Berry" <gberry at codeaurora.org>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: llvm-commits at lists.llvm.org, "Matthias Braun" <matze at braunis.de>
>> Sent: Monday, December 5, 2016 2:17:59 PM
>> Subject: Re: [llvm] r288189 - [LiveRangeEdit] Handle instructions with no defs correctly.
>>
>>
>>
>> On 12/2/2016 9:13 AM, Hal Finkel wrote:
>>> ----- Original Message -----
>>>> From: "Geoff Berry via llvm-commits" <llvm-commits at lists.llvm.org>
>>>> To: llvm-commits at lists.llvm.org
>>>> Sent: Tuesday, November 29, 2016 1:31:35 PM
>>>> Subject: [llvm] r288189 - [LiveRangeEdit] Handle instructions with
>>>> no defs correctly.
>>>>
>>>> Author: gberry
>>>> Date: Tue Nov 29 13:31:35 2016
>>>> New Revision: 288189
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=288189&view=rev
>>>> Log:
>>>> [LiveRangeEdit] Handle instructions with no defs correctly.
>>>>
>>>> Summary:
>>>> The code in LiveRangeEdit::eliminateDeadDef() that computes
>>>> isOrigDef
>>>> doesn't handle instructions in which operand 0 is not a def (e.g.
>>>> KILL)
>>>> correctly.  Add a check that operand 0 is a def before doing the
>>>> rest
>>>> of
>>>> the isOrigDef computation.
>>>>
>>>> Reviewers: qcolombet, MatzeB, wmi
>>>>
>>>> Subscribers: mcrosier, llvm-commits
>>>>
>>>> Differential Revision: https://reviews.llvm.org/D27174
>>>>
>>>> Modified:
>>>>       llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp?rev=288189&r1=288188&r2=288189&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp Tue Nov 29 13:31:35
>>>> 2016
>>>> @@ -272,7 +272,8 @@ void LiveRangeEdit::eliminateDeadDef(Mac
>>>>      bool ReadsPhysRegs = false;
>>>>      bool isOrigDef = false;
>>>>      unsigned Dest;
>>>> -  if (VRM && MI->getOperand(0).isReg()) {
>>>> +  if (VRM && MI->getOperand(0).isReg() &&
>>>> MI->getOperand(0).isDef())
>>>> {
>>>> +    assert(MI->getDesc().getNumDefs() == 1);
>>> I see in the review thread that the issue of instructions that
>>> define multiple operands was discussed with the comment that, "we
>>> don't have to worry about multiple definition because
>>> isReallyTriviallyReMaterializableGeneric filter out multidef
>>> instructions." Could you please add a comment here explaining
>>> that, because I don't think it is obvious.
> I think that the comment is fine, but then I don't understand the review discussion about isReallyTriviallyReMaterializableGeneric. eliminateDeadDef is called by eliminateDeadDefs which is called from a number of different places. Does isReallyTriviallyReMaterializableGeneric ensure this precondition indirectly for all of these callers?
>
> Also, FWIW, isReallyTriviallyReMaterializableGeneric does allow multiple defs in the special case where all defs are of the same register. Does this matter?

I agree, it isn't obvious to me that all the callers of 
eliminateDeadDefs are fulfilling the expectation of not passing in 
instructions with multiple defs.  It could be that something subtle with 
this only being true if we have a non-null VRM (since that is checked in 
the eliinateDeadDef if statement), but these are probably better 
questions for Wei Mi since I believe he is the original author.

> Thanks again,
> Hal
>
>> Sure, how about something like this:
>>
>> diff --git a/lib/CodeGen/LiveRangeEdit.cpp
>> b/lib/CodeGen/LiveRangeEdit.cpp
>> index 88fed62..918be47 100644
>> --- a/lib/CodeGen/LiveRangeEdit.cpp
>> +++ b/lib/CodeGen/LiveRangeEdit.cpp
>> @@ -273,7 +273,9 @@ void LiveRangeEdit::eliminateDeadDef(MachineInstr
>> *MI, ToShrinkSet &ToShrink,
>>      bool isOrigDef = false;
>>      unsigned Dest;
>>      if (VRM && MI->getOperand(0).isReg() &&
>>      MI->getOperand(0).isDef()) {
>> -    assert(MI->getDesc().getNumDefs() == 1);
>> +    // It is assumed that callers of eliminateDeadDefs() will never
>> pass in dead
>> +    // instructions with multiple virtual register defs.
>> +    assert(MI->getDesc().getNumDefs() == 1 && "Unexpected
>> instruction
>> with multiple defs.");
>>        Dest = MI->getOperand(0).getReg();
>>        unsigned Original = VRM->getOriginal(Dest);
>>        LiveInterval &OrigLI = LIS.getInterval(Original);
>>
>>
>>> Thanks again,
>>> Hal
>>>
>>>>        Dest = MI->getOperand(0).getReg();
>>>>        unsigned Original = VRM->getOriginal(Dest);
>>>>        LiveInterval &OrigLI = LIS.getInterval(Original);
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>> --
>> Geoff Berry
>> Employee of Qualcomm Datacenter Technologies, Inc.
>>    Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
>>    Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
>>    Code Aurora Forum, a Linux Foundation Collaborative Project.
>>
>>

-- 
Geoff Berry
Employee of Qualcomm Datacenter Technologies, Inc.
  Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.