[llvm] r283838 - [RegAllocGreedy] Attempt to split unspillable live intervals
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 1 23:24:56 PST 2017
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
>
More information about the llvm-commits
mailing list