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

[Hal Finkel]

----- 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