[llvm-commits] [PATCH] Fix PR15031 (Correct flaw in CriticalAntiDepBreaker::BreakAntiDependencies)

[Bill Schmidt]

On Thu, 2013-01-24 at 13:06 -0800, David Blaikie wrote:
> On Thu, Jan 24, 2013 at 12:24 PM, Bill Schmidt
> <wschmidt at linux.vnet.ibm.com> wrote:
> > On Thu, 2013-01-24 at 18:10 -0200, Adhemerval Zanella wrote:
> >> On 01/24/2013 03:59 PM, Bill Schmidt wrote:
> >> > This patch addresses bug 15031.
> >> >
> >> > The common code in the post-RA scheduler to break anti-dependencies on the
> >> > critical path contained a flaw.  In the reported case, an anti-dependency
> >> > between the overlapping registers %X4 and %R4 exists:
> >> >
> >> >     %X29<def> = OR8 %X4, %X4
> >> >     %R4<def>, %X3<def,dead,tied3> = LBZU 1, %X3<kill,tied1>
> >> >
> >> > The unpatched code breaks the dependency by replacing %R4 and its uses
> >> > with %R3, the first register on the available list.  However, %R3 and
> >> > %X3 overlap, so this creates two overlapping definitions on the same
> >> > instruction.
> >> >
> >> > The fix is straightforward, preventing selection of a register that
> >> > overlaps any other defined register on the same instruction.
> >> >
> >> > The test case is reduced from the bug report, and verifies that we no
> >> > longer produce "lbzu 3, 1(3)" when breaking this anti-dependency.
> >> >
> >> > Is this OK to commit?
> >> >
> >> > Thanks!
> >>
> >> The patch looks ok, just two small suggestions:
> >>
> >>
> >>        continue;
> >> +    // If NewReg overlaps any of the forbidden registers, we can't use it.
> >> +    bool Forbidden = false;
> >> +    for (std::vector<unsigned>::iterator it = Forbid.begin();
> >> +         it != Forbid.end(); ++it)
> >> +      if (TRI->regsOverlap(NewReg, *it)) {
> >> +        Forbidden = true;
> >> +        break;
> >> +      }
> >> +    if (Forbidden) continue;
> >>      return NewReg;
> >>    }
> >>
> >> Avoid to evaluate end() at very time, use something like:
> >>
> >> for (std::vector<unsigned>::iterator it = Forbid.begin(), ite = Forbid.end();
> >>      it != ite; ++it)
> >>
> >> Also, since the std::vector is for unsigned and has a small size I think SmallVector
> >> could be a better joice.
> >>
> >
> > Good points.  The vector will almost always be of size 0 or 1, so a
> > SmallVector is indeed better...
> 
> That seems like an unfortunate thing to use a vector for - why not Optional?

Well, it's "almost always" of size 0 or 1.  The size of the vector is
equal to the number of definitions of an instruction that don't overlap
a particular register.  I don't think I can make an assumption about the
number of registers an instruction might define, hence the use of a
vector.  I did allocate the vector using the default that assumes it
will have size zero, which is the most typical case.

Bill

> 
> >
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>