Skip to content

Commit

Permalink
[RS4GC] Revert optimization attempt due to memory corruption
Browse files Browse the repository at this point in the history 
This change reverts "246133 [RewriteStatepointsForGC] Reduce the number of new instructions for base pointers" and a follow on bugfix 12575.

As pointed out in pr25846, this code suffers from a memory corruption bug.  Since I'm (empirically) not going to get back to this any time soon, simply reverting the problematic change is the right answer.



git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@261565 91177308-0d34-0410-b5e6-96231b3b80d8
  • Loading branch information
@preames
preames committed Feb 22, 2016
1 parent 4a8755b commit a40db4f
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 76 deletions.
66 changes: 3 additions & 63 deletions lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

#include "llvm/Pass.h"
#include "llvm/Analysis/CFG.h"
#include "llvm/Analysis/InstructionSimplify.h"
#include "llvm/Analysis/TargetTransformInfo.h"
#include "llvm/ADT/SetOperations.h"
#include "llvm/ADT/Statistic.h"
Expand Down Expand Up @@ -1025,75 +1024,14 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &cache) {

}

// Now that we're done with the algorithm, see if we can optimize the
// results slightly by reducing the number of new instructions needed.
// Arguably, this should be integrated into the algorithm above, but
// doing as a post process step is easier to reason about for the moment.
DenseMap<Value *, Value *> ReverseMap;
SmallPtrSet<Instruction *, 16> NewInsts;
SmallSetVector<AssertingVH<Instruction>, 16> Worklist;
// Note: We need to visit the states in a deterministic order. We uses the
// Keys we sorted above for this purpose. Note that we are papering over a
// bigger problem with the algorithm above - it's visit order is not
// deterministic. A larger change is needed to fix this.
for (auto Pair : States) {
auto *BDV = Pair.first;
auto State = Pair.second;
Value *Base = State.getBase();
assert(BDV && Base);
assert(!isKnownBaseResult(BDV) && "why did it get added?");
assert(isKnownBaseResult(Base) &&
"must be something we 'know' is a base pointer");
if (!State.isConflict())
continue;

ReverseMap[Base] = BDV;
if (auto *BaseI = dyn_cast<Instruction>(Base)) {
NewInsts.insert(BaseI);
Worklist.insert(BaseI);
}
}
auto ReplaceBaseInstWith = [&](Value *BDV, Instruction *BaseI,
Value *Replacement) {
// Add users which are new instructions (excluding self references)
for (User *U : BaseI->users())
if (auto *UI = dyn_cast<Instruction>(U))
if (NewInsts.count(UI) && UI != BaseI)
Worklist.insert(UI);
// Then do the actual replacement
NewInsts.erase(BaseI);
ReverseMap.erase(BaseI);
BaseI->replaceAllUsesWith(Replacement);
assert(States.count(BDV));
assert(States[BDV].isConflict() && States[BDV].getBase() == BaseI);
States[BDV] = BDVState(BDVState::Conflict, Replacement);
BaseI->eraseFromParent();
};
const DataLayout &DL = cast<Instruction>(def)->getModule()->getDataLayout();
while (!Worklist.empty()) {
Instruction *BaseI = Worklist.pop_back_val();
assert(NewInsts.count(BaseI));
Value *Bdv = ReverseMap[BaseI];
if (auto *BdvI = dyn_cast<Instruction>(Bdv))
if (BaseI->isIdenticalTo(BdvI)) {
DEBUG(dbgs() << "Identical Base: " << *BaseI << "\n");
ReplaceBaseInstWith(Bdv, BaseI, Bdv);
continue;
}
if (Value *V = SimplifyInstruction(BaseI, DL)) {
DEBUG(dbgs() << "Base " << *BaseI << " simplified to " << *V << "\n");
ReplaceBaseInstWith(Bdv, BaseI, V);
continue;
}
}

// Cache all of our results so we can cheaply reuse them
// NOTE: This is actually two caches: one of the base defining value
// relation and one of the base pointer relation! FIXME
for (auto Pair : States) {
auto *BDV = Pair.first;
Value *base = Pair.second.getBase();
assert(BDV && base);
assert(!isKnownBaseResult(BDV) && "why did it get added?");

std::string fromstr = cache.count(BDV) ? cache[BDV]->getName() : "none";
DEBUG(dbgs() << "Updating base value cache"
Expand All @@ -1102,6 +1040,8 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &cache) {
<< " to: " << base->getName() << "\n");

if (cache.count(BDV)) {
assert(isKnownBaseResult(base) &&
"must be something we 'know' is a base pointer");
// Once we transition from the BDV relation being store in the cache to
// the base relation being stored, it must be stable
assert((!isKnownBaseResult(cache[BDV]) || cache[BDV] == base) &&
Expand Down
2 changes: 1 addition & 1 deletion test/Transforms/RewriteStatepointsForGC/base-pointers-4.ll
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
; RUN: opt < %s -rewrite-statepoints-for-gc -spp-print-base-pointers -S 2>&1 | FileCheck %s

; CHECK: derived %obj_to_consume base %obj_to_consume
; CHECK: derived %obj_to_consume base %obj_to_consume.base

declare void @foo()

Expand Down
9 changes: 6 additions & 3 deletions test/Transforms/RewriteStatepointsForGC/base-pointers.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ entry:
; we'd have commoned these, but that's a missed optimization, not correctness.
; CHECK-DAG: [ [[DISCARD:%.*.base.relocated.casted]], %loop ]
; CHECK-NOT: extra.base
; CHECK: next.base = select
; CHECK: next = select
; CHECK: extra2.base = select
; CHECK: extra2 = select
Expand All @@ -107,7 +108,8 @@ taken: ; preds = %entry

merge: ; preds = %taken, %entry
; CHECK-LABEL: merge:
; CHECK-NEXT: %bdv = phi
; CHECK-NEXT: phi
; CHECK-NEXT: phi
; CHECK-NEXT: gc.statepoint
%bdv = phi i64 addrspace(1)* [ %obj, %entry ], [ %obj2, %taken ]
call void @foo() [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
Expand All @@ -124,7 +126,7 @@ taken: ; preds = %entry

merge: ; preds = %taken, %entry
; CHECK-LABEL: merge:
; CHECK-NEXT: %bdv = phi
; CHECK-NEXT: phi
; CHECK-NEXT: gc.statepoint
%bdv = phi i64 addrspace(1)* [ %obj, %entry ], [ %obj, %taken ]
call void @foo() [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
Expand All @@ -138,7 +140,8 @@ entry:

merge: ; preds = %merge, %entry
; CHECK-LABEL: merge:
; CHECK-NEXT: %bdv = phi
; CHECK-NEXT: phi
; CHECK-NEXT: phi
; CHECK-NEXT: br i1
%bdv = phi i64 addrspace(1)* [ %obj, %entry ], [ %obj2, %merge ]
br i1 %cnd, label %merge, label %next
Expand Down
17 changes: 9 additions & 8 deletions test/Transforms/RewriteStatepointsForGC/base-vector.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,13 @@ untaken2: ; preds = %merge

merge2: ; preds = %untaken2, %taken2
; CHECK-LABEL: merge2:
; CHECK-NEXT: %obj = phi i64 addrspace(1)*
; CHECK-NEXT: statepoint
; CHECK: %obj.base = phi i64 addrspace(1)*
; CHECK: %obj = phi i64 addrspace(1)*
; CHECK: statepoint
; CHECK: gc.relocate
; CHECK-DAG: ; (%obj.base, %obj)
; CHECK: gc.relocate
; CHECK-DAG: ; (%obj, %obj)
; CHECK-DAG: ; (%obj.base, %obj.base)
%obj = phi i64 addrspace(1)* [ %obj0, %taken2 ], [ %obj1, %untaken2 ]
call void @do_safepoint() [ "deopt"() ]
ret i64 addrspace(1)* %obj
Expand All @@ -60,7 +63,7 @@ define i64 addrspace(1)* @test3(i64 addrspace(1)* %ptr) gc "statepoint-example"
; CHECK: extractelement
; CHECK: statepoint
; CHECK: gc.relocate
; CHECK-DAG: (%obj, %obj)
; CHECK-DAG: (%obj.base, %obj)
entry:
%vec = insertelement <2 x i64 addrspace(1)*> undef, i64 addrspace(1)* %ptr, i32 0
%obj = extractelement <2 x i64 addrspace(1)*> %vec, i32 0
Expand All @@ -72,9 +75,7 @@ define i64 addrspace(1)* @test4(i64 addrspace(1)* %ptr) gc "statepoint-example"
; CHECK-LABEL: test4
; CHECK: statepoint
; CHECK: gc.relocate
; CHECK-DAG: ; (%ptr, %obj)
; CHECK: gc.relocate
; CHECK-DAG: ; (%ptr, %ptr)
; CHECK-DAG: ; (%obj.base, %obj)
; When we can optimize an extractelement from a known
; index and avoid introducing new base pointer instructions
entry:
Expand All @@ -91,7 +92,7 @@ declare void @use(i64 addrspace(1)*) "gc-leaf-function"
define void @test5(i1 %cnd, i64 addrspace(1)* %obj) gc "statepoint-example" {
; CHECK-LABEL: @test5
; CHECK: gc.relocate
; CHECK-DAG: (%obj, %bdv)
; CHECK-DAG: (%bdv.base, %bdv)
; When we fundementally have to duplicate
entry:
%gep = getelementptr i64, i64 addrspace(1)* %obj, i64 1
Expand Down
6 changes: 6 additions & 0 deletions test/Transforms/RewriteStatepointsForGC/live-vector-nosplit.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,12 @@ exceptional_return: ; preds = %entry
define <2 x i64 addrspace(1)*> @test5(i64 addrspace(1)* %p) gc "statepoint-example" {
; CHECK-LABEL: test5
; CHECK: insertelement
; CHECK-NEXT: insertelement
; CHECK-NEXT: gc.statepoint
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: ret <2 x i64 addrspace(1)*> %vec.relocated.casted
entry:
%vec = insertelement <2 x i64 addrspace(1)*> undef, i64 addrspace(1)* %p, i32 0
Expand All @@ -100,9 +103,12 @@ untaken: ; preds = %entry
merge: ; preds = %untaken, %taken
; CHECK-LABEL: merge:
; CHECK-NEXT: = phi
; CHECK-NEXT: = phi
; CHECK-NEXT: gc.statepoint
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: ret <2 x i64 addrspace(1)*>
%obj = phi <2 x i64 addrspace(1)*> [ %obja, %taken ], [ %objb, %untaken ]
call void @do_safepoint() [ "deopt"() ]
Expand Down
20 changes: 19 additions & 1 deletion test/Transforms/RewriteStatepointsForGC/live-vector.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,25 @@ exceptional_return: ; preds = %entry
define <2 x i64 addrspace(1)*> @test5(i64 addrspace(1)* %p) gc "statepoint-example" {
; CHECK-LABEL: test5
; CHECK: insertelement
; CHECK-NEXT: insertelement
; CHECK-NEXT: extractelement
; CHECK-NEXT: extractelement
; CHECK-NEXT: extractelement
; CHECK-NEXT: extractelement
; CHECK-NEXT: gc.statepoint
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: insertelement
; CHECK-NEXT: insertelement
; CHECK-NEXT: ret <2 x i64 addrspace(1)*> %7
; CHECK-NEXT: insertelement
; CHECK-NEXT: insertelement
; CHECK-NEXT: ret <2 x i64 addrspace(1)*>
; A base vector from a load
entry:
%vec = insertelement <2 x i64 addrspace(1)*> undef, i64 addrspace(1)* %p, i32 0
Expand All @@ -131,13 +140,22 @@ untaken: ; preds = %entry
merge: ; preds = %untaken, %taken
; CHECK-LABEL: merge:
; CHECK-NEXT: = phi
; CHECK-NEXT: = phi
; CHECK-NEXT: extractelement
; CHECK-NEXT: extractelement
; CHECK-NEXT: extractelement
; CHECK-NEXT: extractelement
; CHECK-NEXT: gc.statepoint
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: bitcast
; CHECK-NEXT: insertelement
; CHECK-NEXT: insertelement
; CHECK-NEXT: insertelement
; CHECK-NEXT: insertelement
; CHECK-NEXT: ret <2 x i64 addrspace(1)*>
Expand Down

0 comments on commit a40db4f

Please sign in to comment.