<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Apr 10, 2014, at 4:10 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Thu, Apr 10, 2014 at 3:57 PM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>> wrote:<br><blockquote type="cite"><br>On Apr 10, 2014, at 3:14 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><br>On Thu, Apr 10, 2014 at 2:49 PM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>> wrote:<br><br>Author: grosbach<br>Date: Thu Apr 10 16:49:24 2014<br>New Revision: 205988<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=205988&view=rev">http://llvm.org/viewvc/llvm-project?rev=205988&view=rev</a><br>Log:<br>[ARM64,C++11]: Range'ify loops in the conditional-compare pass.<br><br>Modified:<br> llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp<br><br>Modified: llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp<br>URL:<br><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp?rev=205988&r1=205987&r2=205988&view=diff">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp?rev=205988&r1=205987&r2=205988&view=diff</a><br>==============================================================================<br>--- llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp (original)<br>+++ llvm/trunk/lib/Target/ARM64/ARM64ConditionalCompares.cpp Thu Apr 10<br>16:49:24 2014<br>@@ -212,13 +212,14 @@ public:<br>// Check that all PHIs in Tail are selecting the same value from Head and<br>CmpBB.<br>// This means that no if-conversion is required when merging CmpBB into<br>Head.<br>bool SSACCmpConv::trivialTailPHIs() {<br>- for (MachineBasicBlock::iterator I = Tail->begin(), E = Tail->end();<br>- I != E && I->isPHI(); ++I) {<br>+ for (auto &I : *Tail) {<br>+ if (!I.isPHI())<br>+ break;<br> unsigned HeadReg = 0, CmpBBReg = 0;<br> // PHI operands come in (VReg, MBB) pairs.<br>- for (unsigned oi = 1, oe = I->getNumOperands(); oi != oe; oi += 2) {<br>- MachineBasicBlock *MBB = I->getOperand(oi + 1).getMBB();<br>- unsigned Reg = I->getOperand(oi).getReg();<br>+ for (unsigned oi = 1, oe = I.getNumOperands(); oi != oe; oi += 2) {<br>+ MachineBasicBlock *MBB = I.getOperand(oi + 1).getMBB();<br>+ unsigned Reg = I.getOperand(oi).getReg();<br> if (MBB == Head) {<br> assert((!HeadReg || HeadReg == Reg) && "Inconsistent PHI operands");<br> HeadReg = Reg;<br>@@ -237,14 +238,15 @@ bool SSACCmpConv::trivialTailPHIs() {<br>// Assuming that trivialTailPHIs() is true, update the Tail PHIs by simply<br>// removing the CmpBB operands. The Head operands will be identical.<br>void SSACCmpConv::updateTailPHIs() {<br>- for (MachineBasicBlock::iterator I = Tail->begin(), E = Tail->end();<br>- I != E && I->isPHI(); ++I) {<br>+ for (auto &I : *Tail) {<br>+ if (!I.isPHI())<br>+ break;<br> // I is a PHI. It can have multiple entries for CmpBB.<br>- for (unsigned oi = I->getNumOperands(); oi > 2; oi -= 2) {<br>+ for (unsigned oi = I.getNumOperands(); oi > 2; oi -= 2) {<br> // PHI operands are (Reg, MBB) at (oi-2, oi-1).<br>- if (I->getOperand(oi - 1).getMBB() == CmpBB) {<br>- I->RemoveOperand(oi - 1);<br>- I->RemoveOperand(oi - 2);<br>+ if (I.getOperand(oi - 1).getMBB() == CmpBB) {<br>+ I.RemoveOperand(oi - 1);<br>+ I.RemoveOperand(oi - 2);<br> }<br> }<br> }<br>@@ -387,10 +389,8 @@ bool SSACCmpConv::canSpeculateInstrs(Mac<br><br> // Check all instructions, except the terminators. It is assumed that<br> // terminators never have side effects or define any used register values.<br>- for (MachineBasicBlock::iterator I = MBB->begin(),<br>- E = MBB->getFirstTerminator();<br>- I != E; ++I) {<br>- if (I->isDebugValue())<br>+ for (auto &I : make_range(MBB->begin(), MBB->getFirstTerminator())) {<br>+ if (I.isDebugValue())<br> continue;<br><br> if (++InstrCount > BlockInstrLimit && !Stress) {<br>@@ -400,29 +400,29 @@ bool SSACCmpConv::canSpeculateInstrs(Mac<br> }<br><br> // There shouldn't normally be any phis in a single-predecessor block.<br>- if (I->isPHI()) {<br>- DEBUG(dbgs() << "Can't hoist: " << *I);<br>+ if (I.isPHI()) {<br>+ DEBUG(dbgs() << "Can't hoist: " << I);<br> return false;<br> }<br><br> // Don't speculate loads. Note that it may be possible and desirable to<br> // speculate GOT or constant pool loads that are guaranteed not to trap,<br> // but we don't support that for now.<br>- if (I->mayLoad()) {<br>- DEBUG(dbgs() << "Won't speculate load: " << *I);<br>+ if (I.mayLoad()) {<br>+ DEBUG(dbgs() << "Won't speculate load: " << I);<br> return false;<br> }<br><br> // We never speculate stores, so an AA pointer isn't necessary.<br> bool DontMoveAcrossStore = true;<br>- if (!I->isSafeToMove(TII, 0, DontMoveAcrossStore)) {<br>- DEBUG(dbgs() << "Can't speculate: " << *I);<br>+ if (!I.isSafeToMove(TII, 0, DontMoveAcrossStore)) {<br>+ DEBUG(dbgs() << "Can't speculate: " << I);<br> return false;<br> }<br><br> // Only CmpMI is allowed to clobber the flags.<br>- if (&*I != CmpMI && I->modifiesRegister(ARM64::CPSR, TRI)) {<br>- DEBUG(dbgs() << "Clobbers flags: " << *I);<br>+ if (&I != CmpMI && I.modifiesRegister(ARM64::CPSR, TRI)) {<br>+ DEBUG(dbgs() << "Clobbers flags: " << I);<br> return false;<br> }<br> }<br>@@ -906,11 +906,9 @@ bool ARM64ConditionalCompares::runOnMach<br> // Visit blocks in dominator tree pre-order. The pre-order enables<br>multiple<br> // cmp-conversions from the same head block.<br> // Note that updateDomTree() modifies the children of the DomTree node<br>- // currently being visited. The df_iterator supports that, it doesn't<br>look at<br>+ // currently being visited. The df_iterator supports that; it doesn't<br>look at<br> // child_begin() / child_end() until after a node has been visited.<br>- for (df_iterator<MachineDominatorTree *> I = df_begin(DomTree),<br>- E = df_end(DomTree);<br>- I != E; ++I)<br>+ for (auto *I : make_range(df_begin(DomTree), df_end(DomTree)))<br><br><br>Might be nice to add the range function for these two utilities<br>(df_end/begin - either "dfs" or "df_range" or something more verbose<br>like "dom_frontiers" (at a guess)?)? (I guess this is why you added<br>make_range, though - I'd assumed you were just using it to simplify<br>the implementation of range accessor functions - I'd have a slight<br>preference of avoiding its use more generally in favor of providing<br>reusable range accessors or other utilities (range<br>splitting/merging/etc rather than doing iterator stuff))<br><br><br>Well, I added it for the bits that were iterating over instructions in a<br>block but stopping at the first terminator instruction rather than end().<br></blockquote><br>Ah, looking back at the change I see those now (hadn't spotted them<br>amongst the code changes to the loop bodies).<br><br>Compare this to my change to MachineInstr (<br><a href="http://llvm.org/viewvc/llvm-project?rev=205680&view=rev">http://llvm.org/viewvc/llvm-project?rev=205680&view=rev</a><span class="Apple-converted-space"> </span>) - if it's<br>common to need "all the instructions except the terminators" it might<br>be nice to introduce an accessor for that rather than building it at<br>every loop. Maybe "non_terminators()". Indeed in time we might be able<br>to get rid of "getFirstTerminator" and just have "terminators()",<br>"non_terminators()" and "instructions()" (the latter being terminators<br>+ non_terminators).<br></div></blockquote><div><br></div>Agreed. I’m just working through these as I go. The plan is to start w/ one-off uses w/ make_range() and then convert those to actual accessor functions like you’re suggesting when/if I encounter more than a handful of instances of a given pattern. Sounds like that may be the case for this one and I like the general direction. I’ll have a look, thanks!<br><div><br></div><br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Just ideas, this stuff is all still pretty experimental as to what the<br>"right" usage is.<br></div></blockquote><div><br></div><div>Yep, totally. Spurring some of that discussion is exactly what I’m hoping to get out of making these little changes.</div><div><br></div><div>-Jim</div><br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">Here I just got excited about having make_range() and used it. I have a<br>hammer now, so everything's a nail.<br><br>Actual functions for these seems reasonable. I'll add those in a moment.<br><br><br> if (tryConvert(I->getBlock()))<br> Changed = true;<br><br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</blockquote></div></blockquote></div><br></body></html>