[llvm-commits] [llvm] r91184 - in /llvm/trunk: lib/Transforms/Scalar/ScalarReplAggregates.cpp test/Transforms/ScalarRepl/2009-12-11-NeonTypes.ll

[Chris Lattner]

On Dec 11, 2009, at 3:47 PM, Bob Wilson wrote:

> Author: bwilson
> Date: Fri Dec 11 17:47:40 2009
> New Revision: 91184
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=91184&view=rev
> Log:
> Revise scalar replacement to be more flexible about handle bitcasts and GEPs.
> While scanning through the uses of an alloca, keep track of the current offset
> relative to the start of the alloca, and check memory references to see if
> the offset & size correspond to a component within the alloca.  This has the
> nice benefit of unifying much of the code from isSafeUseOfAllocation,
> isSafeElementUse, and isSafeUseOfBitCastedAllocation.  The code to rewrite
> the uses of a promoted alloca, after it is determined to be safe, is
> reorganized in the same way.
> 
> Also, when rewriting GEP instructions, mark them as "in-bounds" since all the
> indices are known to be safe.

Very nice!  This is a great improvement over the old code.  Here are a couple of thoughts:

> +/// FindElementAndOffset - Return the index of the element containing Offset
> +/// within the specified type, which must be either a struct or an array.
> +/// Sets T to the type of the element and Offset to the offset within that
> +/// element.
> +unsigned SROA::FindElementAndOffset(const Type *&T, uint64_t &Offset) {
> +  unsigned Idx = 0;
> +  if (const StructType *ST = dyn_cast<StructType>(T)) {
> +    const StructLayout *Layout = TD->getStructLayout(ST);
> +    Idx = Layout->getElementContainingOffset(Offset);
> +    T = ST->getContainedType(Idx);
> +    Offset -= Layout->getElementOffset(Idx);

Please early exit here to unnest the else.

> +  } else {
> +    const ArrayType *AT = dyn_cast<ArrayType>(T);
> +    assert(AT && "unexpected type for scalar replacement");

Please use cast<> if you know that it has to be an array.  However, is this really true?  Can't it be a vector?  If so, use SequentialType and explicitly assert it isn't a pointer.


> +    T = AT->getElementType();
> +    uint64_t EltSize = TD->getTypeAllocSize(T);
> +    Idx = (unsigned)(Offset / EltSize);

The returned Idx should be a uint64_t for large indexes into large arrays.



> +/// isSafeForScalarRepl - Check if instruction I is a safe use with regard to
> +/// performing scalar replacement of alloca AI.  The results are flagged in
> +/// the Info parameter.  Offset and ArrayOffset indicate the position within
> +/// AI that is referenced by this instruction.
> +void SROA::isSafeForScalarRepl(Instruction *I, AllocaInst *AI, uint64_t Offset,
> +                               uint64_t ArrayOffset, AllocaInfo &Info) {

I'm not really sure what the difference between ArrayOffset and Offset are.  Later I see:

>   For the
> +/// special case of a variable index to a 2-element array, ArrayOffset is set
> +/// to the array element size.

But what is the semantic of ArrayOffset at this level?  What does it mean and how does it impact safety, what is the relation between it and Offset?  I think the code handles it correctly, but the comment needs to be improved to explain these points.

> +/// isSafeGEP - Check if a GEP instruction can be handled for scalar
> +/// replacement.  It is safe when all the indices are constant, in-bounds
> +/// references, and when the resulting offset corresponds to an element within
> +/// the alloca type.  The results are flagged in the Info parameter.  Upon
> +/// return, Offset is adjusted as specified by the GEP indices.  For the
> +/// special case of a variable index to a 2-element array, ArrayOffset is set
> +/// to the array element size.
> +void SROA::isSafeGEP(GetElementPtrInst *GEPI, AllocaInst *AI,
> +                     uint64_t &Offset, uint64_t &ArrayOffset,
> +                     AllocaInfo &Info) {
> +  gep_type_iterator GEPIt = gep_type_begin(GEPI), E = gep_type_end(GEPI);
> +  if (GEPIt == E)
> +    return;
> +
> +  // The first GEP index must be zero.
> +  if (!isa<ConstantInt>(GEPIt.getOperand()) ||
> +      !cast<ConstantInt>(GEPIt.getOperand())->isZero())
> +    return MarkUnsafe(Info);
> +  if (++GEPIt == E)
> +    return;

With your rewrite, I think this constraint could be relaxed, allowing non-zero constant ints as the first index.  SRoA should be able to handle stuff like this pretty easily:

  A = alloca { i32, [4 x i32] }
  B = gep A, 0, 1, 0
  C = gep B, 2

>   // If the first index is a non-constant index into an array, see if we can
>   // handle it as a special case.
> +  const Type *ArrayEltTy = 0;
> +  if (ArrayOffset == 0 && Offset == 0) {
> +    if (const ArrayType *AT = dyn_cast<ArrayType>(*GEPIt)) {
> +      if (!isa<ConstantInt>(GEPIt.getOperand())) {

Please check the !isa<ConstantInt> as the first predicate for this big "if" block.

> +        uint64_t NumElements = AT->getNumElements();
> +
> +        // If this is an array index and the index is not constant, we cannot
> +        // promote... that is unless the array has exactly one or two elements
> +        // in it, in which case we CAN promote it, but we have to canonicalize
> +        // this out if this is the only problem.
> +        if ((NumElements != 1 && NumElements != 2) || !AllUsersAreLoads(GEPI))
> +          return MarkUnsafe(Info);
>         Info.needsCleanup = true;
> +        ArrayOffset = TD->getTypeAllocSizeInBits(AT->getElementType());
> +        ArrayEltTy = AT->getElementType();
> +        ++GEPIt;

Actually, on reflection, I'm not sure that this transformation is safe without more checking.  Consider indexing into the array with a variable index in a type like {[2 x i32], i32, i32}.  SRoA shouldn't try to canonicalize this because the index might be '3' (even though that is gross).

I'm not really sure how useful "variable indexes in two element arrays" really is anyway though.  It might be better to just remove this transformation.  That would simplify the code quite a bit too.

> @@ -612,144 +494,254 @@
>       // of any accesses into structs where any of the components are variables.
>       if (IdxVal->getZExtValue() >= AT->getNumElements())
>         return MarkUnsafe(Info);
> -    } else if (const VectorType *VT = dyn_cast<VectorType>(*I)) {
> +    } else {
> +      const VectorType *VT = dyn_cast<VectorType>(*GEPIt);
> +      assert(VT && "unexpected type in GEP type iterator");

Please use cast<VectorType>  instead of dyn_cast + assert.  The cast<> turns into a noop in non-assert builds.

> +/// RewriteForScalarRepl - Alloca AI is being split into NewElts, so rewrite
> +/// the instruction I, which references it, to use the separate elements.
> +/// Offset indicates the position within AI that is referenced by this
> +/// instruction.
> +void SROA::RewriteForScalarRepl(Instruction *I, AllocaInst *AI, uint64_t Offset,
> +                                SmallVector<AllocaInst*, 32> &NewElts) {
> +  for (Value::use_iterator UI = I->use_begin(), E = I->use_end(); UI != E; ) {
> +    Instruction *User = cast<Instruction>(*UI++);
> 
> +    if (BitCastInst *BC = dyn_cast<BitCastInst>(User)) {
> +      if (BC->getOperand(0) == AI)
> +        BC->setOperand(0, NewElts[0]);

What does this setOperand do?  If the invariant is that RewriteForScalarRepl leaves I with zero uses after  the transformation, then this isn't needed.  If I does have uses, this seems like potentially the wrong thing to do.  At the very least, this would benefit from a comment.

> +      // If the bitcast type now matches the operand type, it will be removed
> +      // after processing its uses.
> +      RewriteForScalarRepl(BC, AI, Offset, NewElts);
> +    } else if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(User)) {
> +      RewriteGEP(GEPI, AI, Offset, NewElts);
> +    } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(User)) {
> +      ConstantInt *Length = dyn_cast<ConstantInt>(MI->getLength());
> +      uint64_t MemSize = Length->getZExtValue();
> +      if (Offset == 0 &&
> +          MemSize == TD->getTypeAllocSize(AI->getAllocatedType()))
> +        RewriteMemIntrinUserOfAlloca(MI, I, AI, NewElts);

What is the 'else' case here?  If Offset != 0 it seems like a failure of the safety checking code?  Should the 'if' be turned into an assert?
> 
> +  // Delete unused instructions and identity bitcasts.
> +  if (I->use_empty())
> +    I->eraseFromParent();
> +  else if (BitCastInst *BC = dyn_cast<BitCastInst>(I)) {
> +    if (BC->getDestTy() == BC->getSrcTy()) {
> +      BC->replaceAllUsesWith(BC->getOperand(0));
> +      BC->eraseFromParent();

This code (zapping identity bitcasts) seems both unneeded and harmful: this will add another use of the parent instruction whose use list is being iterated over.  Was this something that existed before or did you add it?  It is probably best to just remove this.

-Chris