[llvm] r283838 - [RegAllocGreedy] Attempt to split unspillable live intervals
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 2 12:56:59 PST 2017
Fixed in r293934
> On Feb 1, 2017, at 11:24 PM, Mikael Holmén <mikael.holmen at ericsson.com> wrote:
>
> Hi,
>
> On 02/01/2017 11:03 PM, Quentin Colombet wrote:
>> Hi Mikael,
>>
>> Nice catch!
>
> We have a bunch of test cases originally written to test memcpy/memset of different sizes in different situations and they have proved to be quite efficient of triggering all kinds of weird bugs (totally non-related to memcpy/memset) since they put quite some stress both on the register allocator and the scheduler. Especially when we compile them with random compiler flags. This was such a case :)
>
>>
>>> On Jan 31, 2017, at 11:33 PM, Mikael Holmén
>>> <mikael.holmen at ericsson.com <mailto:mikael.holmen at ericsson.com>> wrote:
>>>
>>> Hi Dylan and Quentin,
>>>
>>> I think I've stumbled upon a problem with this patch. See below.
>>>
>>> On 10/11/2016 03:04 AM, Dylan McKay via llvm-commits wrote:
>>>> Author: dylanmckay
>>>> Date: Mon Oct 10 20:04:36 2016
>>>> New Revision: 283838
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=283838&view=rev
>>>> Log:
>>>> [RegAllocGreedy] Attempt to split unspillable live intervals
>>>>
>>>> Summary:
>>>> Previously, when allocating unspillable live ranges, we would never
>>>> attempt to split. We would always bail out and try last ditch graph
>>>> recoloring.
>>>>
>>>> This patch changes this by attempting to split all live intervals before
>>>> performing recoloring.
>>>>
>>>> This fixes LLVM bug PR14879.
>>>>
>>>> I can't add test cases for any backends other than AVR because none of
>>>> them have small enough register classes to trigger the bug.
>>>>
>>>> Reviewers: qcolombet
>>>>
>>>> Subscribers: MatzeB
>>>>
>>>> Differential Revision: https://reviews.llvm.org/D25070
>>>>
>>>> Added:
>>>> llvm/trunk/test/CodeGen/AVR/high-pressure-on-ptrregs.ll
>>>> Modified:
>>>> llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp
>>>> llvm/trunk/lib/CodeGen/RegAllocGreedy.cpp
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp?rev=283838&r1=283837&r2=283838&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp Mon Oct 10 20:04:36 2016
>>>> @@ -438,6 +438,9 @@ LiveRangeEdit::MRI_NoteNewVirtualRegiste
>>>> if (VRM)
>>>> VRM->grow();
>>>>
>>>> + if (Parent && !Parent->isSpillable())
>>>> + LIS.getInterval(VReg).markNotSpillable();
>>>> +
>>>
>>> I have a case where the above code actually triggers, but
>>> unfortunately it leads to a failed assertion slightly after.
>>>
>>> We have:
>>>
>>> LiveInterval &LiveRangeEdit::createEmptyIntervalFrom(unsigned OldReg) {
>>> unsigned VReg = MRI.createVirtualRegister(MRI.getRegClass(OldReg));
>>>
>>> HEre MachineRegisterInfo::createVirtualRegister is called, in turn doing
>>>
>>> TheDelegate->MRI_NoteNewVirtualRegister(Reg);
>>>
>>> which triggers the code added in the patch above.
>>>
>>> The new VReg doesn't have an interval yet, so
>>>
>>> LIS.getInterval(VReg)
>>>
>>> will actually create one (and compute it)!
>>
>> We don’t want that.
>>
>>>
>>> However, when we return to LiveRangeEdit::createEmptyIntervalFrom we
>>> continue and do
>>>
>>> if (VRM) {
>>> VRM->setIsSplitFromReg(VReg, VRM->getOriginal(OldReg));
>>> }
>>> LiveInterval &LI = LIS.createEmptyInterval(VReg);
>>>
>>> And here an assertion blows since VReg already has an interval, namely
>>> the one created just recently from MRI_NoteNewVirtualRegister.
>>>
>>> Should we check if the new VReg already has an interval before
>>> creating it in createEmptyIntervalFrom or should we do something
>>> smarter in MRI_NoteNewVirtualRegister or what?
>>
>> I would lean toward changing the create.*Form method in LRE to do the
>> marking here. We shouldn’t mess up with the interval in
>> MRI_NoteNewVirtualRegister. Though I'm not super happy with that
>> solution either. (Slightly coupled solution.)
>>
>> Patch attached for convenience.
>>
>> Let me know if that works for you.
>
> Yep, that works. The crashing test passes and I haven't seen anything else breaking so far either. :)
>
> Thanks,
> Mikael
>
>>
>> Cheers,
>> -Quentin
>>
>>>
>>> Found this on my out-of-tree target.
>>>
>>> Regards,
>>> Mikael
>>>
>>>> NewRegs.push_back(VReg);
>>>> }
>>>>
>>>>
>>>> Modified: llvm/trunk/lib/CodeGen/RegAllocGreedy.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocGreedy.cpp?rev=283838&r1=283837&r2=283838&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/RegAllocGreedy.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/RegAllocGreedy.cpp Mon Oct 10 20:04:36 2016
>>>> @@ -2553,18 +2553,20 @@ unsigned RAGreedy::selectOrSplitImpl(Liv
>>>> return 0;
>>>> }
>>>>
>>>> + if (Stage < RS_Spill) {
>>>> + // Try splitting VirtReg or interferences.
>>>> + unsigned NewVRegSizeBefore = NewVRegs.size();
>>>> + unsigned PhysReg = trySplit(VirtReg, Order, NewVRegs);
>>>> + if (PhysReg || (NewVRegs.size() - NewVRegSizeBefore))
>>>> + return PhysReg;
>>>> + }
>>>> +
>>>> // If we couldn't allocate a register from spilling, there is
>>>> probably some
>>>> // invalid inline assembly. The base class wil report it.
>>>> if (Stage >= RS_Done || !VirtReg.isSpillable())
>>>> return tryLastChanceRecoloring(VirtReg, Order, NewVRegs,
>>>> FixedRegisters,
>>>> Depth);
>>>>
>>>> - // Try splitting VirtReg or interferences.
>>>> - unsigned NewVRegSizeBefore = NewVRegs.size();
>>>> - unsigned PhysReg = trySplit(VirtReg, Order, NewVRegs);
>>>> - if (PhysReg || (NewVRegs.size() - NewVRegSizeBefore))
>>>> - return PhysReg;
>>>> -
>>>> // Finally spill VirtReg itself.
>>>> if (EnableDeferredSpilling && getStage(VirtReg) < RS_Memory) {
>>>> // TODO: This is experimental and in particular, we do not model
>>>>
>>>> Added: llvm/trunk/test/CodeGen/AVR/high-pressure-on-ptrregs.ll
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AVR/high-pressure-on-ptrregs.ll?rev=283838&view=auto
>>>> ==============================================================================
>>>> --- llvm/trunk/test/CodeGen/AVR/high-pressure-on-ptrregs.ll (added)
>>>> +++ llvm/trunk/test/CodeGen/AVR/high-pressure-on-ptrregs.ll Mon Oct
>>>> 10 20:04:36 2016
>>>> @@ -0,0 +1,78 @@
>>>> +; RUN: llc < %s -march=avr | FileCheck %s
>>>> +
>>>> +; This tests how LLVM handles IR which puts very high
>>>> +; presure on the PTRREGS class for the register allocator.
>>>> +;
>>>> +; This causes a problem because we only have one small register
>>>> +; class for loading and storing from pointers - 'PTRREGS'.
>>>> +; One of these registers is also used for the frame pointer, meaning
>>>> +; that we only ever have two registers available for these operations.
>>>> +;
>>>> +; There is an existing bug filed for this issue - PR14879.
>>>> +;
>>>> +; The specific failure:
>>>> +; LLVM ERROR: ran out of registers during register allocation
>>>> +;
>>>> +; It has been assembled from the following c code:
>>>> +;
>>>> +; struct ss
>>>> +; {
>>>> +; int a;
>>>> +; int b;
>>>> +; int c;
>>>> +; };
>>>> +;
>>>> +; void loop(struct ss *x, struct ss **y, int z)
>>>> +; {
>>>> +; int i;
>>>> +; for (i=0; i<z; ++i)
>>>> +; {
>>>> +; x->c += y[i]->b;
>>>> +; }
>>>> +; }
>>>> +
>>>> +%struct.ss = type { i16, i16, i16 }
>>>> +
>>>> +; CHECK-LABEL: loop
>>>> +define void @loop(%struct.ss* %x, %struct.ss** %y, i16 %z) #0 {
>>>> +entry:
>>>> + %x.addr = alloca %struct.ss*, align 2
>>>> + %y.addr = alloca %struct.ss**, align 2
>>>> + %z.addr = alloca i16, align 2
>>>> + %i = alloca i16, align 2
>>>> + store %struct.ss* %x, %struct.ss** %x.addr, align 2
>>>> + store %struct.ss** %y, %struct.ss*** %y.addr, align 2
>>>> + store i16 %z, i16* %z.addr, align 2
>>>> + store i16 0, i16* %i, align 2
>>>> + br label %for.cond
>>>> +
>>>> +for.cond: ; preds =
>>>> %for.inc, %entry
>>>> + %tmp = load i16, i16* %i, align 2
>>>> + %tmp1 = load i16, i16* %z.addr, align 2
>>>> + %cmp = icmp slt i16 %tmp, %tmp1
>>>> + br i1 %cmp, label %for.body, label %for.end
>>>> +
>>>> +for.body: ; preds = %for.cond
>>>> + %tmp2 = load %struct.ss**, %struct.ss*** %y.addr, align 2
>>>> + %tmp3 = load i16, i16* %i, align 2
>>>> + %arrayidx = getelementptr inbounds %struct.ss*, %struct.ss**
>>>> %tmp2, i16 %tmp3
>>>> + %tmp4 = load %struct.ss*, %struct.ss** %arrayidx, align 2
>>>> + %b = getelementptr inbounds %struct.ss, %struct.ss* %tmp4, i32 0,
>>>> i32 1
>>>> + %tmp5 = load i16, i16* %b, align 2
>>>> + %tmp6 = load %struct.ss*, %struct.ss** %x.addr, align 2
>>>> + %c = getelementptr inbounds %struct.ss, %struct.ss* %tmp6, i32 0,
>>>> i32 2
>>>> + %tmp7 = load i16, i16* %c, align 2
>>>> + %add = add nsw i16 %tmp7, %tmp5
>>>> + store i16 %add, i16* %c, align 2
>>>> + br label %for.inc
>>>> +
>>>> +for.inc: ; preds = %for.body
>>>> + %tmp8 = load i16, i16* %i, align 2
>>>> + %inc = add nsw i16 %tmp8, 1
>>>> + store i16 %inc, i16* %i, align 2
>>>> + br label %for.cond
>>>> +
>>>> +for.end: ; preds = %for.cond
>>>> + ret void
>>>> +}
>>>> +
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170202/710ee0cb/attachment.html>
More information about the llvm-commits
mailing list