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

[Sergei Larin]

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:

void MachineInstr::bundleWithSucc() {
  assert(!isBundledWithSucc() && "MI is already bundled with its
successor"); <<<<<<<<<<<<<<<<<<<<<<<<
  setFlag(BundledSucc);
  MachineBasicBlock::instr_iterator Succ = this;
  ++Succ;
  assert(!Succ->isBundledWithPred() && "Inconsistent bundle flags");
  Succ->setFlag(BundledPred);
}


Here is the call stack:

#3  ... in llvm::MachineInstr::bundleWithSucc (this=0x4c6aa20) at
...lib/CodeGen/MachineInstr.cpp:882
#4  ... in llvm::MIBundleBuilder::insert (this=0x7fffffff7dc0, I=...,
MI=0x4c6aa20) at ...include/llvm/CodeGen/MachineInstrBuilder.h:417
#5  ... in llvm::MIBundleBuilder::prepend (this=0x7fffffff7dc0,
MI=0x4c6aa20) at ...include/llvm/CodeGen/MachineInstrBuilder.h:435
#6  ... in llvm::finalizeBundle (MBB=..., FirstMI=..., LastMI=...) at
...lib/CodeGen/MachineInstrBundle.cpp:112



  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();

I run into this assert:

void MachineInstr::bundleWithPred() {
  assert(!isBundledWithPred() && "MI is already bundled with its
predecessor");
  setFlag(BundledPred);
  MachineBasicBlock::instr_iterator Pred = this;
  --Pred;
  assert(!Pred->isBundledWithSucc() && "Inconsistent bundle flags");
<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  Pred->setFlag(BundledSucc);
}


Now think about what I need to do for all possible cases when original
instruction was previously bundled and that bundle also needs updating....
and on top of that almost every time I call bundleWith*() I have to guard it
with the check for isBundledWith*(). The code looks rather ugly at that
point...

...and if I start dissolve and reconstruct bundles every time I need to
manipulate them, I think original intent becomes a bit overly
constraining... 

Sergei




---
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation


> -----Original Message-----
> From: Jakob Stoklund Olesen [mailto:stoklund at 2pi.dk]
> Sent: Friday, February 01, 2013 6:15 PM
> To: Sergei Larin
> Cc: llvmdev at cs.uiuc.edu
> Subject: Re: Asserts in bundleWithPred() and bundleWithSucc()
> 
> 
> On Feb 1, 2013, at 3:43 PM, "Sergei Larin" <slarin at codeaurora.org>
> wrote:
> 
> > I have a question about the following (four) asserts recently added
> in
> > bundleWithPred() and bundleWithSucc() (see below). What is the real
> > danger of reasserting a connection even if it already exist?
> 
> 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.
> 
> > My problem with them
> > happens when I try to call finalizeBundle() on an existing bundle to
> > which I have added a new instruction. The goal - a new bundle header
> > with liveness abbreviation, but because of these asserts I now have
> to
> > unbundle all, and re-bundle them right back again for no obvious
> benefit...
> 
> finalizeBundle is calling 'MIBundleBuilder Bundle(MBB, FirstMI,
> LastMI)' which ought to work with pre-bundled instructions. FirstMI and
> LastMI must be pointing at bundle boundaries, but you shouldn't need to
> unbundle everything.
> 
> Which iterators are you passing to finalizeBundle?
> 
> /jakob