<div dir="ltr">Thanks Bob. Committed in r<span style="font-family:arial,sans-serif;font-size:13px">220015.</span></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 16, 2014 at 4:43 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">This looks good. Thanks for the thorough description.<div><br></div><div>The fix itself is pretty obviously correct. I had some concerns that there might be other issues exposed in a case like this where there are very few instructions remaining to split. I generally expect the split-the-basic-block situation to come up when there is a really large block, but in this case the block is small. The reason the constant cannot be placed in this test is that we have a load with a very small offset field and the block is followed by a large constant pool. Our rules for convergence require that constants be placed at the end of an existing pool, which in this case is out of range. Akira and I discussed this in person, and we couldn’t find any other problems. Basically you can always end up with the fall-back of splitting immediately after the load instruction.</div><div><br><div><blockquote type="cite"><div><div class="h5"><div>On Oct 16, 2014, at 4:19 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:</div><br></div></div><div><div><div class="h5"><div dir="ltr"><div>The attached patch fixes a bug in ARM's constant island pass. The bug is in ARMConstantIslandPass.cpp:1307-1314 where the upper bound of the new water split point is computed:</div><div><br></div><div><div style="margin:0px;font-size:11px;font-family:Menlo">// This <span style="color:rgb(255,255,255);background-color:rgb(0,0,0)">could poi</span>nt off the end of the block if we've already got constant<br></div></div><div><div style="margin:0px;font-size:11px;font-family:Menlo">// pool entries following this block; only the last one is in the water list.</div><div style="margin:0px;font-size:11px;font-family:Menlo">// Back past any possible branches (allow for a conditional and a maximally</div><div style="margin:0px;font-size:11px;font-family:Menlo">// long unconditional).</div><div style="margin:0px;font-size:11px;font-family:Menlo">if (BaseInsertOffset + 8 >= UserBBI.postOffset()) {</div><div style="margin:0px;font-size:11px;font-family:Menlo">    BaseInsertOffset = UserBBI.postOffset() - UPad - 8;</div><div style="margin:0px;font-size:11px;font-family:Menlo">    DEBUG(dbgs() << format("Move inside block: %#x\n", BaseInsertOffset));</div><div style="margin:0px;font-size:11px;font-family:Menlo">}</div><div style="margin:0px;font-size:11px;font-family:Menlo"><br></div><div>The split point is supposed to be somewhere between the machine instruction that loads from the constant pool entry and the end of the basic block, before branch instructions. The code above is fine if the basic block is large enough and there are a sufficient number of instructions following the machine instruction. However, if the machine instruction is near the end of the basic block, BaseInsertOffset can point to the machine instruction or another instruction that precedes it, and this can lead to convergence failure (see the example below).</div><div><br></div><div>The attached patch fixes this bug by ensuring BaseInsertOffset is larger than the offset of the instruction following the constant-loading instruction.</div><div style="margin:0px;font-size:11px;font-family:Menlo"><br></div><p style="margin:0px;font-size:11px;font-family:Menlo"></p><div style="color:rgb(34,34,34);font-family:arial;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255)"></div><div><br></div><div style="color:rgb(34,34,34);font-family:arial;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255)"><div style="margin:0px;font-size:11px;font-family:Menlo">-    BaseInsertOffset = UserBBI.postOffset() - UPad - 8;</div><div style="margin:0px;font-size:11px;font-family:Menlo">+    BaseInsertOffset =<br></div><div style="margin:0px;font-size:11px;font-family:Menlo">+        std::max(UserBBI.postOffset() - UPad - 8,</div><div style="margin:0px;font-size:11px;font-family:Menlo">+                 UserOffset + TII->GetInstSizeInBytes(UserMI) + 1);</div><div style="margin:0px;font-size:11px;font-family:Menlo"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="font-family:arial;font-size:small"><a>rdar://problem/18581150</a></span><br></div></div></div><div><br></div><div><br></div><div>This is how the bug can lead to convergence failure:</div><div><br></div><div>1. Instruction VLDRS is too far from the constant pool entry cp#136 in BB#189. A new water has to be created in BB#324 after VLDRS.</div><div><br></div><div><div style="margin:0px;font-size:11px;font-family:Menlo">BB#189: Align 2 (4 bytes)</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">      </span>CONSTPOOL_ENTRY 345, <cp#136>, 4</div><div style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo">...</div><div style="margin:0px;font-size:11px;font-family:Menlo"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo">BB#323: Align 2 (4 bytes)</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap"> </span>CONSTPOOL_ENTRY 479, <cp#156>, 4</div><div style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo">BB#324: derived from LLVM BB %bb35</div><div style="margin:0px;font-size:11px;font-family:Menlo">    Predecessors according to CFG: BB#187</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">      </span>%D0<def> = VORRd %D9, %D9, pred:14, pred:%noreg, %S18<imp-use></div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap"> </span>%S6<def> = VLDRS <cp#345>, 0, pred:14, pred:%noreg, %D3<imp-def>; mem:LD4[ConstantPool]</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">  </span>%D5<def> = VLDRD <cp#346>, 0, pred:14, pred:%noreg; mem:LD8[ConstantPool]</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">      </span>t2STRi12 %R1<kill>, %R10, 0, pred:14, pred:%noreg; mem:ST4[%arg14]</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">   </span>t2B <BB#342>, pred:14, pred:%noreg</div><div style="margin:0px;font-size:11px;font-family:Menlo">    Successors according to CFG: BB#342</div><div><br></div><div><br></div><div>2. However, new water (BB#325) is created before VLDRS and cp#136 is moved there, because VLDRS is too close to the end of its parent basic block. This causes all the constant pool entries between BB#189 and BB#323 to be moved after constant pool entry cp#136, because they are now out of range, and this in turn causes cp#136 to be out of range again.</div><div><br></div><div><div style="margin:0px;font-size:11px;font-family:Menlo">BB#324: derived from LLVM BB %bb35</div><div style="margin:0px;font-size:11px;font-family:Menlo">    Predecessors according to CFG: BB#187</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">      </span>%D0<def> = VORRd %D9, %D9, pred:14, pred:%noreg, %S18<imp-use></div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap"> </span>t2B <BB#461>, pred:14, pred:%noreg</div><div style="margin:0px;font-size:11px;font-family:Menlo">    Successors according to CFG: BB#461</div><div style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo">BB#325: Align 2 (4 bytes)</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">       </span>CONSTPOOL_ENTRY 480, <cp#136>, 4</div><div style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px">...</div><div style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo">BB#460: Align 2 (4 bytes)</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">  </span>CONSTPOOL_ENTRY 615, <cp#156>, 4</div><div style="margin:0px;font-size:11px;font-family:Menlo;min-height:13px"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo">BB#461: derived from LLVM BB %bb35</div><div style="margin:0px;font-size:11px;font-family:Menlo">    Predecessors according to CFG: BB#324</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">      </span>%S6<def> = VLDRS <cp#480>, 0, pred:14, pred:%noreg, %D3<imp-def>; mem:LD4[ConstantPool]</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">  </span>%D5<def> = VLDRD <cp#481>, 0, pred:14, pred:%noreg; mem:LD8[ConstantPool]</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">      </span>t2STRi12 %R1<kill>, %R10, 0, pred:14, pred:%noreg; mem:ST4[%arg14]</div><div style="margin:0px;font-size:11px;font-family:Menlo"><span style="white-space:pre-wrap">   </span>t2B <BB#479>, pred:14, pred:%noreg</div><div style="margin:0px;font-size:11px;font-family:Menlo">    Successors according to CFG: BB#479</div></div></div></div>
</div></div><span><constant-island1.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></div></div></blockquote></div><br></div>