[llvm] r288189 - [LiveRangeEdit] Handle instructions with no defs correctly.
Geoff Berry via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 12 12:17:02 PST 2016
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