[llvm] r288189 - [LiveRangeEdit] Handle instructions with no defs correctly.
Geoff Berry via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 13 07:46:26 PST 2016
That seems reasonable to me. I'll make this change after a bit to give
others a chance to weigh in.
On 12/12/2016 4:17 PM, Wei Mi wrote:
> Right, it will leave some dead defs in the IR which is not good.
>
> Then I think may be better to change
> VRM && MI->getOperand(0).isReg() && MI->getOperand(0).isDef()
> to
> VRM && MI->getOperand(0).isReg() && MI->getOperand(0).isDef() &&
> MI->getDesc().getNumDefs() == 1
>
> That is to say, for multiple defs instruction, we don't bother to try
> keeping them around for rematerialization after they are dead. I think
> we should only miss rematerialization in some rare cases this way,
> since only those multiple defs instruction with the same def register
> is allowed in isReallyTriviallyReMaterializableGeneric.
>
> Do you think it is ok?
>
> Thanks,
> Wei.
>
> On Mon, Dec 12, 2016 at 12:17 PM, Geoff Berry <gberry at codeaurora.org> wrote:
>> Does the 'isOrigDef' code in eliminateDeadDef handle multiple defs correctly
>> though? It looks to me like it might cause problems since only operand 0 is
>> replaced in the original instruction in this case.
>>
>>
>>
>> On 12/12/2016 2:52 PM, Wei Mi wrote:
>>> 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.
>>>>
>> --
>> 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.
More information about the llvm-commits
mailing list