<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="">Hi Matthias,<div class=""><br class=""></div><div class="">Renato did all the heavy work of carefully reviewing this last year and as a result most of my comments are nitpicks or refactoring that we could address in subsequent patches if that is simpler for you.</div><div class=""><br class=""></div><div class="">First a couple of general remarks:</div><div class="">- Use clang-format.</div><div class="">- Add a message in asserts (I have reported that in the first patches, I let you review all of them).</div><div class="">- Use range iterator for the subrange loops.</div><div class="">- Remove brackets for one line blocks (I have reported that in the first patches).</div><div class="">- Use nullptr instead of NULL or 0.</div><div class=""><br class=""></div><div class=""><br class=""><div><div class="">On Nov 18, 2014, at 7:40 PM, Matthias Braun <<a href="mailto:matze@braunis.de" class="">matze@braunis.de</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi<div class=""><br class=""></div><div class="">attached is a new iteration of my subregister liveness tracking patches. I already submitted these for review last year:</div><div class=""><span class="Apple-tab-span" style="white-space:pre"> </span><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131007/190279.html" class="">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131007/190279.html</a></div><div class=""><span class="Apple-tab-span" style="white-space:pre"> </span><a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-October/066207.html" class="">http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-October/066207.html</a></div><div class=""><br class=""></div><div class="">I did not want to commit them last year because I wasn't able to completely fix the testsuite failures found in the X86/ARM backends. However as I started working on this topic again, I do not want to hold the patches back any longer and prepare people for what is coming.</div><div class=""><br class=""></div><div class="">The attached patchset has no fundamental changes to the patches proposed last year:</div><div class="">* Rebased/Adapted to llvm trunk</div><div class="">* Added and improved comments for the RegisterCoalescer changes</div><div class="">* Fix several edges cases in the RegisterCoalescer related to LiveIntervals which are partially undefined. With subregister liveness we must sometimes keep IMPLICIT_DEF instructions to not break the verifier, there was also a case where we also have to recompute the main liverange if any of the subregister liverange needed to be recomputed.</div><div class="">* Various comment improvements/cleanups</div><div class=""><br class=""></div><div class="">If you enable the subregister liveness tracking for X86, ARM, R600 the teststuite state is:</div><div class="">* X86 shows no regression</div><div class="">* ARM is down to 2 regressions after a set of followup patches I will present in my next mail</div><div class="">* R600 shows 1 regression</div><div class="">with subregister liveness tracking disabled no changes are visible.</div><div class=""><br class=""></div><div class="">Greetings</div><div class=""><span class="Apple-tab-span" style="white-space:pre"> </span>Matthias</div><div class=""></div></div><span id="cid:0C1621AD-3B49-488D-A271-8E07AA1BBF7D"><0001-Let-tablegen-compute-maximum-lanemask-for-regs-regcl.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:B292516A-A914-493A-8F4A-A69F3050B799"><0002-Add-function-that-translates-subregister-lane-masks-.patch></span></blockquote><div><br class=""></div><div>LGTM with a few nitpicks.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // are usually the same for many subregisters we can easily combin the steps</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // by combining the masks.</div></div><div>combin -> combine</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if (LaneTransforms.size() == 1) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> LaneTransforms[0].Mask = ~0u;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div><div style="margin: 0px;" class=""><br class=""></div><div style="margin: 0px;" class="">No need for brackets.</div></div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> unsigned Found = ~0u;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> unsigned SIdx = 0;</div><div class=""><br class=""></div><div class="">There are a couple of places with strange formatting like this. Please use clang-format.</div><div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> OS << " --IdxA; assert(IdxA < " << SubRegIndices.size() << ");\n"</div></div><div class=""><br class=""></div><div class="">Could you add a simple message like "Index out of bound" in the generated assert.</div></div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:2CF5EF11-8B05-4908-96C0-DCBCB9D5EDDF"><0003-Add-const-version-of-LiveRange-advanceTo.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:806267D1-5EE5-44B6-BCCA-CF4C1A3EE48B"><0004-Add-a-covers-operation-to-LiveRange.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:E4E3858C-C1A4-4BB9-B5BD-6AB15A1432D5"><0005-Add-support-to-track-liveness-of-subregisters-in-a-L.patch></span></blockquote><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> class SubRange : public LiveRange {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> public:</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> SubRange *Next;</div></div><div>[…]</div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> template<typename T></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> class SingleLinkedListIterator {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> T *P;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> public:</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> SingleLinkedListIterator<T>(T *P) : P(P) {}</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> SingleLinkedListIterator<T> &operator++() {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> P = P->Next;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> return *this;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div><div class=""><br class=""></div></div><div>Instead of having a new template for single linked list, could it be possible to reuse the ilist class for definition of SubRange?</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">-</span> : reg(Reg), weight(Weight) {}</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> : SubRanges(0), reg(Reg), weight(Weight) {}</div><div class=""><br class=""></div><div class="">0 -> nullptr</div><div class="">There are a couple of other places where this should happen.</div><div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> LI.clearSubRanges(); // TODO</div></div><div class=""><br class=""></div><div class="">What is the plan here? Unless I am mistaken, this is not handled by any subsequent patches.</div><div class="">I am just asking, this should not prevent the whole thing to happen. This is strictly an improvement over the existing framework.</div><div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> assert((Mask & I->LaneMask) == 0);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> assert((Mask & ~MaxMask) == 0);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">[…]</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: rgb(255, 255, 255); background-color: rgb(0, 0, 0);" class="">+</span> assert(covers(*I));</div></div><div class=""><br class=""></div><div class="">Although this is clear from the code what those are checking, having a message in the assert explaining what is checked would help track down errors or reading the code.</div><div class="">Could you add such messages please?</div><div class=""><br class=""></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // TODO: do not cheat anymore by simply cleaning all subranges</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> LI.clearSubRanges();</div><div class="">Same question as previously, what is the plan here?</div><div class=""><br class=""></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span>unsigned MachineRegisterInfo::getMaxLaneMaskForReg(unsigned Reg) const</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span>{</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> assert(TargetRegisterInfo::isVirtualRegister(Reg));</div><div class="">Maybe a message saying that physical registers are not supported and a comment saying why.</div><div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> unsigned Mask = 0;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> unsigned MaxMask = MRI->getMaxLaneMaskForReg(Reg);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> for (LiveInterval::const_subrange_iterator I = LI.subrange_begin(),</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> E = LI.subrange_end(); I != E; ++I) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if ((Mask & I->LaneMask) != 0) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> report("Lane masks of sub ranges overlap in live interval", MF, LI);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if ((I->LaneMask & ~MaxMask) != 0) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> report("Subrange lanemask is invalid", MF, LI);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> Mask |= I->LaneMask;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> verifyLiveRange(*I, LI.reg, I->LaneMask);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if (!LI.covers(*I)) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> report("A Subrange is not covered by the main range", MF, LI);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div></div><div class=""><br class=""></div><div class="">Could you think of a way we can refactor the code to share logic with <span style="font-family: Menlo; font-size: 11px;" class="">LiveInterval::verify </span>instead of duplicating it?</div><div class=""><br class=""></div><div class=""><br class=""></div></div><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:37590F8F-9E2A-4F76-B4A3-52259BC0AC1A"><0006-Compute-subregister-ranges-in-LiveIntervalAnalysis.patch></span></blockquote><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> const MachineInstr *MI = MO.getParent();</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // Find the corresponding slot index.</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> SlotIndex Idx;</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if (MI->isPHI())</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // PHI defs begin at the basic block start index.</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> Idx = Indexes->getMBBStartIdx(MI->getParent());</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> else</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // Instructions are either normal 'r', or early clobber 'e'.</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> Idx = Indexes->getInstructionIndex(MI).getRegSlot(MO.isEarlyClobber());</div><div class=""><br class=""></div></div></div>Create a small function for that taking a const MachineOperand & and returning the index. That way we can avoid duplicating this code between both version of createDeadDefs.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> assert(Common == S->LaneMask);</div><div><br class=""></div><div>Message in the assert like “At this point the sub range should be entirely covered”. That may not be super useful right now, but since this is currently trivially true, we have to consider this assert as a guide for future implementation update and thus, explain what was the intent of the assert here.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> const TargetRegisterInfo &TRI = *MRI->getTargetRegisterInfo();</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> assert(MRI && Indexes && "call reset() first”);</div></div><div><br class=""></div>We should invert the order of those statement. If MRI is nullptr, then this will crash instead of asserting.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> // Transfer information from the LiveIn vector to the live ranges.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">-</span>void LiveRangeCalc::updateLiveIns() {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span>void LiveRangeCalc::updateLiveIns(LiveOutData &LiveOuts) {</div></div><div><br class=""></div><div>Add a comment on what the LiveOuts argument is used for. Indeed having LiveIn in the comment and not LiveOuts may be confusing because one can think this is a typo whereas it refers to the class member and both values are used.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> bool findReachingDefs(LiveRange &LR, MachineBasicBlock &KillMBB,</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">-</span> SlotIndex Kill, unsigned PhysReg);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> SlotIndex Kill, unsigned PhysReg,</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> LiveOutData &LiveOuts);</div><p style="margin: 0px; font-size: 11px; font-family: Menlo; min-height: 13px;" class="">[…]</p><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">-</span> void updateSSA();</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> void updateSSA(LiveOutData &LiveOuts);</div><p style="margin: 0px; font-size: 11px; font-family: Menlo; min-height: 13px;" class="">[…]</p><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">-</span> void updateLiveIns();</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> void updateLiveIns(LiveOutData &LiveOuts);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">[…]</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">-</span> void extend(LiveRange &LR, SlotIndex Kill, unsigned PhysReg = 0);</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> void extend(LiveRange &LR, SlotIndex Kill, unsigned PhysReg,</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> LiveOutData &LiveOuts);</div><div style="margin: 0px;" class="">[…]</div><div style="margin: 0px;" class=""><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">-</span> void calculateValues();</div><div style="margin: 0px;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> void calculateValues(LiveOutData &LiveOuts);</div></div></div></div><div><br class=""></div><div>Add a comment on the usage of LiveOuts.</div><div><br class=""></div><div><br class=""></div><div><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:B194BF2C-37F4-4217-97C3-07E16C9149B3"><0007-Update-SubRanges-in-LiveIntervalAnalysis-shrinkToUse.patch></span></blockquote><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span>bool LiveIntervals::shrinkToUses(LiveInterval::SubRange &SR, unsigned Reg)</div></div><div><br class=""></div><div>There is a lot of duplicated code between this new method and the original shrinkToUses.</div><div>I think we can do a better job at reusing the logic, here are my proposals:</div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // Create a new live ranges with only minimal live segments per def.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> LiveRange NewLR;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> for (LiveInterval::vni_iterator I = SR.vni_begin(), E = SR.vni_end();</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> I != E; ++I) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> VNInfo *VNI = *I;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if (VNI->isUnused())</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> continue;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> NewLR.addSegment(LiveInterval::Segment(VNI->def, VNI->def.getDeadSlot(),</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> VNI));</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div></div><div><br class=""></div><div>Create a method for that loop. Arguments could be <font face="Menlo" style="font-size: 11px;" class="">LiveRange &</font>, <span style="font-family: Menlo; font-size: 11px;" class="">LiveInterval::vni_iterator begin, </span><span style="font-family: Menlo; font-size: 11px;" class="">LiveInterval::vni_iterator end.</span></div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // Keep track of the PHIs that are in use.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> SmallPtrSet<VNInfo*, 8> UsedPHIs;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="background-color: rgb(0, 0, 0); color: rgb(255, 255, 255);" class="">+</span></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="background-color: rgb(0, 0, 0); color: rgb(255, 255, 255);" class="">+</span> // Extend intervals to reach all uses in WorkList.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> while (!WorkList.empty()) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">[…]</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: rgb(255, 255, 255); background-color: rgb(0, 0, 0);" class="">+</span> }</div></div><div><br class=""></div><div>Create a method for this whole loop.</div><div><br class=""></div><div>Update computeDeadValues to take iterators, a register, and a bool instead of a LiveInterval.</div><div>- The iterators would be used as begin and end for the loop.</div><div>- The register will be used in place of li->reg.</div><div>- The bool will be used to conditionally execute the else-block. I.e., bool == false for sub registers. (Instead of a bool we may use reg != 0 or something).</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // Handle dead values.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> bool CanSeparate = false;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">[…]</div></div><div>Call computeDeadValues with the appropriate parameter.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:07B65F8F-69BE-452A-96C1-979C463ACE84"><0008-Adapt-LiveIntervalAnalysis-handleMove-for-subregiste.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:2DC846D7-F2C8-4760-8131-B6FD7432B0B0"><0009-Adapt-LiveRangeEdit-eliminateDeadDef-for-subregister.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:7AADE96C-098A-4C26-991F-96D091ECEB68"><0010-Adapt-LiveIntervalAnalysis-repairIntervalsInRange-to.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:18362EBB-8580-4DB9-AC60-C20EF42ACC26"><0011-Add-a-flag-to-enable-disable-subregister-liveness.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:7A027E85-3999-4D1A-BA76-6D9A59C0561D"><0012-Introduce-LiveQuery-accessor-for-dead-or-live-out-va.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:CA17B6A5-7138-4D26-B6B4-C88B47862543"><0013-Add-subregister-aware-variants-of-LiveIntervalAnalys.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:A959C718-59DB-47AF-879E-809CFBCEBC58"><0014-Add-LiveInterval-removeEmptySubRanges.patch></span></blockquote><div><br class=""></div>LGTM with a few nitpicks.<br class=""><div><br class=""></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> /// Removes all subranges without any segments (subranges without segments</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> /// are not considered valid and should only exist temporarily)</div><div><br class=""></div><div>Period at the end of a comment.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // Skip empty subranges until we find the first nonempty one</div></div><div><br class=""></div>Ditto.<br class=""><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:E4CD34F8-39FB-45D3-83FF-E5B000FE318C"><0015-Preserve-subregister-liveranges-in-register-coalesce.patch></span></blockquote><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> /// A LaneMask to remeber on which subregister live ranges we need to call</div></div><div><br class=""></div><div>remeber -> remember</div><div><br class=""></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> void mergeSubRangeInto(LiveInterval &LI, const LiveRange &ToMerge,</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> unsigned LaneMask, CoalescerPair &CP);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: rgb(255, 255, 255); background-color: rgb(0, 0, 0);" class="">+</span></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> bool joinLanes(LiveRange &LRange, LiveRange &RRange,</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> const CoalescerPair &CP);</div><div><br class=""></div>Please add comments on those methods.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span>static void addSegmentsWithValNo(LiveRange &Dst, VNInfo *DstValNo,</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> const LiveRange &Src, const VNInfo *SrcValNo)</div></div><div><br class=""></div><div>The code is quite trivial to understand but a comment would be still nice to detail how the arguments are used.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // duplicate SubRange for newly merged common stuff</div></div><div><br class=""></div><div>Period at the end of the comment and capital letter at the beginning.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // we van reuse the L SubRange</div></div><div><br class=""></div><div>Ditto.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> LiveInterval::SubRange *CommonRange;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if (BRest != 0) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">[…]</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: rgb(255, 255, 255); background-color: rgb(0, 0, 0);" class="">+</span> } else {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // we van reuse the L SubRange</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> SB->LaneMask = Common;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> CommonRange = &*SB;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">[…]</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: rgb(255, 255, 255); background-color: rgb(0, 0, 0);" class="">+</span> AMask &= ~BMask;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if (AMask != 0) {</div><div class="">[...]</div></div><div><br class=""></div><div>If I remember correctly, this is the third time this pattern occurs in the patches. I wonder if we could do something (assuming it make sense) to share this logic somewhere.</div><div>Thoughts?</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> LiveRange &LR;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> unsigned Reg;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> bool SubRangeJoin;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> bool TrackSubRegLiveness;</div></div><div><br class=""></div><div>Please document the new members.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">-</span> void pruneValues(JoinVals &Other, SmallVectorImpl<SlotIndex> &EndPoints);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">+</span> void pruneValues(JoinVals &Other, SmallVectorImpl<SlotIndex> &EndPoints,</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">+</span> bool changeInstrs);</div></div><div><br class=""></div><div>Please document the new argument.</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> // Detect impossible conflicts early.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> if (!LHSVals.mapValues(RHSVals) || !RHSVals.mapValues(LHSVals))</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="color: #ffffff; background-color: #000000" class="">+</span> abort();</div></div><div><br class=""></div><div>I believe this should either be “return false” or llvm_unreachable().</div><div><br class=""></div><div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:1737E2DA-4AFC-472D-A4DA-7BF24E1AE642"><0016-amend-to-RegisterCoalescer.patch></span></blockquote><div><br class=""></div><div>LGTM. BTW it fixes some of the comments on the previous patch.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:13BEEAE0-D76C-4D37-B797-9938D0DCECBE"><0017-Tablegen-erate-lanemasks-for-register-units.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:CB576197-3A1A-4F44-8A56-13FA99D67AF7"><0018-LiveIntervalUnion-allow-specification-of-liverange-w.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:9E11FB01-4E4C-4E33-A965-3F6251ACAAA5"><0019-Respect-subregister-liveness-when-allocating-registe.patch></span></blockquote><div><br class=""></div>LGTM, with maybe some code refactoring possible.<br class=""><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">-</span> for (MCRegUnitIterator Units(PhysReg, TRI); Units.isValid(); ++Units) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">-</span> DEBUG(dbgs() << ' ' << PrintRegUnit(*Units, TRI));</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">-</span> Matrix[*Units].unify(VirtReg);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">+</span> if (VirtReg.hasSubRanges()) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">+</span> for (MCRegUnitMaskIterator Units(PhysReg, TRI); Units.isValid(); ++Units) {</div></div>[…]</div><div>The structure of the code is the same for the three places we update. I was wondering if we could share the logic. What do you think?</div><div><br class=""></div><div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:641FEFB2-1FA5-44BF-B463-AD83CDED0F54"><0020-Do-not-add-implicit-defs-uses-when-tracking-subregis.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:1EC3FF43-5628-4BCC-B2F2-DBC5A5493B05"><0021-Add-MCSubRegIndexIterator.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:5F79CBCC-349F-4477-9147-42C8E2332FD2"><0022-Better-Block-live-in-info-if-subregliveness-is-avail.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:7BB593F1-8897-444B-B401-5FB47ED506F2"><0023-MachineVerifier-Allow-LiveInterval-segments-to-end-a.patch></span></blockquote><div><br class=""></div><div>LGTM.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""></div></div><span id="cid:CBC88F5F-BFDF-44A6-A8C9-22CB84734CAA"><0024-MachineVerifier-allow-physreg-use-if-just-a-subreg-i.patch></span><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""></div></div></blockquote><div><br class=""></div>LGTM.<br class=""><br class=""></div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #ffffff; background-color: #000000" class="">+</span> bool bad = !isReserved(Reg);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""><br class=""></div><div class="">Variables name should start with a capital letter.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">-Quentin</div></div></div></body></html>