[llvm] r283838 - [RegAllocGreedy] Attempt to split unspillable live intervals

Wed Feb 1 14:03:29 PST 2017

Hi Mikael,

Nice catch!

> On Jan 31, 2017, at 11:33 PM, Mikael Holmén <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.

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
>> 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/20170201/efce41f1/attachment-0002.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: markNotSpillable.patch
Type: application/octet-stream
Size: 1465 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170201/efce41f1/attachment-0001.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170201/efce41f1/attachment-0003.html>