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