<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;">I find it extremely suspect that a patch to deprecate IT blocks for ARM required changes to the generic IfConverter pass at all.<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><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>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<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></body></html>