[llvm] r288189 - [LiveRangeEdit] Handle instructions with no defs correctly.
Geoff Berry via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 15 12:06:15 PST 2016
Committed in r289861
On 12/13/2016 10:46 AM, Geoff Berry via llvm-commits wrote:
> 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