[LLVMdev] Asserts in bundleWithPred() and bundleWithSucc()

[Jakob Stoklund Olesen]

On Feb 4, 2013, at 8:59 AM, "Sergei Larin" <slarin at codeaurora.org> wrote:

> Jakob,
> 
>> The intention was to identify code that may have been converted from
>> the old style a little too quickly. I wanted to avoid bugs from a
>> global s/setIsInsideBundle/bundleWithPred/g search and replace.
> 
>  This is a good intent. Maybe a bit temporal but sound nevertheless.
> 
>> finalizeBundle is calling 'MIBundleBuilder Bundle(MBB, FirstMI,
>> LastMI)' which ought to work with pre-bundled instructions.
> 
> Not exactly.  Let me illustrate couple cases here (for illustration purposes
> "^" means "isBundledWithPred()" and "v" means "isBundledWithSucc()"):
> 
> I have the following (existing) bundle for which I want to regenerate the
> bundle header (You see that %R17 is not currently in the def list for the
> bundle header).
> 
> 
>  v  BUNDLE %P3<imp-def>, %R29<imp-use>, %D8<imp-use,kill>,
> %D9<imp-use,kill>, %R6<imp-use>
> *^v  STrid_indexed %R29, 80, %D8<kill>; mem:ST8[FixedStack2]
> *^v  STrid_indexed %R29, 72, %D9<kill>; mem:ST8[FixedStack3]
> *^v  %P3<def> = CMPEQri %R6, 0
> *^   %R17<def> = TFR_cdnNotPt %P3<internal>, %R1
>  v  BUNDLE %R29<imp-use>, %D10<imp-use,kill>, %R7<imp-use>, %D6<imp-use>
> (next bundle).
> 
> finalizeBundle() is called with:
> 
> FirstMI == STrid_indexed %R29, 80, %D8<kill>; mem:ST8[FixedStack2]
> LastMI == BUNDLE %R29<imp-use>, %D10<imp-use,kill>, %R7<imp-use>,
> %D6<imp-use>
> 
> From here this assert is triggered.

Yes, that sounds like correct behavior to me because FirstMI is pointing inside an existing bundle. I assume your intent is to remove the old BUNDLE header after finalizeBundle() inserted a new one. In this case you should either erase the old BUNDLE first, or unbundle it from the instructions you are trying to finalize.

>  Let me give you another example though. I have the following existing
> bundle:
> 
>    v  BUNDLE %R0<imp-def,dead>, %PC<imp-def>, %R18<imp-use>
>  *^v  %R0<def> = AND_ri %R18, 128
>  *^   JMP_EQriPnt_nv_V4 %R0<kill,internal>, 0, <BB#2>, %PC<imp-def>
> 
> I need to move an instruction into this bundle from another location and
> insert it _between_ AND_ri and JMP_EQriPnt_nv_V4. I use MBB->splice(...) to
> do that. Let's pretend that moved instruction was not bundled. New
> instruction is pointed to by MachineBasicBlock::instr_iterator MII. New
> bundle right after splice is:
> 
>    v  BUNDLE %R0<imp-def,dead>, %PC<imp-def>, %R18<imp-use>
>  *^v  %R0<def> = AND_ri %R18, 128
>  *    %R3<def> = HexagonEXTRACTU_ri_Inst %R18, 4, 3  <<<<<<<<<<<<<<<<<<<
> MII
>  *^   JMP_EQriPnt_nv_V4 %R0<kill,internal>, 0, <BB#2>, %PC<imp-def>
> 
> Now I have to integrate MII into the new bundle. As I do this:
> 
> MII->bundleWithPred();

The inconsistent flags should never happen. It should always be the case that:

if II->isBundledWithPred() then prior(II)->isBundledWithSucc()
if II->isBundledWithSucc() then next(II)->isBundledWithPRed()

In fact, the machine code verifier also checks this invariant now.

I changed MBB->splice() to not accept instr_iterators so what you're doing wouldn't be possible. It is hard to add enough assertions, though.

To move instructions into a bundle, I recommend that you use MIBundleBuilder with removeFromBundle():

  Bundle.insert(I, MI->removeFromBundle())

This way you can insert instructions at any position in a bundle without special casing the first and last positions. You can even have MIBundleBuilder represent an empty bundle.

MBB->splice() is ambiguous when given an insert position between two existing bundles. Did you mean to append to the first bundle, prepend to the second bundle, or did you want a stand-alone instruction between the bundles? MIBundleBuilder resolves that ambiguity.

/jakob