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

[Wei Mi via llvm-commits]

Hal is right. eliminateDeadDefs are not only used to eliminate dead
instructions after rematerialization, but also for regcoalescing and
maybe others. I think it is better to remove the assertion. Actually,
the code to compute isOrigDef allows multiple defs instructions. It
only speculate the first def reg is the vreg that can be
rematerialized.  For instructions having multiple defs, we will at
most lose some rematerialization opportunity in very rare cases but
not have correctness issue.

Thanks,
Wei.



On Mon, Dec 12, 2016 at 11:13 AM, Geoff Berry <gberry at codeaurora.org> wrote:
>
>
> 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.
>