[llvm-commits] [llvm] r164925 - in /llvm/trunk: lib/Transforms/Scalar/SROA.cpp test/Transforms/SROA/phi-and-select.ll
Chandler Carruth
chandlerc at gmail.com
Sun Sep 30 18:49:22 PDT 2012
Author: chandlerc
Date: Sun Sep 30 20:49:22 2012
New Revision: 164925
URL: http://llvm.org/viewvc/llvm-project?rev=164925&view=rev
Log:
Refactor the PartitionUse structure to actually use the Use* instead of
a pair of instructions, one for the used pointer and the second for the
user. This simplifies the representation and also makes it more dense.
This was noticed because of the miscompile in PR13926. In that case, we
were running up against a fundamental "bad idea" in the speculation of
PHI and select instructions: the speculation and rewriting are
interleaved, which requires phi speculation to also perform load
rewriting! This is bad, and causes us to miss opportunities to do (for
example) vector rewriting only exposed after PHI speculation, etc etc.
It also, in the old system, required us to insert *new* load uses into
the current partition's use list, which would then be ignored during
rewriting because we had already extracted an end iterator for the use
list. The appending behavior (and much of the other oddities) stem from
the strange de-duplication strategy in the PartitionUse builder.
Amusingly, all this went without notice for so long because it could
only be triggered by having *different* GEPs into the same partition of
the same alloca, where both different GEPs were operands of a single
PHI, and where the GEP which was not encountered first also had multiple
uses within that same PHI node... Hence the insane steps required to
reproduce.
So, step one in fixing this fundamental bad idea is to make the
PartitionUse actually contain a Use*, and to make the builder do proper
deduplication instead of funky de-duplication. This is enough to remove
the appending behavior, and fix the miscompile in PR13926, but there is
more work to be done here. Subsequent commits will lift the speculation
into its own visitor. It'll be a useful step toward potentially
extracting all of the speculation logic into a generic utility
transform.
The existing PHI test case for repeated operands has been made more
extreme to catch even these issues. This test case, run through the old
pass, will exactly reproduce the miscompile from PR13926. ;] We were so
close here!
Modified:
llvm/trunk/lib/Transforms/Scalar/SROA.cpp
llvm/trunk/test/Transforms/SROA/phi-and-select.ll
Modified: llvm/trunk/lib/Transforms/Scalar/SROA.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SROA.cpp?rev=164925&r1=164924&r2=164925&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/SROA.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/SROA.cpp Sun Sep 30 20:49:22 2012
@@ -155,16 +155,12 @@
/// memory use itself with a particular partition. As a consequence there is
/// intentionally overlap between various uses of the same partition.
struct PartitionUse : public ByteRange {
- /// \brief The user of this range of the alloca.
- AssertingVH<Instruction> User;
+ /// \brief The use in question. Provides access to both user and used value.
+ Use* U;
- /// \brief The particular pointer value derived from this alloca in use.
- AssertingVH<Instruction> Ptr;
-
- PartitionUse() : ByteRange(), User(), Ptr() {}
- PartitionUse(uint64_t BeginOffset, uint64_t EndOffset,
- Instruction *User, Instruction *Ptr)
- : ByteRange(BeginOffset, EndOffset), User(User), Ptr(Ptr) {}
+ PartitionUse() : ByteRange(), U() {}
+ PartitionUse(uint64_t BeginOffset, uint64_t EndOffset, Use *U)
+ : ByteRange(BeginOffset, EndOffset), U(U) {}
};
/// \brief Construct a partitioning of a particular alloca.
@@ -202,11 +198,11 @@
use_iterator use_begin(const_iterator I) { return Uses[I - begin()].begin(); }
use_iterator use_end(unsigned Idx) { return Uses[Idx].end(); }
use_iterator use_end(const_iterator I) { return Uses[I - begin()].end(); }
- void use_push_back(unsigned Idx, const PartitionUse &U) {
- Uses[Idx].push_back(U);
+ void use_push_back(unsigned Idx, const PartitionUse &PU) {
+ Uses[Idx].push_back(PU);
}
- void use_push_back(const_iterator I, const PartitionUse &U) {
- Uses[I - begin()].push_back(U);
+ void use_push_back(const_iterator I, const PartitionUse &PU) {
+ Uses[I - begin()].push_back(PU);
}
void use_erase(unsigned Idx, use_iterator UI) { Uses[Idx].erase(UI); }
void use_erase(const_iterator I, use_iterator UI) {
@@ -267,10 +263,9 @@
/// When manipulating PHI nodes or selects, they can use more than one
/// partition of an alloca. We store a special mapping to allow finding the
/// partition referenced by each of these operands, if any.
- iterator findPartitionForPHIOrSelectOperand(Instruction &I, Value *Op) {
- SmallDenseMap<std::pair<Instruction *, Value *>,
- std::pair<unsigned, unsigned> >::const_iterator MapIt
- = PHIOrSelectOpMap.find(std::make_pair(&I, Op));
+ iterator findPartitionForPHIOrSelectOperand(Use *U) {
+ SmallDenseMap<Use *, std::pair<unsigned, unsigned> >::const_iterator MapIt
+ = PHIOrSelectOpMap.find(U);
if (MapIt == PHIOrSelectOpMap.end())
return end();
@@ -282,11 +277,9 @@
///
/// Similar to mapping these operands back to the partitions, this maps
/// directly to the use structure of that partition.
- use_iterator findPartitionUseForPHIOrSelectOperand(Instruction &I,
- Value *Op) {
- SmallDenseMap<std::pair<Instruction *, Value *>,
- std::pair<unsigned, unsigned> >::const_iterator MapIt
- = PHIOrSelectOpMap.find(std::make_pair(&I, Op));
+ use_iterator findPartitionUseForPHIOrSelectOperand(Use *U) {
+ SmallDenseMap<Use *, std::pair<unsigned, unsigned> >::const_iterator MapIt
+ = PHIOrSelectOpMap.find(U);
assert(MapIt != PHIOrSelectOpMap.end());
return Uses[MapIt->second.first].begin() + MapIt->second.second;
}
@@ -373,8 +366,7 @@
SmallDenseMap<Instruction *, std::pair<uint64_t, bool> > PHIOrSelectSizes;
/// \brief Auxiliary information for particular PHI or select operands.
- SmallDenseMap<std::pair<Instruction *, Value *>,
- std::pair<unsigned, unsigned>, 4> PHIOrSelectOpMap;
+ SmallDenseMap<Use *, std::pair<unsigned, unsigned>, 4> PHIOrSelectOpMap;
/// \brief A utility routine called from the constructor.
///
@@ -401,6 +393,8 @@
const uint64_t AllocSize;
AllocaPartitioning &P;
+ SmallPtrSet<Use *, 8> VisitedUses;
+
struct OffsetUse {
Use *U;
int64_t Offset;
@@ -412,14 +406,12 @@
int64_t Offset;
void enqueueUsers(Instruction &I, int64_t UserOffset) {
- SmallPtrSet<User *, 8> UserSet;
for (Value::use_iterator UI = I.use_begin(), UE = I.use_end();
UI != UE; ++UI) {
- if (!UserSet.insert(*UI))
- continue;
-
- OffsetUse OU = { &UI.getUse(), UserOffset };
- Queue.push_back(OU);
+ if (VisitedUses.insert(&UI.getUse())) {
+ OffsetUse OU = { &UI.getUse(), UserOffset };
+ Queue.push_back(OU);
+ }
}
}
@@ -858,12 +850,11 @@
B = llvm::prior(B);
for (iterator I = B, E = P.end(); I != E && I->BeginOffset < EndOffset;
++I) {
- PartitionUse NewUse(std::max(I->BeginOffset, BeginOffset),
- std::min(I->EndOffset, EndOffset),
- &User, cast<Instruction>(*U));
- P.use_push_back(I, NewUse);
+ PartitionUse NewPU(std::max(I->BeginOffset, BeginOffset),
+ std::min(I->EndOffset, EndOffset), U);
+ P.use_push_back(I, NewPU);
if (isa<PHINode>(U->getUser()) || isa<SelectInst>(U->getUser()))
- P.PHIOrSelectOpMap[std::make_pair(&User, U->get())]
+ P.PHIOrSelectOpMap[U]
= std::make_pair(I - P.begin(), P.Uses[I - P.begin()].size() - 1);
}
}
@@ -1115,20 +1106,20 @@
Type *AllocaPartitioning::getCommonType(iterator I) const {
Type *Ty = 0;
for (const_use_iterator UI = use_begin(I), UE = use_end(I); UI != UE; ++UI) {
- if (isa<IntrinsicInst>(*UI->User))
+ if (isa<IntrinsicInst>(*UI->U->getUser()))
continue;
if (UI->BeginOffset != I->BeginOffset || UI->EndOffset != I->EndOffset)
continue;
Type *UserTy = 0;
- if (LoadInst *LI = dyn_cast<LoadInst>(&*UI->User)) {
+ if (LoadInst *LI = dyn_cast<LoadInst>(UI->U->getUser())) {
UserTy = LI->getType();
- } else if (StoreInst *SI = dyn_cast<StoreInst>(&*UI->User)) {
+ } else if (StoreInst *SI = dyn_cast<StoreInst>(UI->U->getUser())) {
UserTy = SI->getValueOperand()->getType();
- } else if (SelectInst *SI = dyn_cast<SelectInst>(&*UI->User)) {
+ } else if (SelectInst *SI = dyn_cast<SelectInst>(UI->U->getUser())) {
if (PointerType *PtrTy = dyn_cast<PointerType>(SI->getType()))
UserTy = PtrTy->getElementType();
- } else if (PHINode *PN = dyn_cast<PHINode>(&*UI->User)) {
+ } else if (PHINode *PN = dyn_cast<PHINode>(UI->U->getUser())) {
if (PointerType *PtrTy = dyn_cast<PointerType>(PN->getType()))
UserTy = PtrTy->getElementType();
}
@@ -1157,8 +1148,8 @@
for (const_use_iterator UI = use_begin(I), UE = use_end(I);
UI != UE; ++UI) {
OS << Indent << " [" << UI->BeginOffset << "," << UI->EndOffset << ") "
- << "used by: " << *UI->User << "\n";
- if (MemTransferInst *II = dyn_cast<MemTransferInst>(&*UI->User)) {
+ << "used by: " << *UI->U->getUser() << "\n";
+ if (MemTransferInst *II = dyn_cast<MemTransferInst>(UI->U->getUser())) {
const MemTransferOffsets &MTO = MemTransferInstData.lookup(II);
bool IsDest;
if (!MTO.IsSplittable)
@@ -1710,19 +1701,20 @@
(EndOffset - BeginOffset) != VecSize)
return false;
- if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(&*I->User)) {
+ if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I->U->getUser())) {
if (MI->isVolatile())
return false;
- if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(&*I->User)) {
+ if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(I->U->getUser())) {
const AllocaPartitioning::MemTransferOffsets &MTO
= P.getMemTransferOffsets(*MTI);
if (!MTO.IsSplittable)
return false;
}
- } else if (I->Ptr->getType()->getPointerElementType()->isStructTy()) {
+ } else if (I->U->get()->getType()->getPointerElementType()->isStructTy()) {
// Disable vector promotion when there are loads or stores of an FCA.
return false;
- } else if (!isa<LoadInst>(*I->User) && !isa<StoreInst>(*I->User)) {
+ } else if (!isa<LoadInst>(I->U->getUser()) &&
+ !isa<StoreInst>(I->U->getUser())) {
return false;
}
}
@@ -1751,20 +1743,20 @@
// splittable later) and lose the ability to promote each element access.
bool WholeAllocaOp = false;
for (; I != E; ++I) {
- if (LoadInst *LI = dyn_cast<LoadInst>(&*I->User)) {
+ if (LoadInst *LI = dyn_cast<LoadInst>(I->U->getUser())) {
if (LI->isVolatile() || !LI->getType()->isIntegerTy())
return false;
if (LI->getType() == Ty)
WholeAllocaOp = true;
- } else if (StoreInst *SI = dyn_cast<StoreInst>(&*I->User)) {
+ } else if (StoreInst *SI = dyn_cast<StoreInst>(I->U->getUser())) {
if (SI->isVolatile() || !SI->getValueOperand()->getType()->isIntegerTy())
return false;
if (SI->getValueOperand()->getType() == Ty)
WholeAllocaOp = true;
- } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(&*I->User)) {
+ } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I->U->getUser())) {
if (MI->isVolatile())
return false;
- if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(&*I->User)) {
+ if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(I->U->getUser())) {
const AllocaPartitioning::MemTransferOffsets &MTO
= P.getMemTransferOffsets(*MTI);
if (!MTO.IsSplittable)
@@ -1816,6 +1808,7 @@
// The offset of the partition user currently being rewritten.
uint64_t BeginOffset, EndOffset;
+ Use *OldUse;
Instruction *OldPtr;
// The name prefix to use when rewriting instructions for this alloca.
@@ -1854,9 +1847,10 @@
for (; I != E; ++I) {
BeginOffset = I->BeginOffset;
EndOffset = I->EndOffset;
- OldPtr = I->Ptr;
+ OldUse = I->U;
+ OldPtr = cast<Instruction>(I->U->get());
NamePrefix = (Twine(NewAI.getName()) + "." + Twine(BeginOffset)).str();
- CanSROA &= visit(I->User);
+ CanSROA &= visit(cast<Instruction>(I->U->getUser()));
}
if (VecTy) {
assert(CanSROA);
@@ -2471,7 +2465,8 @@
// Map the value to the new alloca pointer if this was the old alloca
// pointer.
- bool ThisOperand = InVal == OldPtr;
+ Use *InUse = &PN.getOperandUse(PN.getOperandNumForIncomingValue(Idx));
+ bool ThisOperand = InUse == OldUse;
if (ThisOperand)
InVal = NewPtr;
@@ -2484,31 +2479,28 @@
Load->setMetadata(LLVMContext::MD_tbaa, TBAATag);
NewPN->addIncoming(Load, Pred);
- if (ThisOperand)
- continue;
- Instruction *OtherPtr = dyn_cast<Instruction>(InVal);
- if (!OtherPtr)
+ Instruction *Ptr = dyn_cast<Instruction>(InVal);
+ if (!Ptr)
// No uses to rewrite.
continue;
// Try to lookup and rewrite any partition uses corresponding to this phi
// input.
AllocaPartitioning::iterator PI
- = P.findPartitionForPHIOrSelectOperand(PN, OtherPtr);
- if (PI != P.end()) {
- // If the other pointer is within the partitioning, replace the PHI in
- // its uses with the load we just speculated, or add another load for
- // it to rewrite if we've already replaced the PHI.
- AllocaPartitioning::use_iterator UI
- = P.findPartitionUseForPHIOrSelectOperand(PN, OtherPtr);
- if (isa<PHINode>(*UI->User))
- UI->User = Load;
- else {
- AllocaPartitioning::PartitionUse OtherUse = *UI;
- OtherUse.User = Load;
- P.use_push_back(PI, OtherUse);
- }
- }
+ = P.findPartitionForPHIOrSelectOperand(InUse);
+ if (PI == P.end())
+ continue;
+
+ // Replace the Use in the PartitionUse for this operand with the Use
+ // inside the load. This will already have been re-written for the
+ // partition use currently being processed.
+ // FIXME: This is really gross. We should do PHI and select speculation
+ // as a pre-processing pass first, and then use the existing
+ // load-rewriting logic.
+ AllocaPartitioning::use_iterator UI
+ = P.findPartitionUseForPHIOrSelectOperand(InUse);
+ assert(isa<PHINode>(*UI->U->getUser()));
+ UI->U = &Load->getOperandUse(Load->getPointerOperandIndex());
}
DEBUG(dbgs() << " speculated to: " << *NewPN << "\n");
return NewPtr == &NewAI;
@@ -2575,15 +2567,15 @@
return false;
}
- Value *OtherPtr = IsTrueVal ? SI.getFalseValue() : SI.getTrueValue();
+ Use *OtherOp = &SI.getOperandUse(IsTrueVal ? 2 : 1);
AllocaPartitioning::iterator PI
- = P.findPartitionForPHIOrSelectOperand(SI, OtherPtr);
+ = P.findPartitionForPHIOrSelectOperand(OtherOp);
AllocaPartitioning::PartitionUse OtherUse;
if (PI != P.end()) {
// If the other pointer is within the partitioning, remove the select
// from its uses. We'll add in the new loads below.
AllocaPartitioning::use_iterator UI
- = P.findPartitionUseForPHIOrSelectOperand(SI, OtherPtr);
+ = P.findPartitionUseForPHIOrSelectOperand(OtherOp);
OtherUse = *UI;
P.use_erase(PI, UI);
}
@@ -2600,12 +2592,6 @@
LoadInst *FL =
IRB.CreateLoad(FV, getName("." + LI->getName() + ".false"));
NumLoadsSpeculated += 2;
- if (PI != P.end()) {
- LoadInst *OtherLoad = IsTrueVal ? FL : TL;
- assert(OtherUse.Ptr == OtherLoad->getOperand(0));
- OtherUse.User = OtherLoad;
- P.use_push_back(PI, OtherUse);
- }
// Transfer alignment and TBAA info if present.
TL->setAlignment(LI->getAlignment());
@@ -2615,10 +2601,18 @@
FL->setMetadata(LLVMContext::MD_tbaa, Tag);
}
- Value *V = IRB.CreateSelect(SI.getCondition(), TL, FL);
- V->takeName(LI);
- DEBUG(dbgs() << " speculated to: " << *V << "\n");
- LI->replaceAllUsesWith(V);
+ SelectInst *NewSI
+ = cast<SelectInst>(IRB.CreateSelect(SI.getCondition(), TL, FL));
+ NewSI->takeName(LI);
+ if (PI != P.end()) {
+ LoadInst *OtherLoad = IsTrueVal ? FL : TL;
+ Use *OtherLoadUse = &OtherLoad->getOperandUse(0);
+ assert(OtherUse.U->get() == OtherLoadUse->get());
+ OtherUse.U = OtherLoadUse;
+ P.use_push_back(PI, OtherUse);
+ }
+ DEBUG(dbgs() << " speculated to: " << *NewSI << "\n");
+ LI->replaceAllUsesWith(NewSI);
Pass.DeadInsts.push_back(LI);
}
Modified: llvm/trunk/test/Transforms/SROA/phi-and-select.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SROA/phi-and-select.ll?rev=164925&r1=164924&r2=164925&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/SROA/phi-and-select.ll (original)
+++ llvm/trunk/test/Transforms/SROA/phi-and-select.ll Sun Sep 30 20:49:22 2012
@@ -59,15 +59,24 @@
%a = alloca [2 x i32]
; CHECK-NOT: alloca
+ ; Note that we build redundant GEPs here to ensure that having different GEPs
+ ; into the same alloca partation continues to work with PHI speculation. This
+ ; was the underlying cause of PR13926.
%a0 = getelementptr [2 x i32]* %a, i64 0, i32 0
+ %a0b = getelementptr [2 x i32]* %a, i64 0, i32 0
%a1 = getelementptr [2 x i32]* %a, i64 0, i32 1
+ %a1b = getelementptr [2 x i32]* %a, i64 0, i32 1
store i32 0, i32* %a0
store i32 1, i32* %a1
; CHECK-NOT: store
switch i32 %x, label %bb0 [ i32 1, label %bb1
i32 2, label %bb2
- i32 3, label %bb3 ]
+ i32 3, label %bb3
+ i32 4, label %bb4
+ i32 5, label %bb5
+ i32 6, label %bb6
+ i32 7, label %bb7 ]
bb0:
br label %exit
@@ -77,10 +86,19 @@
br label %exit
bb3:
br label %exit
+bb4:
+ br label %exit
+bb5:
+ br label %exit
+bb6:
+ br label %exit
+bb7:
+ br label %exit
exit:
- %phi = phi i32* [ %a1, %bb0 ], [ %a0, %bb1 ], [ %a0, %bb2 ], [ %a1, %bb3 ]
-; CHECK: phi i32 [ 1, %{{.*}} ], [ 0, %{{.*}} ], [ 0, %{{.*}} ], [ 1, %{{.*}} ]
+ %phi = phi i32* [ %a1, %bb0 ], [ %a0, %bb1 ], [ %a0, %bb2 ], [ %a1, %bb3 ],
+ [ %a1b, %bb4 ], [ %a0b, %bb5 ], [ %a0b, %bb6 ], [ %a1b, %bb7 ]
+; CHECK: phi i32 [ 1, %{{.*}} ], [ 0, %{{.*}} ], [ 0, %{{.*}} ], [ 1, %{{.*}} ], [ 1, %{{.*}} ], [ 0, %{{.*}} ], [ 0, %{{.*}} ], [ 1, %{{.*}} ]
%result = load i32* %phi
ret i32 %result
More information about the llvm-commits
mailing list