[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>
> Sent: Thursday, November 28, 2013 8:23:58 AM
> Subject: RE: [llvm] r190309 - [ARMv8] Prevent generation of deprecated IT	blocks	on ARMv8 in Thumb mode.
> 
> Hello Hal,
> 
> >>>> Hal, can you confirm that this patch restores the correct
> >>>> functioning
> >>>> on PPC?
> >>>
> >>> I'll check.
> >>>
> >>> Why are you restoring this part?
> >>>
> >>>      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;
> >>> +      }
> >>> +
> >> 
> >> This enables conversion of fragments such as
> >> 
> >>         cmp     r12, r2
> >>         bne     .LBB6_4
> >> @ BB#2:
> >>         cmp     r1, r3
> >>         bne     .LBB6_4
> >>         strexd  r5, lr, r4, [r0]
> >> 
> >> into
> >> 
> >>         cmp     r12, r2
> >>         it      eq
> >>         cmpeq   r1, r3
> >>         bne     .LBB6_4
> >> @ BB#2:
> >>         strexd  r5, lr, r4, [r0]
> >> 
> >> As you can see, in this case, the last instruction of the branch
> >> (cmp
> >> r1, r3; bne .LBB6_4) is not predicable on the branch condition
> >> (the
> >> original CPSR), but it's going to be eliminated during the
> >> conversion, so its non-predicability doesn't matter.
> >
> > Can you explain what this means? I still obviously see those
> > instructions in the transformation result.
> 
> I'll do my best.
> 
> The source construct here is of the form
> 
>          BB1
>         /  |
>      BB2   |
>     /   \  |
>   BB3    BB4
> 
> --corresponding to if(cond1 && cond2) {BB3} else {BB4}
> 
> ScanInstructions() is called on BB2 to check if it can be predicated
> and merged into BB1.
> Because of the clause with "if(isCondBr)", ScanInstructions() skips
> over the final branch instruction of BB2, and returns with
> IsUnpredicable=false, indicating that the conversion is possible.
> 
> This allows IfConverter::AnalyzeBlock() to recognize the construct as
> a valid ICTriangle, and IfConverter::IfConvertTriangle() is called
> to perform the actual IfConversion.
> If you scroll the source of IfConverter::IfConvertTriangle() down, to
> the comment
> "// Predicate the 'true' block after removing its branch.",
> you will see the unpredicable final branch of BB2 getting removed, as
> well as the final branch of BB1.
> Following this, the two BB's are merged, and a new branch to BB4 is
> created to terminate the merged BB.
> 
> If the clause with "if(isCondBr)" is removed, then none of this
> conversion is possible for the shown example.
>  
> Hope this explanation helps.
> 
> 

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