[llvm] r288189 - [LiveRangeEdit] Handle instructions with no defs correctly.
Wei Mi via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 12 13:17:59 PST 2016
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.
>
More information about the llvm-commits
mailing list