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

[David Blaikie via llvm-commits]

Forward to the right mailing list.
---------- Forwarded message ----------
From: David Blaikie <dblaikie at gmail.com>
Date: Fri, Sep 11, 2015 at 2:44 PM
Subject: Re: [llvm] r216023 - IR: Fix a missed case when threading
OnlyIfReduced through ConstantExpr
To: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
Cc: "llvm-commits at cs.uiuc.edu" <llvm-commits at cs.uiuc.edu>




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

(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)


> +  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150911/aff4574f/attachment-0001.html>