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

[David Blaikie]

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?

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