[llvm] r190309 - [ARMv8] Prevent generation of deprecated IT blocks on ARMv8 in Thumb mode.
Hal Finkel
hfinkel at anl.gov
Wed Dec 11 14:27:23 PST 2013
----- Original Message -----
> From: "Evan Cheng" <evan.cheng at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Jim Grosbach" <grosbach at apple.com>, "llvm-commits" <llvm-commits at cs.uiuc.edu>, "Bill Wendling"
> <isanbard at gmail.com>, "Artyom Skrobov" <Artyom.Skrobov at arm.com>
> Sent: Wednesday, December 11, 2013 2:03:24 PM
> Subject: Re: [llvm] r190309 - [ARMv8] Prevent generation of deprecated IT blocks on ARMv8 in Thumb mode.
>
>
> On Dec 10, 2013, at 4:38 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> > ----- Original Message -----
> >> From: "Evan Cheng" <evan.cheng at apple.com>
> >> To: "Artyom Skrobov" <Artyom.Skrobov at arm.com>
> >> Cc: "Jim Grosbach" <grosbach at apple.com>, "Hal Finkel"
> >> <hfinkel at anl.gov>, "llvm-commits" <llvm-commits at cs.uiuc.edu>,
> >> "Bill Wendling" <isanbard at gmail.com>
> >> Sent: Tuesday, December 10, 2013 6:18:14 PM
> >> Subject: Re: [llvm] r190309 - [ARMv8] Prevent generation of
> >> deprecated IT blocks on ARMv8 in Thumb mode.
> >>
> >>
> >>
> >>
> >> On Dec 10, 2013, at 4:13 PM, Artyom Skrobov <
> >> Artyom.Skrobov at arm.com
> >>> wrote:
> >>
> >>
> >> Jim, Evan et al:
> >>
> >> I want to remind that three weeks ago, I suggested a patch that
> >> restores IfConversion.cpp to its pre-r190309 state, and
> >> reimplements
> >> the change necessary for the IT blocks in ARMv8 in
> >> Thumb2InstrInfo.cpp instead.
> >> My patch is archived at
> >> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131118/196278.html
> >> , and Hal's regression test to go it with it, at
> >> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131125/196852.html
> >> However, committing this was delayed due to Hal's concern about
> >> skipping over the unpredicated conditional branches, in the
> >> pre-r190309 implementation , which may not always be valid.
> >>
> >>
> >> Can you work towards resolving this? It seems clear (unless I
> >> misunderstand) that r190309 is causing problems.
> >
> > r190309 does trigger the bugs in self-hosting on PowerPC, and
> > Artyom's fix does fix that problem without ARM performance
> > regressions (I assume). If nothing else, we should use it. I'm
> > concerned that the existing special case is also problematic (just
> > even less likely to trigger a bug), and I was hoping to understand
> > it better.
> >
> > Given that, as far as I can tell, you wrote the code in question,
> > can you explain the purpose of, in IfConverter::ScanInstructions:
> >
> > if (BBI.ClobbersPred && !isPredicated) {
> > // Predicate modification instruction should end the block
> > (except for
> > // already predicated instructions and end of block branches).
> > if (isCondBr) {
> > // A conditional branch is not predicable, but it may be
> > eliminated.
> > continue;
> > }
> >
> > It looks like this is a refactored version (r37543) of the logic
> > from r37518.
>
>
> It has been a long time. Let me see...
>
> This has to do with nested if-conversion (wired-and). When there are
> nested ifs, we can run into situation where a block is already
> predicated but its predicate can be subsumed. In this case, it's
> possible to if-convert the BB. However, the BB has to be fully
> predicated. If the BB has a compare and follow by unpredicated
> instructions, then it's no legal to if-convert it. The only
> exception is the end of the BB conditional branches since they will
> be eliminated.
Thanks! I think that I've figured this out: There is an error in PPCInstrInfo::SubsumesPredicate. I'm testing now...
Artyom and everyone else who helped with this, thank you very much!
-Hal
>
> Evan
>
> >
> > Thanks again,
> > Hal
> >
> >>
> >>
> >> Evan
> >>
> >>
> >>
> >>
> >> This is why the solution isn't as straightforward as just
> >> reverting
> >> r190309.
> >>
> >>
> >> On Tue Dec 10 22:52:16 GMT 2013, "Evan Cheng" <
> >> evan.cheng at apple.com
> >>> wrote:
> >>
> >>
> >>
> >>
> >>
> >> On Dec 10, 2013, at 11:59 AM, Jim Grosbach < grosbach at apple.com >
> >> wrote:
> >>
> >>
> >>
> >> I find it extremely suspect that a patch to deprecate IT blocks
> >> for
> >> ARM required changes to the generic IfConverter pass at all.
> >>
> >> I agree. This is very suspicious.
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> The symptoms here seem very similar to the earlier problem I
> >> referred
> >> to, which was r187071.
> >>
> >>
> >>
> >>
> >> On Dec 10, 2013, at 11:45 AM, Hal Finkel < hfinkel at anl.gov >
> >> wrote:
> >>
> >>
> >>
> >> Jim,
> >>
> >> Thanks! If you have another minute, let me quickly summarize my
> >> current understanding:
> >>
> >> In IfConverter::ScanInstructions, pre-r190309, we had the
> >> following
> >> special case:
> >>
> >> if (BBI.ClobbersPred && !isPredicated) {
> >> // Predicate modification instruction should end the block (except
> >> for
> >> // already predicated instructions and end of block branches).
> >> if (isCondBr) {
> >> // A conditional branch is not predicable, but it may be
> >> eliminated.
> >> continue;
> >> }
> >>
> >> // Predicate may have been modified, the subsequent (currently)
> >> // unpredicated instructions cannot be correctly predicated.
> >> BBI.IsUnpredicable = true;
> >> return;
> >> }
> >>
> >> ...
> >>
> >> if (!TII->isPredicable(I)) {
> >> BBI.IsUnpredicable = true;
> >> return;
> >> }
> >>
> >> 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:
> >>
> >> if (CvtBBI->BB->pred_size() > 1) {
> >> ...
> >> } else {
> >> // Predicate the 'true' block after removing its branch.
> >> CvtBBI->NonPredSize -= TII->RemoveBranch(*CvtBBI->BB);
> >> PredicateBlock(*CvtBBI, CvtBBI->BB->end(), Cond, Redefs);
> >>
> >> // Now merge the entry of the triangle with the true block.
> >> BBI.NonPredSize -= TII->RemoveBranch(*BBI.BB);
> >> MergeBlocks(BBI, *CvtBBI, false);
> >> }
> >>
> >> 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:
> >>
> >>
> >>
> >>
> >> That change sounds really questionable, even for ARM.
> >>
> >> 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?
> >>
> >>
> >> Evan
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> ...
> >> .LBB0_6: # %if.end7.i37
> >> lbz 4, 0(3)
> >> li 30, 1
> >> cmplwi 0, 4, 0
> >> beq 0, .LBB0_15
> >> # BB#7: # %if.then9.i39
> >> bne 1, .LBB0_14
> >> b .LBB0_15
> >> .LBB0_8: # %if.end
> >> ...
> >>
> >> BB6
> >> | \
> >> | BB7
> >> | / \
> >> | / \
> >> BB15 BB14
> >>
> >> being transformed into:
> >>
> >> ...
> >> .LBB0_6: # %if.end7.i37
> >> lbz 4, 0(3)
> >> li 30, 1
> >> cmplwi 0, 4, 0
> >> bne 1, .LBB0_13
> >> b .LBB0_14
> >> .LBB0_7: # %if.end
> >> ...
> >>
> >> (adjusting for the later block numbering change)
> >> BB6
> >> | \
> >> | \
> >> BB15 BB14
> >>
> >> 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).
> >>
> >>
> >>
> >> Ouch. That’s really nasty. :(
> >>
> >>
> >>
> >>
> >> 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.
> >>
> >>
> >>
> >> 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.
> >>
> >>
> >> -Jim
> >>
> >>
> >>
> >>
> >> -Hal
> >>
> >> ----- Original Message -----
> >>
> >>
> >> From: "Jim Grosbach" < grosbach at apple.com >
> >> To: "Hal Finkel" < hfinkel at anl.gov >
> >> Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu >, "Bill Wendling" <
> >> isanbard at gmail.com >, "Artyom Skrobov"
> >> < Artyom.Skrobov at arm.com >
> >> Sent: Tuesday, December 10, 2013 12:47:23 PM
> >> Subject: Re: [llvm] r190309 - [ARMv8] Prevent generation of
> >> deprecated IT blocks on ARMv8 in Thumb mode.
> >>
> >> Hi Hal,
> >>
> >> I’m not very familiar with the patch this thread is in response
> >> to,
> >> so I can’t comment very effectively on that aspect of things
> >> directly.
> >>
> >> Your comment below about multiple branch instructions at the end
> >> of a
> >> basic block, however, is correct. The IfConversion pass needs to
> >> handle those sorts of cases, including degenerate cases where
> >> AnalyzeBranch() fails (by punting and not doing if-conversion of
> >> that).
> >>
> >> There are helper functions in the ARM backend (and I assume
> >> others)
> >> which deal with getting the inverted condition code for
> >> if-converting if/else constructs. Perhaps there’s something wrong
> >> there either in the general pass or the PPC backend? I seem to
> >> recall there was a nasty bug in the ARM version a while back that
> >> was painful to track down.
> >>
> >> -Jim
> >>
> >> On Dec 10, 2013, at 10:24 AM, Hal Finkel < hfinkel at anl.gov >
> >> wrote:
> >>
> >>
> >>
> >> Jim, as you probably saw, Bill recommended you as someone who
> >> could
> >> help clear up the confusion here. I would appreciate your input; I
> >> need to get this cleared up this week (given our current release
> >> schedule).
> >>
> >> Thanks in advance,
> >> Hal
> >>
> >> ----- Original Message -----
> >>
> >>
> >> From: "Hal Finkel" < hfinkel at anl.gov >
> >> To: "Artyom Skrobov" < Artyom.Skrobov at arm.com >
> >> Cc: llvm-commits at cs.uiuc.edu
> >> Sent: Tuesday, December 3, 2013 11:02:29 AM
> >> Subject: Re: [llvm] r190309 - [ARMv8] Prevent generation of
> >> deprecated IT blocks on ARMv8 in Thumb mode.
> >>
> >> ----- Original Message -----
> >>
> >>
> >> From: "Artyom Skrobov" < Artyom.Skrobov at arm.com >
> >> To: "Hal Finkel" < hfinkel at anl.gov >
> >> Cc: "Joey Gouly" < Joey.Gouly at arm.com >, llvm-commits at cs.uiuc.edu
> >> ,
> >> "Tim Northover" < t.p.northover at gmail.com >, "Evan
> >> Cheng" < evan.cheng at apple.com >
> >> Sent: Monday, December 2, 2013 12:23:27 PM
> >> Subject: RE: [llvm] r190309 - [ARMv8] Prevent generation of
> >> deprecated IT blocks on ARMv8 in Thumb mode.
> >>
> >> Hal, could you please clarify why the PPC backend produces
> >> BasicBlocks with several branch instructions sharing the same BB?
> >>
> >> The doxygen comments in LLVM imply that a BB is a "single entry,
> >> single exit" sequence, and any branch instruction in a BB must
> >> terminate it.
> >>
> >> That seems mostly right, but a block can have multiple terminators
> >> (if one is conditional and one is unconditional), and the
> >> unconditional branch may be elided if its destination is the next
> >> block in layout ordering. Taking a quick look at AnalyzeBranch in
> >> the various backends, I believe that this is true also on X86,
> >> Hexagon, Sparc, SystemZ, etc. This allows forming a TBB/FBB pair
> >> using two consecutive branches.
> >>
> >> Is this somehow incompatible with the design of the if conversion
> >> pass?
> >>
> >> Thanks again,
> >> Hal
> >>
> >>
> >>
> >>
> >> As you can see in the code examples I posted earlier, this is
> >> indeed
> >> the case with the ARM backend.
> >>
> >>
> >> -----Original Message-----
> >> From: Hal Finkel [ mailto:hfinkel at anl.gov ]
> >> Sent: 28 November 2013 19:50
> >> To: Artyom Skrobov
> >> Cc: Joey Gouly; llvm-commits at cs.uiuc.edu ; Tim Northover; Evan
> >> Cheng
> >> Subject: Re: [llvm] r190309 - [ARMv8] Prevent generation of
> >> deprecated IT blocks on ARMv8 in Thumb mode.
> >>
> >>
> >> Thanks for the explanation! I still don't understand, however,
> >> under
> >> what set of assumptions this is generally valid. In the test case
> >> showing the problem with the PPC backend, we have:
> >>
> >> ...
> >> .LBB0_6: # %if.end7.i37
> >> lbz 4, 0(3)
> >> li 30, 1
> >> cmplwi 0, 4, 0
> >> beq 0, .LBB0_15
> >> # BB#7: # %if.then9.i39
> >> bne 1, .LBB0_14
> >> b .LBB0_15
> >> .LBB0_8: # %if.end
> >> ...
> >>
> >> BB6
> >> | \
> >> | BB7
> >> | / \
> >> | / \
> >> BB15 BB14
> >>
> >> being transformed into:
> >>
> >> ...
> >> .LBB0_6: # %if.end7.i37
> >> lbz 4, 0(3)
> >> li 30, 1
> >> cmplwi 0, 4, 0
> >> bne 1, .LBB0_13
> >> b .LBB0_14
> >> .LBB0_7: # %if.end
> >> ...
> >>
> >> (adjusting for the later block numbering change)
> >> BB6
> >> | \
> >> | \
> >> BB15 BB14
> >>
> >> but it did so without combining the conditions from BB6 and BB7
> >> (but
> >> instead just eliminated one).
> >>
> >> I don't understand how this could be generally valid. It looks
> >> like
> >> what is happening is that, in the (current) broken state,
> >> ScanInstructions returns true on BB7 (because it can predicate
> >> the
> >> unconditional branch, and it skips checking the conditional
> >> branch),
> >> and then tries to predicate BB7. BB7 has nothing in it, however,
> >> except for its branches, and so nothing predicates. 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). FWIW, PPCInstrInfo::PredicateInstruction
> >> is
> >> never called on the "bne 1, .LBB0_14" instruction (if it were,
> >> then
> >> this would assert instead of being a misompile).
> >>
> >> Artyom, can you explain how the ARM equivalent of this leads to
> >> correct code?
> >>
> >> Evan, as I believe that you wrote the original code in question
> >> --
> >> 6.5 years ago ;) -- perhaps you'll be able to lend some insight
> >> here?
> >>
> >> Thanks again,
> >> Hal
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> --
> >> Hal Finkel
> >> Assistant Computational Scientist
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >>
> >> --
> >> Hal Finkel
> >> Assistant Computational Scientist
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> >>
> >>
> >>
> >> --
> >> Hal Finkel
> >> Assistant Computational Scientist
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> >>
> >>
> >>
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
>
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list