[llvm] r216023 - IR: Fix a missed case when threading OnlyIfReduced through ConstantExpr
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 11 15:55:29 PDT 2015
On Fri, Sep 11, 2015 at 3:40 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > 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 :/.
>
Yeah, that changed (in fact, I changed it) in r236160 - used to be that you
passed the Type and the address space as the first two arguments...
I'm not sure why I thought it was a good idea to merge these into a
PointerType for the sake of typeless pointer work... it looks like the
exact opposite of what I wanted. But I was young and naive... (5 months
ago)
More or less I'm reverting that change (might be better to just straight up
revert it, really)
But before or after my change I think the unit test is still wrong,
probably...
>
> 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.)
>
OK, I'll have a go at that.
>
> (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
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150911/44dfa90c/attachment.html>
More information about the llvm-commits
mailing list