More inline responses, updated patch still to come.<br><br><div class="gmail_quote">On Thu, Aug 23, 2012 at 12:58 PM, Duncan Sands <span dir="ltr"><<a href="mailto:baldrick@free.fr" target="_blank">baldrick@free.fr</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Chandler,<div class="im"><br>
<br>
On 22/08/12 07:46, Chandler Carruth wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Ok, I think this patch is pretty close to being ready to go into the tree,<br>
modulo any detailed code review folks are willing to do. Just send me comments<br>
if you have them.<br>
</blockquote>
<br></div>
more comments, picking up where I stopped off last night. Well, there is still<br>
a chunk I didn't go through yet, maybe tomorrow.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+namespace {<br>
+// InstVisitor to rewrite instructions using a particular region of an alloca<br>
+// to use a new alloca.<br>
+class AllocaPartitionRewriter : public InstVisitor<<u></u>AllocaPartitionRewriter,<br>
+ bool> {<br>
+ // Befriend the base class so it can delegate to private visit methods.<br>
+ friend class llvm::InstVisitor<<u></u>AllocaPartitionRewriter, bool>;<br>
+<br>
+ const TargetData &TD;<br>
+ AllocaPartitioning &P;<br>
+ SROA &Pass;<br>
+ AllocaInst &OldAI, &NewAI;<br>
+ const uint64_t NewAllocaBeginOffset, NewAllocaEndOffset;<br>
+<br>
+ // If we are rewriting an alloca partition which can be written as pure<br>
+ // vector operations, we stash extra information here.<br>
</blockquote>
<br>
It would be nice to have a summary of what is known in that case. The later<br>
code seems to assume that you never have to deal with only part of a vector<br>
element for example.<br></blockquote><div><br></div><div>Yea, this is confusing. I actually had a "cleaner" design before where we had totally separate rewriting for the vector promotable so it was obvious what assumptions were being made. However, that required duplicating a massive amount of code from the normal rewriter so I ended up merging them with these extra bits of state.</div>
<div><br></div><div>Let me know if the comments I'm adding aren't enough.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ VectorType *VecTy;<br>
+ Type *ElementTy;<br>
+ uint64_t ElementSize;<br>
+<br>
+ // The offset of the partition user currently being rewritten.<br>
+ uint64_t BeginOffset, EndOffset;<br>
+ Instruction *OldPtr;<br>
+<br>
+ // The name prefix to use when rewriting instructions for this alloca.<br>
+ std::string NamePrefix;<br>
+<br>
+public:<br>
+ AllocaPartitionRewriter(const TargetData &TD, AllocaPartitioning &P,<br>
+ AllocaPartitioning::iterator PI,<br>
+ SROA &Pass, AllocaInst &OldAI, AllocaInst &NewAI,<br>
+ uint64_t NewBeginOffset, uint64_t NewEndOffset)<br>
+ : TD(TD), P(P), Pass(Pass),<br>
+ OldAI(OldAI), NewAI(NewAI),<br>
+ NewAllocaBeginOffset(<u></u>NewBeginOffset),<br>
+ NewAllocaEndOffset(<u></u>NewEndOffset),<br>
+ VecTy(), ElementTy(), ElementSize(),<br>
+ BeginOffset(), EndOffset() {<br>
+ }<br>
+<br>
+ bool visitUsers(AllocaPartitioning:<u></u>:const_use_iterator I,<br>
+ AllocaPartitioning::const_use_<u></u>iterator E) {<br>
+ if (isVectorPromotionViable(TD, NewAI.getAllocatedType(), P,<br>
+ NewAllocaBeginOffset, NewAllocaEndOffset,<br>
+ I, E)) {<br>
+ ++NumVectorized;<br>
+ VecTy = cast<VectorType>(NewAI.<u></u>getAllocatedType());<br>
+ ElementTy = VecTy->getElementType();<br>
+ ElementSize = TD.getTypeAllocSize(ElementTy)<u></u>;<br>
+ }<br>
+ bool CanSROA = true;<br>
+ for (; I != E; ++I) {<br>
+ BeginOffset = I->BeginOffset;<br>
+ EndOffset = I->EndOffset;<br>
+ OldPtr = I->Ptr;<br>
+ NamePrefix = (Twine(NewAI.getName()) + "." + Twine(BeginOffset)).str();<br>
+ CanSROA &= visit(I->User);<br>
+ }<br>
+ if (VecTy) {<br>
+ assert(CanSROA);<br>
+ VecTy = 0;<br>
+ ElementTy = 0;<br>
+ ElementSize = 0;<br>
+ }<br>
+ return CanSROA;<br>
+ }<br>
+<br>
+private:<br>
+ // Every instruction which can end up as a user must have a rewrite rule.<br>
+ bool visitInstruction(Instruction &I) {<br>
+ DEBUG(dbgs() << " !!!! Cannot rewrite: " << I << "\n");<br>
+ llvm_unreachable("No rewrite rule for this instruction!");<br>
+ }<br>
+<br>
+ Twine getName(const Twine &Suffix) {<br>
+ return NamePrefix + Suffix;<br>
+ }<br>
+<br>
+ Value *getAdjustedAllocaPtr(<u></u>IRBuilder<> IRB, Type *PointerTy) {<br>
+ assert(BeginOffset >= NewAllocaBeginOffset);<br>
+ APInt Offset(TD.<u></u>getPointerSizeInBits(), BeginOffset - NewAllocaBeginOffset);<br>
+ return getAdjustedPtr(IRB, TD, &NewAI, Offset, PointerTy, getName(""));<br>
+ }<br>
+<br>
+ ConstantInt *getIndex(uint64_t Offset) {<br>
+ assert(VecTy && "Can only call getIndex when rewriting a vector");<br>
+ uint64_t RelOffset = Offset - NewAllocaBeginOffset;<br>
+ uint64_t Index = RelOffset / ElementSize;<br>
+ assert(Index * ElementSize == RelOffset);<br>
+ return ConstantInt::get(Type::<u></u>getInt32Ty(VecTy->getContext()<u></u>), Index);<br>
+ }<br>
+<br>
+ void deleteIfTriviallyDead(Value *V) {<br>
+ Instruction *I = cast<Instruction>(V);<br>
+ if (isInstructionTriviallyDead(I)<u></u>)<br>
+ Pass.DeadInsts.push_back(I);<br>
+ }<br>
+<br>
+ Value *getValueCast(IRBuilder<> IRB, Value *V, Type *Ty) {<br>
+ if (V->getType()->isIntegerTy() && Ty->isPointerTy())<br>
+ return IRB.CreateIntToPtr(V, Ty);<br>
+ if (V->getType()->isPointerTy() && Ty->isIntegerTy())<br>
+ return IRB.CreatePtrToInt(V, Ty);<br>
+<br>
+ return IRB.CreateBitCast(V, Ty);<br>
</blockquote>
<br>
Maybe you can use getCastOpcode here.<br></blockquote><div><br></div><div>I'm a bit torn here, but ultimately I think I prefer the narrower accepted set of casts. It keeps my code honest and shouldn't let questionable patterns sneak through as easily.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ bool rewriteVectorizedLoadInst(<u></u>IRBuilder<> IRB, LoadInst &LI, Value *OldOp) {<br>
+ Value *Result;<br>
+ if (LI.getType() == VecTy->getElementType() ||<br>
+ BeginOffset > NewAllocaBeginOffset || EndOffset < NewAllocaEndOffset) {<br>
+ Result<br>
+ = IRB.CreateExtractElement(IRB.<u></u>CreateLoad(&NewAI, getName(".load")),<br>
+ getIndex(BeginOffset),<br>
+ getName(".extract"));<br>
+ } else {<br>
+ Result = IRB.CreateLoad(&NewAI, getName(".load"));<br>
+ }<br>
+ if (Result->getType() != LI.getType())<br>
+ Result = getValueCast(IRB, Result, LI.getType());<br>
+ LI.replaceAllUsesWith(Result);<br>
+ Pass.DeadInsts.push_back(&LI);<br>
+<br>
+ DEBUG(dbgs() << " to: " << *Result << "\n");<br>
+ return true;<br>
+ }<br>
+<br>
+ bool visitLoadInst(LoadInst &LI) {<br>
+ DEBUG(dbgs() << " original: " << LI << "\n");<br>
+ Value *OldOp = LI.getOperand(0);<br>
+ assert(OldOp == OldPtr);<br>
+ IRBuilder<> IRB(&LI);<br>
+<br>
+ if (VecTy)<br>
+ return rewriteVectorizedLoadInst(IRB, LI, OldOp);<br>
+<br>
+ Value *NewPtr = getAdjustedAllocaPtr(IRB,<br>
+ LI.getPointerOperand()-><u></u>getType());<br>
+ LI.setOperand(0, NewPtr);<br>
+ DEBUG(dbgs() << " to: " << LI << "\n");<br>
+<br>
+ deleteIfTriviallyDead(OldOp);<br>
+ return NewPtr == &NewAI && !LI.isVolatile();<br>
+ }<br>
+<br>
+ bool rewriteVectorizedStoreInst(<u></u>IRBuilder<> IRB, StoreInst &SI,<br>
+ Value *OldOp) {<br>
+ Value *V = SI.getValueOperand();<br>
+ if (V->getType() == ElementTy ||<br>
+ BeginOffset > NewAllocaBeginOffset || EndOffset < NewAllocaEndOffset) {<br>
+ if (V->getType() != ElementTy)<br>
+ V = getValueCast(IRB, V, ElementTy);<br>
+ V = IRB.CreateInsertElement(IRB.<u></u>CreateLoad(&NewAI, getName(".load")), V,<br>
+ getIndex(BeginOffset),<br>
+ getName(".insert"));<br>
+ } else if (V->getType() != VecTy) {<br>
+ V = getValueCast(IRB, V, VecTy);<br>
+ }<br>
+ StoreInst *Store = IRB.CreateStore(V, &NewAI);<br>
+ Pass.DeadInsts.push_back(&SI);<br>
+<br>
+ (void)Store;<br>
+ DEBUG(dbgs() << " to: " << *Store << "\n");<br>
+ return true;<br>
+ }<br>
+<br>
+ bool visitStoreInst(StoreInst &SI) {<br>
+ DEBUG(dbgs() << " original: " << SI << "\n");<br>
+ Value *OldOp = SI.getOperand(1);<br>
+ assert(OldOp == OldPtr);<br>
+ IRBuilder<> IRB(&SI);<br>
+<br>
+ if (VecTy)<br>
+ return rewriteVectorizedStoreInst(<u></u>IRB, SI, OldOp);<br>
+<br>
+ Value *NewPtr = getAdjustedAllocaPtr(IRB,<br>
+ SI.getPointerOperand()-><u></u>getType());<br>
+ SI.setOperand(1, NewPtr);<br>
+ DEBUG(dbgs() << " to: " << SI << "\n");<br>
+<br>
+ deleteIfTriviallyDead(OldOp);<br>
+ return NewPtr == &NewAI && !SI.isVolatile();<br>
+ }<br>
+<br>
+ bool visitMemSetInst(MemSetInst &II) {<br>
+ DEBUG(dbgs() << " original: " << II << "\n");<br>
+ IRBuilder<> IRB(&II);<br>
+ assert(II.getRawDest() == OldPtr);<br>
+<br>
+ // Record this instruction for deletion.<br>
+ if (Pass.DeadSplitInsts.insert(&<u></u>II))<br>
+ Pass.DeadInsts.push_back(&II);<br>
+<br>
+ Type *AllocaTy = NewAI.getAllocatedType();<br>
+ Type *ScalarTy = AllocaTy->getScalarType();<br>
+<br>
+ // If this doesn't map cleanly onto the alloca type, and that type isn't<br>
+ // a single value type, just emit a memcpy.<br>
</blockquote>
<br>
memcpy -> memset<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (!VecTy && (BeginOffset != NewAllocaBeginOffset ||<br>
+ EndOffset != NewAllocaEndOffset ||<br>
+ !AllocaTy->isSingleValueType() ||<br>
+ !TD.isLegalInteger(TD.<u></u>getTypeSizeInBits(ScalarTy)))) {<br>
+ Type *SizeTy = II.getLength()->getType();<br>
+ Constant *Size = ConstantInt::get(SizeTy, EndOffset - BeginOffset);<br>
+<br>
+ CallInst *New<br>
+ = IRB.CreateMemSet(<u></u>getAdjustedAllocaPtr(IRB,<br>
+ II.getRawDest()->getType()),<br>
+ II.getValue(), Size, II.getAlignment(),<br>
+ II.isVolatile());<br>
+ (void)New;<br>
+ DEBUG(dbgs() << " to: " << *New << "\n");<br>
+ return false;<br>
+ }<br>
+<br>
+ // If we can represent this as a simple value, we have to build the actual<br>
+ // value to store, which requires expanding the byte present in memset to<br>
+ // a sensible representation for the alloca type. This is essentially<br>
+ // splatting the byte to a sufficiently wide integer, bitcasting to the<br>
+ // desired scalar type, and splatting it across any desired vector type.<br>
</blockquote>
<br>
How useful is it for you to do this kind of thing? Won't it happen later in<br>
instcombine anyway?<br></blockquote><div><br></div><div>I don't think so, and we really want to do this here because it exposes an alloca promotion opportunity. The whole point of the rewriter is to make things suitable for promotion.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ Value *V = II.getValue();<br>
+ IntegerType *VTy = cast<IntegerType>(V->getType()<u></u>);<br>
+ Type *IntTy = Type::getIntNTy(VTy-><u></u>getContext(),<br>
+ TD.getTypeSizeInBits(ScalarTy)<u></u>);<br>
+ if (TD.getTypeSizeInBits(<u></u>ScalarTy) > VTy->getBitWidth())<br>
+ V = IRB.CreateMul(IRB.CreateZExt(<u></u>V, IntTy, getName(".zext")),<br>
+ ConstantExpr::getUDiv(<br>
+ Constant::getAllOnesValue(<u></u>IntTy),<br>
+ ConstantExpr::getZExt(<br>
+ Constant::getAllOnesValue(V-><u></u>getType()),<br>
+ IntTy)),<br>
+ getName(".isplat"));<br>
+ if (V->getType() != ScalarTy) {<br>
+ if (ScalarTy->isPointerTy())<br>
+ V = IRB.CreateIntToPtr(V, ScalarTy);<br>
+ else if (ScalarTy->isPrimitiveType() || ScalarTy->isVectorTy())<br>
+ V = IRB.CreateBitCast(V, ScalarTy);<br>
+ else if (ScalarTy->isIntegerTy())<br>
+ llvm_unreachable("Computed different integer types with equal widths");<br>
+ else<br>
+ llvm_unreachable("Invalid scalar type");<br>
+ }<br>
+<br>
+ // If this is an element-wide memset of a vectorizable alloca, insert it.<br>
+ if (VecTy && (BeginOffset > NewAllocaBeginOffset ||<br>
+ EndOffset < NewAllocaEndOffset)) {<br>
+ StoreInst *Store = IRB.CreateStore(<br>
+ IRB.CreateInsertElement(IRB.<u></u>CreateLoad(&NewAI, getName(".load")), V,<br>
+ getIndex(BeginOffset),<br>
+ getName(".insert")),<br>
+ &NewAI);<br>
+ (void)Store;<br>
+ DEBUG(dbgs() << " to: " << *Store << "\n");<br>
+ return true;<br>
+ }<br>
+<br>
+ // Splat to a vector if needed.<br>
+ if (VectorType *VecTy = dyn_cast<VectorType>(AllocaTy)<u></u>) {<br>
+ VectorType *SplatSourceTy = VectorType::get(V->getType(), 1);<br>
+ V = IRB.CreateShuffleVector(<br>
+ IRB.CreateInsertElement(<u></u>UndefValue::get(SplatSourceTy)<u></u>, V,<br>
+ ConstantInt::get(IRB.<u></u>getInt32Ty(), 0),<br>
+ getName(".vsplat.insert")),<br>
+ UndefValue::get(SplatSourceTy)<u></u>,<br>
+ ConstantVector::getSplat(<u></u>VecTy->getNumElements(),<br>
+ ConstantInt::get(IRB.<u></u>getInt32Ty(), 0)),<br>
+ getName(".vsplat.shuffle"));<br>
+ assert(V->getType() == VecTy);<br>
+ }<br>
+<br>
+ Value *New = IRB.CreateStore(V, &NewAI, II.isVolatile());<br>
+ (void)New;<br>
+ DEBUG(dbgs() << " to: " << *New << "\n");<br>
+ return !II.isVolatile();<br>
+ }<br>
+<br>
+ bool visitMemTransferInst(<u></u>MemTransferInst &II) {<br>
+ // Rewriting of memory transfer instructions can be a bit tricky. We break<br>
+ // them into two categories: split intrinsics and unsplit intrinsics.<br>
+<br>
+ DEBUG(dbgs() << " original: " << II << "\n");<br>
+ IRBuilder<> IRB(&II);<br>
+<br>
+ const AllocaPartitioning::<u></u>MemTransferOffsets &MTO<br>
+ = P.getMemTransferOffsets(II);<br>
+<br>
+ // For unsplit intrinsics, we simply modify the source and destination<br>
+ // pointers in place. This isn't just an optimization, it is a matter of<br>
+ // correctness. With unsplit intrinsics we may be dealing with transfers<br>
+ // within a single alloca before SROA ran. We may also be dealing with<br>
+ // memmove instead of memcpy, and so simply updating the pointers is the<br>
+ // necessary for us to update both source and dest of a single call.<br>
+ if (!MTO.IsSplittable) {<br>
+ Value *OldOp = BeginOffset == MTO.DestBegin ? II.getRawDest()<br>
+ : II.getRawSource();<br>
+ if (BeginOffset == MTO.DestBegin)<br>
+ II.setDest(<u></u>getAdjustedAllocaPtr(IRB, II.getRawDest()->getType()));<br>
+ else<br>
+ II.setSource(<u></u>getAdjustedAllocaPtr(IRB, II.getRawSource()->getType()))<u></u>;<br>
+<br>
+ DEBUG(dbgs() << " to: " << II << "\n");<br>
+ deleteIfTriviallyDead(OldOp);<br>
+ return false;<br>
+ }<br>
+ // For split transfer intrinsics we have an incredibly useful assurance:<br>
+ // the source and destination do not reside within the same alloca, and at<br>
+ // least one of them does not escape. This means that we can replace<br>
+ // memmove with memcpy, and we don't need to worry about all manner of<br>
+ // downsides to splitting and transforming the operations.<br>
+<br>
+ assert(II.getRawSource() == OldPtr || II.getRawDest() == OldPtr);<br>
+ bool IsDest = II.getRawDest() == OldPtr;<br>
+ // Compute the relative offset within the transfer.<br>
+ unsigned IntPtrWidth = TD.getPointerSizeInBits();<br>
+ APInt RelOffset(IntPtrWidth, BeginOffset - (IsDest ? MTO.DestBegin<br>
+ : MTO.SourceBegin));<br>
+<br>
+ // If this doesn't map cleanly onto the alloca type, and that type isn't<br>
+ // a single value type, just emit a memcpy.<br>
</blockquote>
<br>
Do you care if it is not a *legal* single value type?<br></blockquote><div><br></div><div>Not really. The only types we end up with in the alloca type that are single value types are types that were already in use in the source program. Because we only ever re-use types, we should only form things that the source code and/or frontend already thought were plausible.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ bool EmitMemCpy<br>
+ = !VecTy && (BeginOffset != NewAllocaBeginOffset ||<br>
+ EndOffset != NewAllocaEndOffset ||<br>
+ !NewAI.getAllocatedType()-><u></u>isSingleValueType());<br>
+<br>
+ // If we're just going to emit a memcpy and the alloca hasn't changed, this<br>
+ // is a no-op.<br>
+ if (EmitMemCpy && &OldAI == &NewAI) {<br>
+ // Assert that we are in fact not changing the copy size.<br>
+ if (IsDest) {<br>
+ assert(BeginOffset == MTO.DestBegin);<br>
+ assert(EndOffset == MTO.DestEnd);<br>
+ } else {<br>
+ assert(BeginOffset == MTO.SourceBegin);<br>
+ assert(EndOffset == MTO.SourceEnd);<br>
+ }<br>
+ return false;<br>
+ }<br>
+ // Record this instruction for deletion.<br>
+ if (Pass.DeadSplitInsts.insert(&<u></u>II))<br>
+ Pass.DeadInsts.push_back(&II);<br>
+<br>
+ bool IsVectorElement = VecTy && (BeginOffset > NewAllocaBeginOffset ||<br>
+ EndOffset < NewAllocaEndOffset);<br>
</blockquote>
<br>
In the case of VecTy what do you know here? I'm kind of lost disentangling<br>
all the different cases that need to be handled, and which bits of code are<br>
dealing with what exactly.<br></blockquote><div><br></div><div>Yea, see my comment above. Do my new comments in the code help? If not, any concrete ideas about how to improve this? I've tried a few things, and worry that I'm stuck in my personal local optima, missing some simplification I've not yet thought of...</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ Type *OtherPtrTy = IsDest ? II.getRawSource()->getType()<br>
+ : II.getRawDest()->getType();<br>
+ if (!EmitMemCpy)<br>
+ OtherPtrTy = IsVectorElement ? VecTy->getElementType()-><u></u>getPointerTo()<br>
+ : NewAI.getType();<br>
+<br>
+ // Compute the other pointer, folding as much as possible to produce<br>
+ // a single, simple GEP in most cases.<br>
+ Value *OtherPtr = IsDest ? II.getRawSource() : II.getRawDest();<br>
+ OtherPtr = getAdjustedPtr(IRB, TD, OtherPtr, RelOffset, OtherPtrTy,<br>
+ getName("." + OtherPtr->getName()));<br>
+<br>
+ // Strip all inbounds GEPs and pointer casts to try to dig out any root<br>
+ // alloca that should be re-examined after rewriting this instruction.<br>
+ if (AllocaInst *AI<br>
+ = dyn_cast<AllocaInst>(OtherPtr-<u></u>>stripInBoundsOffsets()))<br>
+ Pass.Worklist.insert(AI);<br>
+<br>
+ if (EmitMemCpy) {<br>
+ Value *OurPtr<br>
+ = getAdjustedAllocaPtr(IRB, IsDest ? II.getRawDest()->getType()<br>
+ : II.getRawSource()->getType());<br>
+ Type *SizeTy = II.getLength()->getType();<br>
+ Constant *Size = ConstantInt::get(SizeTy, EndOffset - BeginOffset);<br>
+<br>
+ CallInst *New = IRB.CreateMemCpy(IsDest ? OurPtr : OtherPtr,<br>
+ IsDest ? OtherPtr : OurPtr,<br>
+ Size, II.getAlignment(),<br>
+ II.isVolatile());<br>
+ (void)New;<br>
+ DEBUG(dbgs() << " to: " << *New << "\n");<br>
+ return false;<br>
+ }<br>
+<br>
+ Value *SrcPtr = OtherPtr;<br>
+ Value *DstPtr = &NewAI;<br>
+ if (!IsDest)<br>
+ std::swap(SrcPtr, DstPtr);<br>
+<br>
+ Value *Src;<br>
+ if (IsVectorElement && !IsDest) {<br>
+ // We have to extract rather than load.<br>
+ Src = IRB.CreateExtractElement(IRB.<u></u>CreateLoad(SrcPtr,<br>
+ getName(".copyload")),<br>
+ getIndex(BeginOffset),<br>
+ getName(".copyextract"));<br>
+ } else {<br>
+ Src = IRB.CreateLoad(SrcPtr, II.isVolatile(), getName(".copyload"));<br>
+ }<br>
+<br>
+ if (IsVectorElement && IsDest) {<br>
+ // We have to insert into a loaded copy before storing.<br>
+ Src = IRB.CreateInsertElement(IRB.<u></u>CreateLoad(&NewAI, getName(".load")),<br>
+ Src, getIndex(BeginOffset),<br>
+ getName(".insert"));<br>
+ }<br>
+<br>
+ Value *Store = IRB.CreateStore(Src, DstPtr, II.isVolatile());<br>
+ (void)Store;<br>
+ DEBUG(dbgs() << " to: " << *Store << "\n");<br>
+ return !II.isVolatile();<br>
+ }<br>
+<br>
+ bool visitIntrinsicInst(<u></u>IntrinsicInst &II) {<br>
+ assert(II.getIntrinsicID() == Intrinsic::lifetime_start ||<br>
+ II.getIntrinsicID() == Intrinsic::lifetime_end);<br>
+ DEBUG(dbgs() << " original: " << II << "\n");<br>
+ IRBuilder<> IRB(&II);<br>
+ assert(II.getArgOperand(1) == OldPtr);<br>
+<br>
+ // Record this instruction for deletion.<br>
+ if (Pass.DeadSplitInsts.insert(&<u></u>II))<br>
+ Pass.DeadInsts.push_back(&II);<br>
+<br>
+ ConstantInt *Size<br>
+ = ConstantInt::get(cast<<u></u>IntegerType>(II.getArgOperand(<u></u>0)->getType()),<br>
+ EndOffset - BeginOffset);<br>
+ Value *Ptr = getAdjustedAllocaPtr(IRB, II.getArgOperand(1)->getType()<u></u>);<br>
+ Value *New;<br>
+ if (II.getIntrinsicID() == Intrinsic::lifetime_start)<br>
+ New = IRB.CreateLifetimeStart(Ptr, Size);<br>
+ else<br>
+ New = IRB.CreateLifetimeEnd(Ptr, Size);<br>
+<br>
+ DEBUG(dbgs() << " to: " << *New << "\n");<br>
+ return true;<br>
+ }<br>
+<br>
+ /// PHI instructions that use an alloca and are subsequently loaded can be<br>
+ /// rewritten to load both input pointers in the pred blocks and then PHI the<br>
+ /// results, allowing the load of the alloca to be promoted.<br>
+ /// From this:<br>
+ /// %P2 = phi [i32* %Alloca, i32* %Other]<br>
+ /// %V = load i32* %P2<br>
+ /// to:<br>
+ /// %V1 = load i32* %Alloca -> will be mem2reg'd<br>
+ /// ...<br>
+ /// %V2 = load i32* %Other<br>
+ /// ...<br>
+ /// %V = phi [i32 %V1, i32 %V2]<br>
+ ///<br>
+ /// We can do this to a select if its only uses are loads and if the operand<br>
+ /// to the select can be loaded unconditionally.<br>
+ ///<br>
+ /// FIXME: This should be hoisted into a generic utility, likely in<br>
+ /// Transforms/Util/Local.h<br>
+ bool isSafePHIToSpeculate(PHINode &PN, SmallVectorImpl<LoadInst *> &Loads) {<br>
+ // For now, we can only do this promotion if the load is in the same block<br>
+ // as the PHI, and if there are no stores between the phi and load.<br>
+ // TODO: Allow recursive phi users.<br>
+ // TODO: Allow stores.<br>
+ BasicBlock *BB = PN.getParent();<br>
+ unsigned MaxAlign = 0;<br>
+ for (Value::use_iterator UI = PN.use_begin(), UE = PN.use_end();<br>
+ UI != UE; ++UI) {<br>
+ LoadInst *LI = dyn_cast<LoadInst>(*UI);<br>
+ if (LI == 0 || !LI->isSimple()) return false;<br>
+<br>
+ // For now we only allow loads in the same block as the PHI. This is<br>
+ // a common case that happens when instcombine merges two loads through<br>
+ // a PHI.<br>
+ if (LI->getParent() != BB) return false;<br>
+<br>
+ // Ensure that there are no instructions between the PHI and the load that<br>
+ // could store.<br>
+ for (BasicBlock::iterator BBI = &PN; &*BBI != LI; ++BBI)<br>
+ if (BBI->mayWriteToMemory())<br>
+ return false;<br>
+<br>
+ MaxAlign = std::max(MaxAlign, LI->getAlignment());<br>
+ Loads.push_back(LI);<br>
+ }<br>
+<br>
+ // We can only transform this if it is safe to push the loads into the<br>
+ // predecessor blocks. The only thing to watch out for is that we can't put<br>
+ // a possibly trapping load in the predecessor if it is a critical edge.<br>
+ for (unsigned Idx = 0, Num = PN.getNumIncomingValues(); Idx != Num;<br>
+ ++Idx) {<br>
+ TerminatorInst *TI = PN.getIncomingBlock(Idx)-><u></u>getTerminator();<br>
+ Value *InVal = PN.getIncomingValue(Idx);<br>
+<br>
+ // If the value is produced by the terminator of the predecessor (an<br>
+ // invoke) or it has siide-effects, there is no valid place to put a load<br>
</blockquote>
<br>
siide-effects -> side-effects<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // in the predecessor.<br>
+ if (TI == InVal || TI->mayHaveSideEffects())<br>
+ return false;<br>
+<br>
+ // If the predecessor has a single successor, then the edge isn't<br>
+ // critical.<br>
+ if (TI->getNumSuccessors() == 1)<br>
+ continue;<br>
+<br>
+ // If this pointer is always safe to load, or if we can prove that there<br>
+ // is already a load in the block, then we can move the load to the pred<br>
+ // block.<br>
+ if (InVal-><u></u>isDereferenceablePointer() ||<br>
+ isSafeToLoadUnconditionally(<u></u>InVal, TI, MaxAlign, &TD))<br>
+ continue;<br>
+<br>
+ return false;<br>
+ }<br>
+<br>
+ return true;<br>
+ }<br>
+<br>
+ bool visitPHINode(PHINode &PN) {<br>
+ DEBUG(dbgs() << " original: " << PN << "\n");<div class="im"><br>
+<br>
+ // Find the operand we need to rewrite here.<br>
+ User::op_iterator OI = PN.op_begin(), OE = PN.op_end();<br>
+ for (; OI != OE && *OI != OldPtr; ++OI)<br>
+ ;<br>
+ assert(OI != OE);<br></div>
+<br>
+ IRBuilder<> Builder(&PN);<br>
+<br>
+ SmallVector<LoadInst *, 4> Loads;<br>
+ if (!isSafePHIToSpeculate(PN, Loads)) {<br>
+ Value *NewPtr = getAdjustedAllocaPtr(Builder, (*OI)->getType());<br>
+ *OI = NewPtr;<br>
+ DEBUG(dbgs() << " to: " << PN << "\n");<br>
+ deleteIfTriviallyDead(OldPtr);<br>
+ return false;<br>
+ }<br>
+ assert(!Loads.empty());<br>
+<br>
+ Type *LoadTy = cast<PointerType>(PN.getType()<u></u>)->getElementType();<br>
+ PHINode *NewPN = Builder.CreatePHI(LoadTy, PN.getNumIncomingValues());<br>
+ NewPN->takeName(&PN);<br>
+<br>
+ // Get the TBAA tag and alignment to use from one of the loads. It doesn't<br>
+ // matter which one we get and if any differ, it doesn't matter.<br>
</blockquote>
<br>
Why doesn't it matter? In essence your are saying that the tbaa tag is a<br>
property of the value being loaded, not a property of the load. But is this<br>
warranted? It may be true for how clang produces tbaa tags, but that's not<br>
what matters... In fact even then is it safe?<br></blockquote><div><br></div><div>I have no idea. This is not new code, this was lifted directly from the existing SROA. I think Dan is looking at changing all of this too.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ LoadInst *SomeLoad = cast<LoadInst>(Loads.back());<br>
+ MDNode *TBAATag = SomeLoad->getMetadata(<u></u>LLVMContext::MD_tbaa);<br>
+ unsigned Align = SomeLoad->getAlignment();<br>
+ Value *NewPtr = getAdjustedAllocaPtr(Builder, OldPtr->getType());<br>
+<br>
+ // Rewrite all loads of the PN to use the new PHI.<br>
+ do {<br>
+ LoadInst *LI = Loads.pop_back_val();<br>
+ LI->replaceAllUsesWith(NewPN);<br>
+ Pass.DeadInsts.push_back(LI);<br>
+ } while (!Loads.empty());<br>
+<br>
+ // Inject loads into all of the pred blocks.<br>
+ for (unsigned Idx = 0, Num = PN.getNumIncomingValues(); Idx != Num; ++Idx) {<br>
+ BasicBlock *Pred = PN.getIncomingBlock(Idx);<br>
+ TerminatorInst *TI = Pred->getTerminator();<br>
+ Value *InVal = PN.getIncomingValue(Idx);<br>
+ IRBuilder<> PredBuilder(TI);<br>
+<br>
+ // Map the value to the new alloca pointer if this was the old alloca<br>
+ // pointer.<br>
+ bool ThisOperand = InVal == OldPtr;<br>
+ if (ThisOperand)<br>
+ InVal = NewPtr;<br>
+<br>
+ LoadInst *Load<br>
+ = PredBuilder.CreateLoad(InVal, getName(".sroa.speculate." +<br>
+ Pred->getName()));<br>
+ ++NumLoadsSpeculated;<br>
+ Load->setAlignment(Align);<br>
+ if (TBAATag)<br>
+ Load->setMetadata(LLVMContext:<u></u>:MD_tbaa, TBAATag);<br>
+ NewPN->addIncoming(Load, Pred);<br>
+<br>
+ if (ThisOperand)<br>
+ continue;<br>
+ Instruction *OtherPtr = dyn_cast<Instruction>(InVal);<br>
+ if (!OtherPtr)<br>
+ // No uses to rewrite.<br>
+ continue;<br>
+<br>
+ // Try to lookup and rewrite any partition uses corresponding to this phi<br>
+ // input.<br>
+ AllocaPartitioning::iterator PI<br>
+ = P.<u></u>findPartitionForPHIOrSelectOpe<u></u>rand(PN, OtherPtr);<br>
+ if (PI != P.end()) {<br>
+ // If the other pointer is within the partitioning, replace the PHI in<br>
+ // its uses with the load we just speculated, or add another load for<br>
+ // it to rewrite if we've already replaced the PHI.<br>
+ AllocaPartitioning::use_<u></u>iterator UI<br>
+ = P.<u></u>findPartitionUseForPHIOrSelect<u></u>Operand(PN, OtherPtr);<br>
+ if (isa<PHINode>(*UI->User))<br>
+ UI->User = Load;<br>
+ else {<br>
+ AllocaPartitioning::<u></u>PartitionUse OtherUse = *UI;<br>
+ OtherUse.User = Load;<br>
+ P.use_insert(PI, std::upper_bound(UI, P.use_end(PI), OtherUse),<br>
+ OtherUse);<br>
+ }<br>
+ }<br>
+ }<br>
+ DEBUG(dbgs() << " speculated to: " << *NewPN << "\n");<br>
+ return true;<br>
+ }<br>
+<br>
+ /// Select instructions that use an alloca and are subsequently loaded can be<br>
+ /// rewritten to load both input pointers and then select between the result,<br>
+ /// allowing the load of the alloca to be promoted.<br>
+ /// From this:<br>
+ /// %P2 = select i1 %cond, i32* %Alloca, i32* %Other<br>
+ /// %V = load i32* %P2<br>
+ /// to:<br>
+ /// %V1 = load i32* %Alloca -> will be mem2reg'd<br>
+ /// %V2 = load i32* %Other<br>
+ /// %V = select i1 %cond, i32 %V1, i32 %V2<br>
+ ///<br>
+ /// We can do this to a select if its only uses are loads and if the operand<br>
+ /// to the select can be loaded unconditionally.<br>
+ bool isSafeSelectToSpeculate(<u></u>SelectInst &SI,<br>
+ SmallVectorImpl<LoadInst *> &Loads) {<br>
+ Value *TValue = SI.getTrueValue();<br>
+ Value *FValue = SI.getFalseValue();<br>
+ bool TDerefable = TValue-><u></u>isDereferenceablePointer();<br>
+ bool FDerefable = FValue-><u></u>isDereferenceablePointer();<br>
+<br>
+ for (Value::use_iterator UI = SI.use_begin(), UE = SI.use_end();<br>
+ UI != UE; ++UI) {<br>
+ LoadInst *LI = dyn_cast<LoadInst>(*UI);<br>
+ if (LI == 0 || !LI->isSimple()) return false;<br>
+<br>
+ // Both operands to the select need to be dereferencable, either<br>
+ // absolutely (e.g. allocas) or at this point because we can see other<br>
+ // accesses to it.<br>
+ if (!TDerefable && !isSafeToLoadUnconditionally(<u></u>TValue, LI,<br>
+ LI->getAlignment(), &TD))<br>
+ return false;<br>
+ if (!FDerefable && !isSafeToLoadUnconditionally(<u></u>FValue, LI,<br>
+ LI->getAlignment(), &TD))<br>
+ return false;<br>
+ Loads.push_back(LI);<br>
+ }<br>
+<br>
+ return true;<br>
+ }<br>
+<br>
+ bool visitSelectInst(SelectInst &SI) {<br>
+ DEBUG(dbgs() << " original: " << SI << "\n");<div class="im"><br>
+<br>
+ // Find the operand we need to rewrite here.<br></div>
+ bool IsTrueVal = SI.getTrueValue() == OldPtr;<br>
</blockquote>
<br>
How about an assert that if it isn't the true value then it must be the false<br>
value? And how about an assert that it isn't both values?<br></blockquote><div><br></div><div>Yea, this is particularly nice as if we just miss an optimization several steps earlier, it stops holding.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ Value *NewPtr = getAdjustedAllocaPtr(<u></u>IRBuilder<>(&SI), OldPtr->getType());<br>
+<br>
+ // If the select isn't safe to speculate, just use simple logic to emit it.<br>
+ SmallVector<LoadInst *, 4> Loads;<br>
+ if (!isSafeSelectToSpeculate(SI, Loads)) {<br>
+ SI.setOperand(IsTrueVal ? 1 : 2, NewPtr);<br>
+ DEBUG(dbgs() << " to: " << SI << "\n");<br>
+ deleteIfTriviallyDead(OldPtr);<br>
+ return false;<br>
+ }<br>
+<br>
+ Value *OtherPtr = IsTrueVal ? SI.getFalseValue() : SI.getTrueValue();<br>
+ AllocaPartitioning::iterator PI<br>
+ = P.<u></u>findPartitionForPHIOrSelectOpe<u></u>rand(SI, OtherPtr);<br>
+ AllocaPartitioning::<u></u>PartitionUse OtherUse;<br>
+ if (PI != P.end()) {<br>
+ // If the other pointer is within the partitioning, remove the select<br>
+ // from its uses. We'll add in the new loads below.<br>
+ AllocaPartitioning::use_<u></u>iterator UI<br>
+ = P.<u></u>findPartitionUseForPHIOrSelect<u></u>Operand(SI, OtherPtr);<br>
+ OtherUse = *UI;<br>
+ P.use_erase(PI, UI);<br>
</blockquote>
<br>
Having to keep track of uses of other partitions like looks like a pain and<br>
seems fragile. Do you really need to precompute all uses for all partitions?<br>
If you could just visit one partition at a time, compute its uses and rewrite<br>
them then you wouldn't have to worry about the other partitions like this. Is<br>
something like that feasible?<br></blockquote><div><br></div><div>We actually talked about this some in another context, but to recap, not really.</div><div><br></div><div>The problem is that the only way to compute the uses of a partition is to walk the entire recursive user set for the original alloca. Because this is a recursive user chain walk (mmm linked lists) it is *really* slow. Doing it once per partition would make the increased complexity of this algorithm much more problematic. One of the things I've worked very hard on is to minimize the impact of the aggressive splitting by having everything in the complexity domain of the number of splits have quite high locality and low constant factors. Rotating the use mapping to be per-partition in a dense data structure is a big part of this.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ Value *TV = IsTrueVal ? NewPtr : SI.getTrueValue();<br>
+ Value *FV = IsTrueVal ? SI.getFalseValue() : NewPtr;<br>
+ // Replace the loads of the select with a select of two loads.<br>
+ while (!Loads.empty()) {<br>
+ LoadInst *LI = Loads.pop_back_val();<br>
+<br>
+ IRBuilder<> Builder(LI);<br>
+ LoadInst *TL =<br>
+ Builder.CreateLoad(TV, getName("." + LI->getName() + ".true"));<br>
+ LoadInst *FL =<br>
+ Builder.CreateLoad(FV, getName("." + LI->getName() + ".false"));<br>
+ NumLoadsSpeculated += 2;<br>
+ if (PI != P.end()) {<br>
+ LoadInst *OtherLoad = IsTrueVal ? FL : TL;<br>
+ assert(OtherUse.Ptr == OtherLoad->getOperand(0));<br>
+ OtherUse.User = OtherLoad;<br>
+ P.use_insert(PI, P.use_end(PI), OtherUse);<br>
+ }<br>
+<br>
+ // Transfer alignment and TBAA info if present.<br>
+ TL->setAlignment(LI-><u></u>getAlignment());<br>
+ FL->setAlignment(LI-><u></u>getAlignment());<br>
+ if (MDNode *Tag = LI->getMetadata(LLVMContext::<u></u>MD_tbaa)) {<br>
+ TL->setMetadata(LLVMContext::<u></u>MD_tbaa, Tag);<br>
+ FL->setMetadata(LLVMContext::<u></u>MD_tbaa, Tag);<br>
+ }<br>
+<br>
+ Value *V = Builder.CreateSelect(SI.<u></u>getCondition(), TL, FL);<br>
+ V->takeName(LI);<br>
+ DEBUG(dbgs() << " speculated to: " << *V << "\n");<br>
+ LI->replaceAllUsesWith(V);<br>
+ Pass.DeadInsts.push_back(LI);<br>
+ }<br>
+ if (PI != P.end())<br>
+ std::stable_sort(P.use_begin(<u></u>PI), P.use_end(PI));<br>
+<br>
+ deleteIfTriviallyDead(OldPtr);<br>
+ return NewPtr == &NewAI;<br>
+ }<br>
+<br>
+};<br>
+}<br>
</blockquote>
<br>
Ciao, Duncan.<br>
</blockquote></div><br>