[llvm] r283838 - [RegAllocGreedy] Attempt to split unspillable live intervals
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 3 04:49:30 PST 2017
On 02/02/2017 09:56 PM, Quentin Colombet wrote:
> Fixed in r293934
Excellent. Thanks Quentin!
/Mikael
>
>> On Feb 1, 2017, at 11:24 PM, Mikael Holmén <mikael.holmen at ericsson.com
>> <mailto: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>
>>>> <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
>>>
>>
>
More information about the llvm-commits
mailing list