<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 11, 2015 at 3:40 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015-Sep-11, at 14:44, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Aug 19, 2014 at 2:18 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> Author: dexonsmith<br>
> Date: Tue Aug 19 16:18:21 2014<br>
> New Revision: 216023<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=216023&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=216023&view=rev</a><br>
> Log:<br>
> IR: Fix a missed case when threading OnlyIfReduced through ConstantExpr<br>
><br>
> In r216015 I missed propagating `OnlyIfReduced` through the inline<br>
> versions of `getGetElementPtr()` (I was relying on compile failures on<br>
> mismatches between the header and source signatures to get them all).<br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/IR/Constants.h<br>
> llvm/trunk/unittests/IR/ConstantsTest.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/IR/Constants.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Constants.h?rev=216023&r1=216022&r2=216023&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Constants.h?rev=216023&r1=216022&r2=216023&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/IR/Constants.h (original)<br>
> +++ llvm/trunk/include/llvm/IR/Constants.h Tue Aug 19 16:18:21 2014<br>
> @@ -1036,9 +1036,9 @@ public:<br>
> static Constant *getGetElementPtr(Constant *C, ArrayRef<Constant *> IdxList,<br>
> bool InBounds = false,<br>
> Type *OnlyIfReducedTy = nullptr) {<br>
> - return getGetElementPtr(C, makeArrayRef((Value * const *)IdxList.data(),<br>
> - IdxList.size()),<br>
> - InBounds);<br>
> + return getGetElementPtr(<br>
> + C, makeArrayRef((Value * const *)IdxList.data(), IdxList.size()),<br>
> + InBounds, OnlyIfReducedTy);<br>
> }<br>
> static Constant *getGetElementPtr(Constant *C, Constant *Idx,<br>
> bool InBounds = false,<br>
> @@ -1046,7 +1046,7 @@ public:<br>
> // This form of the function only exists to avoid ambiguous overload<br>
> // warnings about whether to convert Idx to ArrayRef<Constant *> or<br>
> // ArrayRef<Value *>.<br>
> - return getGetElementPtr(C, cast<Value>(Idx), InBounds);<br>
> + return getGetElementPtr(C, cast<Value>(Idx), InBounds, OnlyIfReducedTy);<br>
> }<br>
> static Constant *getGetElementPtr(Constant *C, ArrayRef<Value *> IdxList,<br>
> bool InBounds = false,<br>
><br>
> Modified: llvm/trunk/unittests/IR/ConstantsTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/ConstantsTest.cpp?rev=216023&r1=216022&r2=216023&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/ConstantsTest.cpp?rev=216023&r1=216022&r2=216023&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/unittests/IR/ConstantsTest.cpp (original)<br>
> +++ llvm/trunk/unittests/IR/ConstantsTest.cpp Tue Aug 19 16:18:21 2014<br>
> @@ -322,5 +322,30 @@ TEST(ConstantsTest, ConstantExprReplaceW<br>
> ASSERT_EQ(Int2, Ref->getInitializer());<br>
> }<br>
><br>
> +TEST(ConstantsTest, GEPReplaceWithConstant) {<br>
> + LLVMContext Context;<br>
> + std::unique_ptr<Module> M(new Module("MyModule", Context));<br>
> +<br>
> + Type *IntTy = Type::getInt32Ty(Context);<br>
> + Type *PtrTy = PointerType::get(IntTy, 0);<br>
> + auto *C1 = ConstantInt::get(IntTy, 1);<br>
> + auto *Placeholder = new GlobalVariable(<br>
> + *M, IntTy, false, GlobalValue::ExternalWeakLinkage, nullptr);<br>
> + auto *GEP = ConstantExpr::getGetElementPtr(Placeholder, C1);<br>
> + ASSERT_EQ(GEP->getOperand(0), Placeholder);<br>
> +<br>
> + auto *Ref =<br>
> + new GlobalVariable(*M, PtrTy, false, GlobalValue::ExternalLinkage, GEP);<br>
> + ASSERT_EQ(GEP, Ref->getInitializer());<br>
> +<br>
> + auto *Global = new GlobalVariable(*M, PtrTy, false,<br>
> + GlobalValue::ExternalLinkage, nullptr);<br>
> + auto *Alias = GlobalAlias::create(IntTy, 0, GlobalValue::ExternalLinkage,<br>
> + "alias", Global, M.get());<br>
><br>
> *casts Lesser Thread Necromancy*<br>
><br>
> 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<br>
><br>
> If I correct the GlobalAlias's type, then it doesn't match the Placeholder which it's RAUW'd with.<br>
><br>
> Any idea which parts of this need to remain in tact for the test to be valuable?<br>
<br>
</div></div>I don't understand how that even compiles. Looking at my tree,<br>
`GlobalAlias::create()` takes `PointerType*` as it's first argument.<br>
I'm confused :/.<br></blockquote><div><br></div><div>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... <br><br>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)<br><br>More or less I'm reverting that change (might be better to just straight up revert it, really)<br><br>But before or after my change I think the unit test is still wrong, probably... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Anyway, the point was to provide explicit coverage for RAUW hitting<br>
this code in `ConstantExpr::getGetElementPtr()`:<br>
--<br>
Type *DestTy = GetElementPtrInst::getIndexedType(Ty, Idxs);<br>
assert(DestTy && "GEP indices invalid!");<br>
unsigned AS = C->getType()->getPointerAddressSpace();<br>
Type *ReqTy = DestTy->getPointerTo(AS);<br>
if (VectorType *VecTy = dyn_cast<VectorType>(C->getType()))<br>
ReqTy = VectorType::get(ReqTy, VecTy->getNumElements());<br>
<br>
if (OnlyIfReducedTy == ReqTy) // <-- this is the key code<br>
return nullptr; // <-- (and this)<br>
--<br>
and thereby not recreating (and RAUW-ing) the GEP.<br>
<br>
What's happening is that we've RAUW'ed a constant, and all its users<br>
that are also constants need to be re-canonicalized, and possibly<br>
RAUW'ed. `Constant::handleOperandChange()` gets called on each of<br>
these constant users. For GEP, this forks out to<br>
`ConstantExpr::handleOperandChangeImpl()`, which calls<br>
`ConstantExpr::getWithOperands(..., /*OnlyIfReduced*/ true)`. This<br>
function should return a new `ConstantExpr` if and *only* if it's<br>
a different result from updating the operand in place.<br>
<br>
(Why? It's a micro-optimization, since this reduces malloc traffic,<br>
and prevents long, unnecessary RAUW chains. My driving goal at the<br>
time was to make use-list order easily predictable (RAUWs shuffle<br>
use-lists).)<br>
<br>
As long as the test covers the `return nullptr` logic -- we RAUW'ed<br>
a GEP operand and should mutate the GEP in place -- then it's still<br>
covering the right thing. You can check this by deleting the early<br>
return and checking that the unit test fails. (The only other<br>
failing tests should be `verify-uselistorder` tests.)<br></blockquote><div><br></div><div>OK, I'll have a go at that.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
(I keep thinking that the key check above should be simpler:<br>
--<br>
if (OnlyIfReducedTy) {<br>
assert(OnlyIfReducedTy == ReqTy && "RAUW cannot mutate types!");<br>
return nullptr;<br>
}<br>
--<br>
But I seem to remember trying that, and being wrong. Maybe not?)<br>
<span class=""><br>
> Should we change the Placeholder type to match the GlobalAlias and Globals type?<br>
> Should we add a bitcast of the Global when constructing the GlobalAlias?<br>
> Something else?<br>
<br>
</span>The RAUW'ed object needs to be a direct operand of the GEP. I<br>
think the types don't matter here (aside from making the IR valid).<br>
<span class=""><br>
> (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)<br>
<br>
</span>I wonder about the end state with the explicit pointee type work.<br>
Maybe the `Type` can also be mutate<br>
<div class="HOEnZb"><div class="h5"><br>
> + Placeholder->replaceAllUsesWith(Alias);<br>
> + ASSERT_EQ(GEP, Ref->getInitializer());<br>
> + ASSERT_EQ(GEP->getOperand(0), Alias);<br>
> +}<br>
> +<br>
> } // end anonymous namespace<br>
> } // end namespace llvm<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>