<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 Dec 10, 2013, at 11:59 AM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">I find it extremely suspect that a patch to deprecate IT blocks for ARM required changes to the generic IfConverter pass at all.</div></blockquote><div><br></div>I agree. This is very suspicious.</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div>The symptoms here seem very similar to the earlier problem I referred to, which was r187071.<br><div><br><div><div>On Dec 10, 2013, at 11:45 AM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</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;">Jim,<br><br>Thanks! If you have another minute, let me quickly summarize my current understanding:<br><br>In IfConverter::ScanInstructions, pre-r190309, we had the following special case:<br><br> if (BBI.ClobbersPred && !isPredicated) {<br> // Predicate modification instruction should end the block (except for<br> // already predicated instructions and end of block branches).<br> if (isCondBr) {<br> // A conditional branch is not predicable, but it may be eliminated.<br> continue;<br> }<br><br> // Predicate may have been modified, the subsequent (currently)<br> // unpredicated instructions cannot be correctly predicated.<br> BBI.IsUnpredicable = true;<br> return;<br> }<br><br> ...<br><br> if (!TII->isPredicable(I)) {<br> BBI.IsUnpredicable = true;<br> return;<br> }<br><br>and so, for an unpredicated conditional branch, in a block which clobbers the predicate, we would not check whether or not the branch could be predicated under some assumption that it will be later removed. I don't understand this, but as Artyom pointed out to me, the relevant code is in IfConverter::IfConvertTriangle:<br><br> if (CvtBBI->BB->pred_size() > 1) {<br> ...<br> } else {<br> // Predicate the 'true' block after removing its branch.<br> CvtBBI->NonPredSize -= TII->RemoveBranch(*CvtBBI->BB);<br> PredicateBlock(*CvtBBI, CvtBBI->BB->end(), Cond, Redefs);<br><br> // Now merge the entry of the triangle with the true block.<br> BBI.NonPredSize -= TII->RemoveBranch(*BBI.BB);<br> MergeBlocks(BBI, *CvtBBI, false);<br> }<br><br>then in r190309 this special case was expanded to include *all* conditional branches (whether or not they are in predicate-clobbering blocks). This is what triggers the self-hosting failure on PowerPC. For example, we now end up with code like this:<br><br></div></blockquote><div><br></div><div>That change sounds really questionable, even for ARM.</div></div></div></div></div></blockquote><div><br></div>The fact that r190309 required changes to IfConverter.cpp is very questionable. The change which had to do with conditional branches seems suspicious. Is reverting the IfConverter.cpp part of r190309 the right solution for now?</div><div><br></div><div>Evan</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div><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>.LBB0_6: # %if.end7.i37<br> lbz 4, 0(3)<br> li 30, 1<br> cmplwi 0, 4, 0<br> beq 0, .LBB0_15<br># BB#7: # %if.then9.i39<br> bne 1, .LBB0_14<br> b .LBB0_15<br>.LBB0_8: # %if.end<br> ...<br><br> BB6<br> | \<br> | BB7<br> | / \<br> | / \<br> BB15 BB14<br><br>being transformed into:<br><br> ...<br>.LBB0_6: # %if.end7.i37<br> lbz 4, 0(3)<br> li 30, 1<br> cmplwi 0, 4, 0<br> bne 1, .LBB0_13<br> b .LBB0_14<br>.LBB0_7: # %if.end<br> ...<br><br>(adjusting for the later block numbering change)<br> BB6<br> | \<br> | \<br> BB15 BB14<br><br>but without combining the conditions from BB6 and BB7 (but instead just eliminated one). Now if the "bne 1, .LBB0_14" in BB7 were actually predicated on the "(eq, cr0)" value, then this would all make sense (but it is not, and can't be on this architecture). TII->isPredicable on this instruction returns false (which I think should prevent this problem in the first place, but does not, because of this special case for conditional branches).<br></div></blockquote><div><br></div><div>Ouch. That’s really nasty. :(</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>My fear is that the special case pre-r190309 was also wrong for PowerPC, and that by restoring it, we're just reinstating an even-more-subtle bug for non-ARM backends.<br></div></blockquote><div><br></div><div>That’s possible. I don’t know enough about the PPC details to know. The theory is that the TII->SubsumesPredicate() check can help disambiguate strange cases like this, but that’s generally for non-branch instructions.</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>-Hal<br><br>----- Original Message -----<br><blockquote type="cite">From: "Jim Grosbach" <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>><br>To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>Cc: "llvm-commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>>, "Bill Wendling" <<a href="mailto:isanbard@gmail.com">isanbard@gmail.com</a>>, "Artyom Skrobov"<br><<a href="mailto:Artyom.Skrobov@arm.com">Artyom.Skrobov@arm.com</a>><br>Sent: Tuesday, December 10, 2013 12:47:23 PM<br>Subject: Re: [llvm] r190309 - [ARMv8] Prevent generation of deprecated IT blocks<span class="Apple-tab-span" style="white-space: pre;"> </span>on ARMv8 in Thumb mode.<br><br>Hi Hal,<br><br>I’m not very familiar with the patch this thread is in response to,<br>so I can’t comment very effectively on that aspect of things<br>directly.<br><br>Your comment below about multiple branch instructions at the end of a<br>basic block, however, is correct. The IfConversion pass needs to<br>handle those sorts of cases, including degenerate cases where<br>AnalyzeBranch() fails (by punting and not doing if-conversion of<br>that).<br><br>There are helper functions in the ARM backend (and I assume others)<br>which deal with getting the inverted condition code for<br>if-converting if/else constructs. Perhaps there’s something wrong<br> there either in the general pass or the PPC backend? I seem to<br>recall there was a nasty bug in the ARM version a while back that<br>was painful to track down.<br><br>-Jim<br><br>On Dec 10, 2013, at 10:24 AM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br><br><blockquote type="cite">Jim, as you probably saw, Bill recommended you as someone who could<br>help clear up the confusion here. I would appreciate your input; I<br>need to get this cleared up this week (given our current release<br>schedule).<br><br>Thanks in advance,<br>Hal<br><br>----- Original Message -----<br><blockquote type="cite">From: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>To: "Artyom Skrobov" <<a href="mailto:Artyom.Skrobov@arm.com">Artyom.Skrobov@arm.com</a>><br>Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>Sent: Tuesday, December 3, 2013 11:02:29 AM<br>Subject: Re: [llvm] r190309 - [ARMv8] Prevent generation of<br>deprecated IT<span class="Apple-tab-span" style="white-space: pre;"> </span>blocks<span class="Apple-tab-span" style="white-space: pre;"> </span>on ARMv8 in Thumb mode.<br><br>----- Original Message -----<br><blockquote type="cite">From: "Artyom Skrobov" <<a href="mailto:Artyom.Skrobov@arm.com">Artyom.Skrobov@arm.com</a>><br>To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>Cc: "Joey Gouly" <<a href="mailto:Joey.Gouly@arm.com">Joey.Gouly@arm.com</a>>, <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>,<br>"Tim Northover" <<a href="mailto:t.p.northover@gmail.com">t.p.northover@gmail.com</a>>, "Evan<br>Cheng" <<a href="mailto:evan.cheng@apple.com">evan.cheng@apple.com</a>><br>Sent: Monday, December 2, 2013 12:23:27 PM<br>Subject: RE: [llvm] r190309 - [ARMv8] Prevent generation of<br>deprecated IT<span class="Apple-tab-span" style="white-space: pre;"> </span>blocks<span class="Apple-tab-span" style="white-space: pre;"> </span>on ARMv8 in Thumb mode.<br><br>Hal, could you please clarify why the PPC backend produces<br>BasicBlocks with several branch instructions sharing the same BB?<br><br>The doxygen comments in LLVM imply that a BB is a "single entry,<br>single exit" sequence, and any branch instruction in a BB must<br>terminate it.<br></blockquote><br>That seems mostly right, but a block can have multiple terminators<br>(if one is conditional and one is unconditional), and the<br>unconditional branch may be elided if its destination is the next<br>block in layout ordering. Taking a quick look at AnalyzeBranch in<br>the various backends, I believe that this is true also on X86,<br>Hexagon, Sparc, SystemZ, etc. This allows forming a TBB/FBB pair<br>using two consecutive branches.<br><br>Is this somehow incompatible with the design of the if conversion<br>pass?<br><br>Thanks again,<br>Hal<br><br><blockquote type="cite"><br>As you can see in the code examples I posted earlier, this is<br>indeed<br>the case with the ARM backend.<br><br><br>-----Original Message-----<br>From: Hal Finkel [<a href="mailto:hfinkel@anl.gov">mailto:hfinkel@anl.gov</a>]<br>Sent: 28 November 2013 19:50<br>To: Artyom Skrobov<br>Cc: Joey Gouly; <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>; Tim Northover; Evan<br>Cheng<br>Subject: Re: [llvm] r190309 - [ARMv8] Prevent generation of<br>deprecated IT blocks on ARMv8 in Thumb mode.<br><br><br>Thanks for the explanation! I still don't understand, however,<br>under<br>what set of assumptions this is generally valid. In the test case<br>showing the problem with the PPC backend, we have:<br><br> ...<br>.LBB0_6: # %if.end7.i37<br> lbz 4, 0(3)<br> li 30, 1<br> cmplwi 0, 4, 0<br> beq 0, .LBB0_15<br># BB#7: # %if.then9.i39<br> bne 1, .LBB0_14<br> b .LBB0_15<br>.LBB0_8: # %if.end<br> ...<br><br> BB6<br> | \<br> | BB7<br> | / \<br> | / \<br>BB15 BB14<br><br>being transformed into:<br><br> ...<br>.LBB0_6: # %if.end7.i37<br> lbz 4, 0(3)<br> li 30, 1<br> cmplwi 0, 4, 0<br> bne 1, .LBB0_13<br> b .LBB0_14<br>.LBB0_7: # %if.end<br> ...<br><br>(adjusting for the later block numbering change)<br> BB6<br> | \<br> | \<br>BB15 BB14<br><br>but it did so without combining the conditions from BB6 and BB7<br>(but<br>instead just eliminated one).<br><br>I don't understand how this could be generally valid. It looks<br>like<br>what is happening is that, in the (current) broken state,<br>ScanInstructions returns true on BB7 (because it can predicate<br>the<br>unconditional branch, and it skips checking the conditional<br>branch),<br>and then tries to predicate BB7. BB7 has nothing in it, however,<br>except for its branches, and so nothing predicates. Now if the<br>"bne<br>1, .LBB0_14" in BB7 were actually predicated on the "(eq, cr0)"<br>value, then this would all make sense (but it is not, and can't<br>be<br>on this architecture). FWIW, PPCInstrInfo::PredicateInstruction<br>is<br>never called on the "bne 1, .LBB0_14" instruction (if it were,<br>then<br>this would assert instead of being a misompile).<br><br>Artyom, can you explain how the ARM equivalent of this leads to<br>correct code?<br><br>Evan, as I believe that you wrote the original code in question<br>--<br>6.5 years ago ;) -- perhaps you'll be able to lend some insight<br>here?<br><br>Thanks again,<br>Hal<br><br><br><br><br><br><br></blockquote><br>--<br>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br><br></blockquote><br>--<br>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<br></blockquote><br><br></blockquote><br>--<span class="Apple-converted-space"> </span><br>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory</div></blockquote></div><br></div></div></div></blockquote></div><br></body></html>