Please use GitHub pull requests for new patches. Phabricator shutdown timeline
Changeset View
Changeset View
Standalone View
Standalone View
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
| //===- InstCombineLoadStoreAlloca.cpp -------------------------------------===// | //===- InstCombineLoadStoreAlloca.cpp -------------------------------------===// | |||||||||||||||||
| // | // | |||||||||||||||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | |||||||||||||||||
| // See https://llvm.org/LICENSE.txt for license information. | // See https://llvm.org/LICENSE.txt for license information. | |||||||||||||||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||||||||||||||||
| // | // | |||||||||||||||||
| //===----------------------------------------------------------------------===// | //===----------------------------------------------------------------------===// | |||||||||||||||||
| // | // | |||||||||||||||||
| // This file implements the visit functions for load, store and alloca. | // This file implements the visit functions for load, store and alloca. | |||||||||||||||||
| // | // | |||||||||||||||||
| //===----------------------------------------------------------------------===// | //===----------------------------------------------------------------------===// | |||||||||||||||||
| #include "InstCombineInternal.h" | #include "InstCombineInternal.h" | |||||||||||||||||
| #include "llvm/ADT/MapVector.h" | #include "llvm/ADT/MapVector.h" | |||||||||||||||||
| #include "llvm/ADT/SmallString.h" | #include "llvm/ADT/SmallString.h" | |||||||||||||||||
nikic: Shouldn't be needed (already used in file). | ||||||||||||||||||
| #include "llvm/ADT/Statistic.h" | #include "llvm/ADT/Statistic.h" | |||||||||||||||||
| #include "llvm/Analysis/AliasAnalysis.h" | #include "llvm/Analysis/AliasAnalysis.h" | |||||||||||||||||
| #include "llvm/Analysis/Loads.h" | #include "llvm/Analysis/Loads.h" | |||||||||||||||||
| #include "llvm/IR/DataLayout.h" | #include "llvm/IR/DataLayout.h" | |||||||||||||||||
| #include "llvm/IR/DebugInfoMetadata.h" | #include "llvm/IR/DebugInfoMetadata.h" | |||||||||||||||||
| #include "llvm/IR/IntrinsicInst.h" | #include "llvm/IR/IntrinsicInst.h" | |||||||||||||||||
| #include "llvm/IR/LLVMContext.h" | #include "llvm/IR/LLVMContext.h" | |||||||||||||||||
| #include "llvm/IR/PatternMatch.h" | #include "llvm/IR/PatternMatch.h" | |||||||||||||||||
| Show All 18 Lines | ||||||||||||||||||
| isOnlyCopiedFromConstantMemory(AAResults *AA, AllocaInst *V, | isOnlyCopiedFromConstantMemory(AAResults *AA, AllocaInst *V, | |||||||||||||||||
| MemTransferInst *&TheCopy, | MemTransferInst *&TheCopy, | |||||||||||||||||
| SmallVectorImpl<Instruction *> &ToDelete) { | SmallVectorImpl<Instruction *> &ToDelete) { | |||||||||||||||||
| // We track lifetime intrinsics as we encounter them. If we decide to go | // We track lifetime intrinsics as we encounter them. If we decide to go | |||||||||||||||||
| // ahead and replace the value with the memory location, this lets the caller | // ahead and replace the value with the memory location, this lets the caller | |||||||||||||||||
| // quickly eliminate the markers. | // quickly eliminate the markers. | |||||||||||||||||
| SmallVector<PointerIntPair<Value *, 1, bool>, 35> ValuesToInspect; | SmallVector<PointerIntPair<Value *, 1, bool>, 35> ValuesToInspect; | |||||||||||||||||
| DenseSet<PHINode *> SeenPHIs; | ||||||||||||||||||
| ValuesToInspect.emplace_back(V, false); | ValuesToInspect.emplace_back(V, false); | |||||||||||||||||
| while (!ValuesToInspect.empty()) { | while (!ValuesToInspect.empty()) { | |||||||||||||||||
| const auto [Value, IsOffset] = ValuesToInspect.pop_back_val(); | const auto [Value, IsOffset] = ValuesToInspect.pop_back_val(); | |||||||||||||||||
Use the new C++17 tuple syntax, arsenm: Use the new C++17 tuple syntax,
| ||||||||||||||||||
| for (auto &U : Value->uses()) { | for (auto &U : Value->uses()) { | |||||||||||||||||
| auto *I = cast<Instruction>(U.getUser()); | auto *I = cast<Instruction>(U.getUser()); | |||||||||||||||||
Why do we need the LastPHI variable? I'd have expected that just setting IsOffset=true is equivalent? (With a rename IsOffset -> ForbidCopy or something?) nikic: Why do we need the LastPHI variable? I'd have expected that just setting `IsOffset=true` is… | ||||||||||||||||||
Spurious change. nikic: Spurious change. | ||||||||||||||||||
| if (auto *LI = dyn_cast<LoadInst>(I)) { | if (auto *LI = dyn_cast<LoadInst>(I)) { | |||||||||||||||||
Uh, why can we just skip phi nodes here? Don't they need to be added to the worklist? nikic: Uh, why can we just skip phi nodes here? Don't they need to be added to the worklist? | ||||||||||||||||||
| // Ignore non-volatile loads, they are always ok. | // Ignore non-volatile loads, they are always ok. | |||||||||||||||||
| if (!LI->isSimple()) return false; | if (!LI->isSimple()) return false; | |||||||||||||||||
| continue; | continue; | |||||||||||||||||
| } | } | |||||||||||||||||
| if (isa<PHINode>(I) && !SeenPHIs.contains(cast<PHINode>(I))) { | ||||||||||||||||||
| if (isa<BitCastInst>(I) || isa<AddrSpaceCastInst>(I)) { | ValuesToInspect.emplace_back(I, true); | |||||||||||||||||
| SeenPHIs.insert(cast<PHINode>(I)); | ||||||||||||||||||
Why do we care about default address space here? I think what you probably want to check is PHI->getType()->getPointerAddressSpace() != TheCopy->getPointerAddressSpace()? But also, this code assumes that TheCopy is available before the PHI is seen, and I don't think anything guarantees that -- order of uses is unpredictable. I think you'd want to perform the bailout in PointerReplacer::collectUsers() instead? Maybe I'm misunderstanding the purpose here. nikic: Why do we care about default address space here? I think what you probably want to check is… | ||||||||||||||||||
I implemented it for the function @addrspace_diff_keep_alloca in the test file. I improved upon the solution by moving the changes over to collectUsers() which does make a lot more sense wrt my previous solution. I hope it seems fine now. gandhi21299: I implemented it for the function @addrspace_diff_keep_alloca in the test file. I improved upon… | ||||||||||||||||||
| continue; | ||||||||||||||||||
| } | ||||||||||||||||||
| if (isa<BitCastInst, AddrSpaceCastInst>(I)) { | ||||||||||||||||||
Cosmetic review: What do you think about using the varadic template version of isa ? You could collapse this into isa<BitCastInst, AddrSpaceCastInst, PHINode>(I) jmmartinez: Cosmetic review: What do you think about using the varadic template version of `isa` ? You… | ||||||||||||||||||
| // If uses of the bitcast are ok, we are ok. | // If uses of the bitcast are ok, we are ok. | |||||||||||||||||
| ValuesToInspect.emplace_back(I, IsOffset); | ValuesToInspect.emplace_back(I, IsOffset); | |||||||||||||||||
| continue; | continue; | |||||||||||||||||
| } | } | |||||||||||||||||
| if (auto *GEP = dyn_cast<GetElementPtrInst>(I)) { | if (auto *GEP = dyn_cast<GetElementPtrInst>(I)) { | |||||||||||||||||
| // If the GEP has all zero indices, it doesn't offset the pointer. If it | // If the GEP has all zero indices, it doesn't offset the pointer. If it | |||||||||||||||||
| // doesn't, it does. | // doesn't, it does. | |||||||||||||||||
| ValuesToInspect.emplace_back(I, IsOffset || !GEP->hasAllZeroIndices()); | ValuesToInspect.emplace_back(I, IsOffset || !GEP->hasAllZeroIndices()); | |||||||||||||||||
| Show All 30 Lines | for (auto &U : Value->uses()) { | |||||||||||||||||
| // Lifetime intrinsics can be handled by the caller. | // Lifetime intrinsics can be handled by the caller. | |||||||||||||||||
| if (I->isLifetimeStartOrEnd()) { | if (I->isLifetimeStartOrEnd()) { | |||||||||||||||||
| assert(I->use_empty() && "Lifetime markers have no result to use!"); | assert(I->use_empty() && "Lifetime markers have no result to use!"); | |||||||||||||||||
| ToDelete.push_back(I); | ToDelete.push_back(I); | |||||||||||||||||
| continue; | continue; | |||||||||||||||||
| } | } | |||||||||||||||||
| // If this is isn't our memcpy/memmove, reject it as something we can't | // If this is isn't our memcpy/memmove, reject it as something we can't | |||||||||||||||||
| // handle. | // handle. If a PHI was already defined, then this MemTransferInst is a | |||||||||||||||||
| // descendant of the PHI. Reject this case as well. | ||||||||||||||||||
| MemTransferInst *MI = dyn_cast<MemTransferInst>(I); | MemTransferInst *MI = dyn_cast<MemTransferInst>(I); | |||||||||||||||||
| if (!MI) | if (!MI) | |||||||||||||||||
This is not sufficient: E.g. consider doing the memcpy to an addrspacecast of the phi or similar. I think what we'd need instead is that set IsOffset=true for phi nodes, so we'll recursively reject memcpys into it. Of course we would need a different name than IsOffset in that case. It would more generally indicate that the pointer is not equivalent to the base. nikic: This is not sufficient: E.g. consider doing the memcpy to an addrspacecast of the phi or… | ||||||||||||||||||
@nikic What do you mean by "need a different name"? I could simply emplace_back a pair of the instruction and a true to ValuesToInspect vector right? gandhi21299: @nikic What do you mean by "need a different name"? I could simply emplace_back a pair of the… | ||||||||||||||||||
I think I'm getting lost in the different revisions of the patch; not sure what this comment is referring to anymore. There isn't anything about phi handling right here? arsenm: I think I'm getting lost in the different revisions of the patch; not sure what this comment is… | ||||||||||||||||||
I had a revision previously where I was tracking PHI nodes seperately, as opposed to IsOffset. I replaced it with my changes in PointerReplacer::collectUsers(..). gandhi21299: I had a revision previously where I was tracking PHI nodes seperately, as opposed to `IsOffset`. | ||||||||||||||||||
| return false; | return false; | |||||||||||||||||
| // If the transfer is volatile, reject it. | // If the transfer is volatile, reject it. | |||||||||||||||||
| if (MI->isVolatile()) | if (MI->isVolatile()) | |||||||||||||||||
| return false; | return false; | |||||||||||||||||
| // If the transfer is using the alloca as a source of the transfer, then | // If the transfer is using the alloca as a source of the transfer, then | |||||||||||||||||
| // ignore it since it is a load (unless the transfer is volatile). | // ignore it since it is a load (unless the transfer is volatile). | |||||||||||||||||
| ▲ Show 20 Lines • Show All 116 Lines • ▼ Show 20 Lines | ||||||||||||||||||
| // instructions, then replaces the old pointer in the load instructions with | // instructions, then replaces the old pointer in the load instructions with | |||||||||||||||||
| // the new pointer. If during the chasing it sees bitcast or GEP, it will | // the new pointer. If during the chasing it sees bitcast or GEP, it will | |||||||||||||||||
| // create new bitcast or GEP with the new pointer and use them in the load | // create new bitcast or GEP with the new pointer and use them in the load | |||||||||||||||||
| // instruction. | // instruction. | |||||||||||||||||
| class PointerReplacer { | class PointerReplacer { | |||||||||||||||||
| public: | public: | |||||||||||||||||
| PointerReplacer(InstCombinerImpl &IC) : IC(IC) {} | PointerReplacer(InstCombinerImpl &IC) : IC(IC) {} | |||||||||||||||||
| bool collectUsers(Instruction &I); | bool collectUsers(Instruction &I, MemTransferInst *Copy); | |||||||||||||||||
Copy argument no longer needed nikic: Copy argument no longer needed | ||||||||||||||||||
| void replacePointer(Instruction &I, Value *V); | void replacePointer(Instruction &I, Value *V); | |||||||||||||||||
| private: | private: | |||||||||||||||||
| void replace(Instruction *I); | void replace(Instruction *I); | |||||||||||||||||
| Value *getReplacement(Value *I); | Value *getReplacement(Value *I); | |||||||||||||||||
| SmallSetVector<Instruction *, 4> Worklist; | SmallSetVector<Instruction *, 4> Worklist; | |||||||||||||||||
| MapVector<Value *, Value *> WorkMap; | MapVector<Value *, Value *> WorkMap; | |||||||||||||||||
| InstCombinerImpl &IC; | InstCombinerImpl &IC; | |||||||||||||||||
| }; | }; | |||||||||||||||||
| } // end anonymous namespace | } // end anonymous namespace | |||||||||||||||||
| bool PointerReplacer::collectUsers(Instruction &I) { | bool PointerReplacer::collectUsers(Instruction &I, MemTransferInst *Copy) { | |||||||||||||||||
| for (auto *U : I.users()) { | for (auto *U : I.users()) { | |||||||||||||||||
| auto *Inst = cast<Instruction>(&*U); | auto *Inst = cast<Instruction>(&*U); | |||||||||||||||||
| if (auto *Load = dyn_cast<LoadInst>(Inst)) { | if (auto *Load = dyn_cast<LoadInst>(Inst)) { | |||||||||||||||||
| if (Load->isVolatile()) | if (Load->isVolatile()) | |||||||||||||||||
| return false; | return false; | |||||||||||||||||
| Worklist.insert(Load); | Worklist.insert(Load); | |||||||||||||||||
| } else if (isa<GetElementPtrInst>(Inst) || isa<BitCastInst>(Inst)) { | } else if (auto *PHI = dyn_cast<PHINode>(Inst)) { | |||||||||||||||||
| // Check if any of the incoming values of PHI is the destination of Copy | ||||||||||||||||||
Could make ValuesToRevisit use Instruction * rather than Value * and save this cast. nikic: Could make ValuesToRevisit use `Instruction *` rather than `Value *` and save this cast. | ||||||||||||||||||
| unsigned CopySrcAddrSpace = Copy->getSourceAddressSpace(); | ||||||||||||||||||
Why do we need to add the phi incoming values to the worklist? Can't this use the same code as bitcast/GEP? Also, I think it would be good to add a test involving a loop phi, because I think as written, this code might go into infinite recursion in that case. nikic: Why do we need to add the phi incoming values to the worklist? Can't this use the same code as… | ||||||||||||||||||
I suppose the incoming values would have already been inserted prior to collecting users of the PHI. Thanks! A loop test sounds like a great idea! gandhi21299: I suppose the incoming values would have already been inserted prior to collecting users of the… | ||||||||||||||||||
@nikic Never mind, the incoming values are missing in the WorkMap when PHIs are being replaced replace if I skip inserting them. gandhi21299: @nikic Never mind, the incoming values are missing in the `WorkMap` when PHIs are being… | ||||||||||||||||||
Don't like auto for addrspace arsenm: Don't like auto for addrspace | ||||||||||||||||||
Drop llvm:: prefix. nikic: Drop `llvm::` prefix. | ||||||||||||||||||
| unsigned PHIAddrSpace = PHI->getType()->getPointerAddressSpace(); | ||||||||||||||||||
Maybe in a separate patch, but if you're touching this, might as well handle select too arsenm: Maybe in a separate patch, but if you're touching this, might as well handle select too | ||||||||||||||||||
Sounds good, I will handle select in a seperate patch. gandhi21299: Sounds good, I will handle select in a seperate patch. | ||||||||||||||||||
| for (unsigned Idx = 0; Idx < PHI->getNumIncomingValues(); ++Idx) { | ||||||||||||||||||
| auto *V = PHI->getIncomingValue(Idx); | ||||||||||||||||||
| if (CopySrcAddrSpace != PHIAddrSpace && V == Copy->getDest()) | ||||||||||||||||||
| return false; | ||||||||||||||||||
| if (auto *Inst = dyn_cast<Instruction>(V)) | ||||||||||||||||||
| Worklist.insert(Inst); | Worklist.insert(Inst); | |||||||||||||||||
Missing the insertion into ValuesToRevisit here? nikic: Missing the insertion into ValuesToRevisit here? | ||||||||||||||||||
Unneeded Inst capture. nikic: Unneeded `Inst` capture. | ||||||||||||||||||
| if (!collectUsers(*Inst)) | } | |||||||||||||||||
| Worklist.insert(PHI); | ||||||||||||||||||
| if (!collectUsers(*PHI, Copy)) | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } else if (isa<GetElementPtrInst, BitCastInst>(Inst)) { | ||||||||||||||||||
Can use any_of instead of for here and avoid the ValueInserted variable. nikic: Can use `any_of` instead of `for` here and avoid the ValueInserted variable. | ||||||||||||||||||
The ValuesToRevisit update should happen outside the any_of. nikic: The ValuesToRevisit update should happen outside the any_of. | ||||||||||||||||||
| Worklist.insert(Inst); | ||||||||||||||||||
| if (!collectUsers(*Inst, Copy)) | ||||||||||||||||||
| return false; | return false; | |||||||||||||||||
| } else if (auto *MI = dyn_cast<MemTransferInst>(Inst)) { | } else if (auto *MI = dyn_cast<MemTransferInst>(Inst)) { | |||||||||||||||||
| if (MI->isVolatile()) | if (MI->isVolatile()) | |||||||||||||||||
| return false; | return false; | |||||||||||||||||
| Worklist.insert(Inst); | Worklist.insert(Inst); | |||||||||||||||||
I don't think you need this check (and as such this whole loop can be dropped). nikic: I don't think you need this check (and as such this whole loop can be dropped). | ||||||||||||||||||
| } else if (Inst->isLifetimeStartOrEnd()) { | } else if (Inst->isLifetimeStartOrEnd()) { | |||||||||||||||||
| continue; | continue; | |||||||||||||||||
| } else { | } else { | |||||||||||||||||
| LLVM_DEBUG(dbgs() << "Cannot handle pointer user: " << *U << '\n'); | LLVM_DEBUG(dbgs() << "Cannot handle pointer user: " << *U << '\n'); | |||||||||||||||||
| return false; | return false; | |||||||||||||||||
| } | } | |||||||||||||||||
| } | } | |||||||||||||||||
Spurious change nikic: Spurious change | ||||||||||||||||||
| return true; | return true; | |||||||||||||||||
| } | } | |||||||||||||||||
| Value *PointerReplacer::getReplacement(Value *V) { return WorkMap.lookup(V); } | Value *PointerReplacer::getReplacement(Value *V) { return WorkMap.lookup(V); } | |||||||||||||||||
I think this should only be done after the whole collectUsers() phase, currently you do it on each recursive call. nikic: I think this should only be done after the whole collectUsers() phase, currently you do it on… | ||||||||||||||||||
| void PointerReplacer::replace(Instruction *I) { | void PointerReplacer::replace(Instruction *I) { | |||||||||||||||||
| if (getReplacement(I)) | if (getReplacement(I)) | |||||||||||||||||
| return; | return; | |||||||||||||||||
| if (auto *LT = dyn_cast<LoadInst>(I)) { | if (auto *LT = dyn_cast<LoadInst>(I)) { | |||||||||||||||||
| auto *V = getReplacement(LT->getPointerOperand()); | auto *V = getReplacement(LT->getPointerOperand()); | |||||||||||||||||
| assert(V && "Operand not replaced"); | assert(V && "Operand not replaced"); | |||||||||||||||||
| auto *NewI = new LoadInst(LT->getType(), V, "", LT->isVolatile(), | auto *NewI = new LoadInst(LT->getType(), V, "", LT->isVolatile(), | |||||||||||||||||
| LT->getAlign(), LT->getOrdering(), | LT->getAlign(), LT->getOrdering(), | |||||||||||||||||
| LT->getSyncScopeID()); | LT->getSyncScopeID()); | |||||||||||||||||
| NewI->takeName(LT); | NewI->takeName(LT); | |||||||||||||||||
| copyMetadataForLoad(*NewI, *LT); | copyMetadataForLoad(*NewI, *LT); | |||||||||||||||||
| IC.InsertNewInstWith(NewI, *LT); | IC.InsertNewInstWith(NewI, *LT); | |||||||||||||||||
| IC.replaceInstUsesWith(*LT, NewI); | IC.replaceInstUsesWith(*LT, NewI); | |||||||||||||||||
| WorkMap[LT] = NewI; | WorkMap[LT] = NewI; | |||||||||||||||||
| } else if (auto *PHI = dyn_cast<PHINode>(I)) { | ||||||||||||||||||
| Type *NewTy = getReplacement(PHI->getIncomingValue(0))->getType(); | ||||||||||||||||||
| auto *NewPHI = PHINode::Create(NewTy, PHI->getNumIncomingValues(), | ||||||||||||||||||
Use operands() iterator? Also, where is ReplacedOperands actually being used? nikic: Use `operands()` iterator? Also, where is `ReplacedOperands` actually being used? | ||||||||||||||||||
| PHI->getName(), PHI); | ||||||||||||||||||
| for (unsigned int I = 0; I < PHI->getNumIncomingValues(); ++I) | ||||||||||||||||||
| NewPHI->addIncoming(getReplacement(PHI->getIncomingValue(I)), | ||||||||||||||||||
| PHI->getIncomingBlock(I)); | ||||||||||||||||||
| WorkMap[PHI] = NewPHI; | ||||||||||||||||||
| } else if (auto *GEP = dyn_cast<GetElementPtrInst>(I)) { | } else if (auto *GEP = dyn_cast<GetElementPtrInst>(I)) { | |||||||||||||||||
| auto *V = getReplacement(GEP->getPointerOperand()); | auto *V = getReplacement(GEP->getPointerOperand()); | |||||||||||||||||
| assert(V && "Operand not replaced"); | assert(V && "Operand not replaced"); | |||||||||||||||||
| SmallVector<Value *, 8> Indices; | SmallVector<Value *, 8> Indices; | |||||||||||||||||
| Indices.append(GEP->idx_begin(), GEP->idx_end()); | Indices.append(GEP->idx_begin(), GEP->idx_end()); | |||||||||||||||||
| auto *NewI = | auto *NewI = | |||||||||||||||||
| GetElementPtrInst::Create(GEP->getSourceElementType(), V, Indices); | GetElementPtrInst::Create(GEP->getSourceElementType(), V, Indices); | |||||||||||||||||
| IC.InsertNewInstWith(NewI, *GEP); | IC.InsertNewInstWith(NewI, *GEP); | |||||||||||||||||
| NewI->takeName(GEP); | NewI->takeName(GEP); | |||||||||||||||||
| WorkMap[GEP] = NewI; | WorkMap[GEP] = NewI; | |||||||||||||||||
| } else if (auto *BC = dyn_cast<BitCastInst>(I)) { | } else if (auto *BC = dyn_cast<BitCastInst>(I)) { | |||||||||||||||||
| auto *V = getReplacement(BC->getOperand(0)); | auto *V = getReplacement(BC->getOperand(0)); | |||||||||||||||||
| assert(V && "Operand not replaced"); | assert(V && "Operand not replaced"); | |||||||||||||||||
| auto *NewT = PointerType::getWithSamePointeeType( | auto *NewT = PointerType::getWithSamePointeeType( | |||||||||||||||||
| cast<PointerType>(BC->getType()), | cast<PointerType>(BC->getType()), | |||||||||||||||||
| V->getType()->getPointerAddressSpace()); | V->getType()->getPointerAddressSpace()); | |||||||||||||||||
| auto *NewI = new BitCastInst(V, NewT); | auto *NewI = new BitCastInst(V, NewT); | |||||||||||||||||
| IC.InsertNewInstWith(NewI, *BC); | IC.InsertNewInstWith(NewI, *BC); | |||||||||||||||||
| NewI->takeName(BC); | NewI->takeName(BC); | |||||||||||||||||
In general InstCombine can't introduce a new address space cast when one didn't exist in the first place arsenm: In general InstCombine can't introduce a new address space cast when one didn't exist in the… | ||||||||||||||||||
If an incoming value is eligible to be replaced with a pointer from a different addrspace, other incoming values likely do not belong to the addrspace. This makes addrspacecast unavoidable. gandhi21299: If an incoming value is eligible to be replaced with a pointer from a different addrspace… | ||||||||||||||||||
Right, so I'm saying that would just make this illegal and you need to check the address spaces match. Is there a situation where we would want this to happen, that also isn't possible after InferAddressSpaces? arsenm: Right, so I'm saying that would just make this illegal and you need to check the address spaces… | ||||||||||||||||||
The function @addrspace_cast_remove_alloca in the test I have attached addresses this issue. gandhi21299: The function @addrspace_cast_remove_alloca in the test I have attached addresses this issue. | ||||||||||||||||||
| WorkMap[BC] = NewI; | WorkMap[BC] = NewI; | |||||||||||||||||
| } else if (auto *MemCpy = dyn_cast<MemTransferInst>(I)) { | } else if (auto *MemCpy = dyn_cast<MemTransferInst>(I)) { | |||||||||||||||||
| auto *SrcV = getReplacement(MemCpy->getRawSource()); | auto *SrcV = getReplacement(MemCpy->getRawSource()); | |||||||||||||||||
| // The pointer may appear in the destination of a copy, but we don't want to | // The pointer may appear in the destination of a copy, but we don't want to | |||||||||||||||||
| // replace it. | // replace it. | |||||||||||||||||
| if (!SrcV) { | if (!SrcV) { | |||||||||||||||||
| assert(getReplacement(MemCpy->getRawDest()) && | assert(getReplacement(MemCpy->getRawDest()) && | |||||||||||||||||
| "destination not in replace list"); | "destination not in replace list"); | |||||||||||||||||
Ditto arsenm: Ditto | ||||||||||||||||||
| return; | return; | |||||||||||||||||
| } | } | |||||||||||||||||
| IC.Builder.SetInsertPoint(MemCpy); | IC.Builder.SetInsertPoint(MemCpy); | |||||||||||||||||
| auto *NewI = IC.Builder.CreateMemTransferInst( | auto *NewI = IC.Builder.CreateMemTransferInst( | |||||||||||||||||
| MemCpy->getIntrinsicID(), MemCpy->getRawDest(), MemCpy->getDestAlign(), | MemCpy->getIntrinsicID(), MemCpy->getRawDest(), MemCpy->getDestAlign(), | |||||||||||||||||
| SrcV, MemCpy->getSourceAlign(), MemCpy->getLength(), | SrcV, MemCpy->getSourceAlign(), MemCpy->getLength(), | |||||||||||||||||
| MemCpy->isVolatile()); | MemCpy->isVolatile()); | |||||||||||||||||
| ▲ Show 20 Lines • Show All 91 Lines • ▼ Show 20 Lines | if (AllocaAlign <= SourceAlign && | |||||||||||||||||
| Value *Cast = Builder.CreateBitCast(TheSrc, DestTy); | Value *Cast = Builder.CreateBitCast(TheSrc, DestTy); | |||||||||||||||||
| Instruction *NewI = replaceInstUsesWith(AI, Cast); | Instruction *NewI = replaceInstUsesWith(AI, Cast); | |||||||||||||||||
| eraseInstFromFunction(*Copy); | eraseInstFromFunction(*Copy); | |||||||||||||||||
| ++NumGlobalCopies; | ++NumGlobalCopies; | |||||||||||||||||
| return NewI; | return NewI; | |||||||||||||||||
| } | } | |||||||||||||||||
| PointerReplacer PtrReplacer(*this); | PointerReplacer PtrReplacer(*this); | |||||||||||||||||
| if (PtrReplacer.collectUsers(AI)) { | if (PtrReplacer.collectUsers(AI, Copy)) { | |||||||||||||||||
| for (Instruction *Delete : ToDelete) | for (Instruction *Delete : ToDelete) | |||||||||||||||||
| eraseInstFromFunction(*Delete); | eraseInstFromFunction(*Delete); | |||||||||||||||||
Spurious change. nikic: Spurious change. | ||||||||||||||||||
| Value *Cast = Builder.CreateBitCast(TheSrc, DestTy); | Value *Cast = Builder.CreateBitCast(TheSrc, DestTy); | |||||||||||||||||
| PtrReplacer.replacePointer(AI, Cast); | PtrReplacer.replacePointer(AI, Cast); | |||||||||||||||||
| ++NumGlobalCopies; | ++NumGlobalCopies; | |||||||||||||||||
| } | } | |||||||||||||||||
| } | } | |||||||||||||||||
| } | } | |||||||||||||||||
| // At last, use the generic allocation site handler to aggressively remove | // At last, use the generic allocation site handler to aggressively remove | |||||||||||||||||
| ▲ Show 20 Lines • Show All 1,124 Lines • Show Last 20 Lines | ||||||||||||||||||
Shouldn't be needed (already used in file).