[llvm] r216023 - IR: Fix a missed case when threading OnlyIfReduced through ConstantExpr

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Sep-11, at 14:44, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Aug 19, 2014 at 2:18 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> Author: dexonsmith
> Date: Tue Aug 19 16:18:21 2014
> New Revision: 216023
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=216023&view=rev
> Log:
> IR: Fix a missed case when threading OnlyIfReduced through ConstantExpr
> 
> In r216015 I missed propagating `OnlyIfReduced` through the inline
> versions of `getGetElementPtr()` (I was relying on compile failures on
> mismatches between the header and source signatures to get them all).
> 
> Modified:
>     llvm/trunk/include/llvm/IR/Constants.h
>     llvm/trunk/unittests/IR/ConstantsTest.cpp
> 
> Modified: llvm/trunk/include/llvm/IR/Constants.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Constants.h?rev=216023&r1=216022&r2=216023&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/Constants.h (original)
> +++ llvm/trunk/include/llvm/IR/Constants.h Tue Aug 19 16:18:21 2014
> @@ -1036,9 +1036,9 @@ public:
>    static Constant *getGetElementPtr(Constant *C, ArrayRef<Constant *> IdxList,
>                                      bool InBounds = false,
>                                      Type *OnlyIfReducedTy = nullptr) {
> -    return getGetElementPtr(C, makeArrayRef((Value * const *)IdxList.data(),
> -                                            IdxList.size()),
> -                            InBounds);
> +    return getGetElementPtr(
> +        C, makeArrayRef((Value * const *)IdxList.data(), IdxList.size()),
> +        InBounds, OnlyIfReducedTy);
>    }
>    static Constant *getGetElementPtr(Constant *C, Constant *Idx,
>                                      bool InBounds = false,
> @@ -1046,7 +1046,7 @@ public:
>      // This form of the function only exists to avoid ambiguous overload
>      // warnings about whether to convert Idx to ArrayRef<Constant *> or
>      // ArrayRef<Value *>.
> -    return getGetElementPtr(C, cast<Value>(Idx), InBounds);
> +    return getGetElementPtr(C, cast<Value>(Idx), InBounds, OnlyIfReducedTy);
>    }
>    static Constant *getGetElementPtr(Constant *C, ArrayRef<Value *> IdxList,
>                                      bool InBounds = false,
> 
> Modified: llvm/trunk/unittests/IR/ConstantsTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/ConstantsTest.cpp?rev=216023&r1=216022&r2=216023&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/IR/ConstantsTest.cpp (original)
> +++ llvm/trunk/unittests/IR/ConstantsTest.cpp Tue Aug 19 16:18:21 2014
> @@ -322,5 +322,30 @@ TEST(ConstantsTest, ConstantExprReplaceW
>    ASSERT_EQ(Int2, Ref->getInitializer());
>  }
> 
> +TEST(ConstantsTest, GEPReplaceWithConstant) {
> +  LLVMContext Context;
> +  std::unique_ptr<Module> M(new Module("MyModule", Context));
> +
> +  Type *IntTy = Type::getInt32Ty(Context);
> +  Type *PtrTy = PointerType::get(IntTy, 0);
> +  auto *C1 = ConstantInt::get(IntTy, 1);
> +  auto *Placeholder = new GlobalVariable(
> +      *M, IntTy, false, GlobalValue::ExternalWeakLinkage, nullptr);
> +  auto *GEP = ConstantExpr::getGetElementPtr(Placeholder, C1);
> +  ASSERT_EQ(GEP->getOperand(0), Placeholder);
> +
> +  auto *Ref =
> +      new GlobalVariable(*M, PtrTy, false, GlobalValue::ExternalLinkage, GEP);
> +  ASSERT_EQ(GEP, Ref->getInitializer());
> +
> +  auto *Global = new GlobalVariable(*M, PtrTy, false,
> +                                    GlobalValue::ExternalLinkage, nullptr);
> +  auto *Alias = GlobalAlias::create(IntTy, 0, GlobalValue::ExternalLinkage,
> +                                    "alias", Global, M.get());
> 
> *casts Lesser Thread Necromancy*
> 
> So... this alias is bad. The Global is of type i32*, but the alias is of type i32 - types don't match. Unfortunately there's not (currently) any assertion in GlobalAlias's ctor/factory to ensure this, but it would fail the Verifier around Verifier.cpp:603
> 
> If I correct the GlobalAlias's type, then it doesn't match the Placeholder which it's RAUW'd with.
> 
> Any idea which parts of this need to remain in tact for the test to be valuable?

I don't understand how that even compiles.  Looking at my tree,
`GlobalAlias::create()` takes `PointerType*` as it's first argument.
I'm confused :/.

Anyway, the point was to provide explicit coverage for RAUW hitting
this code in `ConstantExpr::getGetElementPtr()`:
--
  Type *DestTy = GetElementPtrInst::getIndexedType(Ty, Idxs);
  assert(DestTy && "GEP indices invalid!");
  unsigned AS = C->getType()->getPointerAddressSpace();
  Type *ReqTy = DestTy->getPointerTo(AS);
  if (VectorType *VecTy = dyn_cast<VectorType>(C->getType()))
    ReqTy = VectorType::get(ReqTy, VecTy->getNumElements());

  if (OnlyIfReducedTy == ReqTy) // <-- this is the key code
    return nullptr;             // <-- (and this)
--
and thereby not recreating (and RAUW-ing) the GEP.

What's happening is that we've RAUW'ed a constant, and all its users
that are also constants need to be re-canonicalized, and possibly
RAUW'ed.  `Constant::handleOperandChange()` gets called on each of
these constant users.  For GEP, this forks out to
`ConstantExpr::handleOperandChangeImpl()`, which calls
`ConstantExpr::getWithOperands(..., /*OnlyIfReduced*/ true)`.  This
function should return a new `ConstantExpr` if and *only* if it's
a different result from updating the operand in place.

(Why?  It's a micro-optimization, since this reduces malloc traffic,
and prevents long, unnecessary RAUW chains.  My driving goal at the
time was to make use-list order easily predictable (RAUWs shuffle
use-lists).)

As long as the test covers the `return nullptr` logic -- we RAUW'ed
a GEP operand and should mutate the GEP in place -- then it's still
covering the right thing.  You can check this by deleting the early
return and checking that the unit test fails.  (The only other
failing tests should be `verify-uselistorder` tests.)

(I keep thinking that the key check above should be simpler:
--
if (OnlyIfReducedTy) {
  assert(OnlyIfReducedTy == ReqTy && "RAUW cannot mutate types!");
  return nullptr;
}
--
But I seem to remember trying that, and being wrong.  Maybe not?)

> Should we change the Placeholder type to match the GlobalAlias and Globals type?
> Should we add a bitcast of the Global when constructing the GlobalAlias?
> Something else?

The RAUW'ed object needs to be a direct operand of the GEP.  I
think the types don't matter here (aside from making the IR valid).

> (this came up while trying to fix/migrate aliases to take explicit pointee types & using an assertion to ensure I didn't foul up the migration)

I wonder about the end state with the explicit pointee type work.
Maybe the `Type` can also be mutate

> +  Placeholder->replaceAllUsesWith(Alias);
> +  ASSERT_EQ(GEP, Ref->getInitializer());
> +  ASSERT_EQ(GEP->getOperand(0), Alias);
> +}
> +
>  }  // end anonymous namespace
>  }  // end namespace llvm
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>