[llvm] r190309 - [ARMv8] Prevent generation of deprecated IT blocks on ARMv8 in Thumb mode.

[Hal Finkel]

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