[llvm-commits] Sink improvements

[Duncan Sands]

Hi Carlo,

> Index: lib/Transforms/Scalar/Sink.cpp
> ===================================================================
> --- lib/Transforms/Scalar/Sink.cpp	(revisione 157154)
> +++ lib/Transforms/Scalar/Sink.cpp	(copia locale)
> @@ -174,6 +175,41 @@
>    return true;
>  }
>
> +bool Sinking::IsAcceptableTarget(Instruction *Inst, BasicBlock *SuccToSinkTo) const {

this should have a comment describing what it does.

> +  if (SuccToSinkTo == NULL || Inst == NULL)
> +    return false;

It looks like neither of these occurs, so how about getting rid of these or
turning it into an assertion instead.

> +
> +  // It is not possible to sink an instruction into its own block.  This can
> +  // happen with loops.
> +  if (Inst->getParent() == SuccToSinkTo)
> +    return false;
> +
> +  // If the block has multiple predecessors, this would introduce computation on
> +  // a path that it doesn't already exist.  We could split the critical edge,

that it doesn't already exist -> doesn't already exist

> +  // but for now we just punt.
> +  // FIXME: Split critical edges if not backedges.
> +  if (SuccToSinkTo->getUniquePredecessor() != Inst->getParent()) {
> +    // We cannot sink a load across a critical edge - there may be stores in
> +    // other code paths.
> +    if (!isSafeToSpeculativelyExecute(Inst))
> +      return false;
> +
> +    // We don't want to sink across a critical edge if we don't dominate the
> +    // successor. We could be introducing calculations to new code paths.
> +    if (!DT->dominates(Inst->getParent(), SuccToSinkTo))
> +      return false;
> +
> +    // Don't sink instructions into a loop.
> +    Loop *succ = LI->getLoopFor(SuccToSinkTo), *cur = LI->getLoopFor(Inst->getParent());
> +    if (succ != NULL && cur != NULL && succ != cur)

The LLVM style is to use 0 rather than NULL.  Likewise for all the other
instances of NULL you added elsewhere in the patch.

> +      return false;
> +  }
> +
> +  // Finally, check that we all the uses of the instruction are actually

that we all the -> that all the

> +  // dominated by the candidate
> +  return AllUsesDominatedByBlock(Inst, SuccToSinkTo);
> +}
> +
>  /// SinkInstruction - Determine whether it is safe to sink the specified machine
>  /// instruction out of its current block into a successor.
>  bool Sinking::SinkInstruction(Instruction *Inst,

The rest looks OK to me (except for the use of NULL).

Ciao, Duncan.