[llvm-branch-commits] [llvm-branch] r292307 - Merging r291968 and r291979:
Hans Wennborg via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Tue Jan 17 16:12:26 PST 2017
Author: hans
Date: Tue Jan 17 18:12:25 2017
New Revision: 292307
URL: http://llvm.org/viewvc/llvm-project?rev=292307&view=rev
Log:
Merging r291968 and r291979:
------------------------------------------------------------------------
r291968 | dannyb | 2017-01-13 14:40:01 -0800 (Fri, 13 Jan 2017) | 23 lines
NewGVN: Move leaders around properly to ensure we have a canonical dominating leader. Fixes PR 31613.
Summary:
This is a testcase where phi node cycling happens, and because we do
not order the leaders by domination or anything similar, the leader
keeps changing.
Using std::set for the members is too expensive, and we actually don't
need them sorted all the time, only at leader changes.
We could keep both a set and a vector, and keep them mostly sorted and
resort as necessary, or use a set and a fibheap, but all of this seems
premature.
After running some statistics, we are able to avoid the vast majority
of sorting by keeping a "next leader" field. Most congruence classes only have
leader changes once or twice during GVN.
Reviewers: davide
Subscribers: llvm-commits
Differential Revision: https://reviews.llvm.org/D28594
------------------------------------------------------------------------
------------------------------------------------------------------------
r291979 | dannyb | 2017-01-13 15:54:10 -0800 (Fri, 13 Jan 2017) | 1 line
NewGVN: Fix PR31613 test regex naming
------------------------------------------------------------------------
Added:
llvm/branches/release_40/test/Transforms/NewGVN/pr31613.ll
- copied, changed from r291968, llvm/trunk/test/Transforms/NewGVN/pr31613.ll
Modified:
llvm/branches/release_40/ (props changed)
llvm/branches/release_40/lib/Transforms/Scalar/NewGVN.cpp
Propchange: llvm/branches/release_40/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Tue Jan 17 18:12:25 2017
@@ -1,3 +1,3 @@
/llvm/branches/Apple/Pertwee:110850,110961
/llvm/branches/type-system-rewrite:133420-134817
-/llvm/trunk:155241,291863,291875,292242,292255
+/llvm/trunk:155241,291863,291875,291968,291979,292242,292255
Modified: llvm/branches/release_40/lib/Transforms/Scalar/NewGVN.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/branches/release_40/lib/Transforms/Scalar/NewGVN.cpp?rev=292307&r1=292306&r2=292307&view=diff
==============================================================================
--- llvm/branches/release_40/lib/Transforms/Scalar/NewGVN.cpp (original)
+++ llvm/branches/release_40/lib/Transforms/Scalar/NewGVN.cpp Tue Jan 17 18:12:25 2017
@@ -81,6 +81,10 @@ STATISTIC(NumGVNOpsSimplified, "Number o
STATISTIC(NumGVNPhisAllSame, "Number of PHIs whos arguments are all the same");
STATISTIC(NumGVNMaxIterations,
"Maximum Number of iterations it took to converge GVN");
+STATISTIC(NumGVNLeaderChanges, "Number of leader changes");
+STATISTIC(NumGVNSortedLeaderChanges, "Number of sorted leader changes");
+STATISTIC(NumGVNAvoidedSortedLeaderChanges,
+ "Number of avoided sorted leader changes");
//===----------------------------------------------------------------------===//
// GVN Pass
@@ -139,6 +143,10 @@ struct CongruenceClass {
// This is used so we can detect store equivalence changes properly.
int StoreCount = 0;
+ // The most dominating leader after our current leader, because the member set
+ // is not sorted and is expensive to keep sorted all the time.
+ std::pair<Value *, unsigned int> NextLeader = {nullptr, ~0U};
+
explicit CongruenceClass(unsigned ID) : ID(ID) {}
CongruenceClass(unsigned ID, Value *Leader, const Expression *E)
: ID(ID), RepLeader(Leader), DefiningExpr(E) {}
@@ -320,8 +328,8 @@ private:
// Templated to allow them to work both on BB's and BB-edges.
template <class T>
Value *lookupOperandLeader(Value *, const User *, const T &) const;
- void performCongruenceFinding(Value *, const Expression *);
- void moveValueToNewCongruenceClass(Value *, CongruenceClass *,
+ void performCongruenceFinding(Instruction *, const Expression *);
+ void moveValueToNewCongruenceClass(Instruction *, CongruenceClass *,
CongruenceClass *);
// Reachability handling.
void updateReachableEdge(BasicBlock *, BasicBlock *);
@@ -1056,20 +1064,43 @@ void NewGVN::markLeaderChangeTouched(Con
// Move a value, currently in OldClass, to be part of NewClass
// Update OldClass for the move (including changing leaders, etc)
-void NewGVN::moveValueToNewCongruenceClass(Value *V, CongruenceClass *OldClass,
+void NewGVN::moveValueToNewCongruenceClass(Instruction *I,
+ CongruenceClass *OldClass,
CongruenceClass *NewClass) {
- DEBUG(dbgs() << "New congruence class for " << V << " is " << NewClass->ID
+ DEBUG(dbgs() << "New congruence class for " << I << " is " << NewClass->ID
<< "\n");
- OldClass->Members.erase(V);
- NewClass->Members.insert(V);
- if (isa<StoreInst>(V)) {
+
+ if (I == OldClass->NextLeader.first)
+ OldClass->NextLeader = {nullptr, ~0U};
+
+ // The new instruction and new class leader may either be siblings in the
+ // dominator tree, or the new class leader should dominate the new member
+ // instruction. We simply check that the member instruction does not properly
+ // dominate the new class leader.
+ assert(
+ !isa<Instruction>(NewClass->RepLeader) || !NewClass->RepLeader ||
+ I == NewClass->RepLeader ||
+ !DT->properlyDominates(
+ I->getParent(),
+ cast<Instruction>(NewClass->RepLeader)->getParent()) &&
+ "New class for instruction should not be dominated by instruction");
+
+ if (NewClass->RepLeader != I) {
+ auto DFSNum = InstrDFS.lookup(I);
+ if (DFSNum < NewClass->NextLeader.second)
+ NewClass->NextLeader = {I, DFSNum};
+ }
+
+ OldClass->Members.erase(I);
+ NewClass->Members.insert(I);
+ if (isa<StoreInst>(I)) {
--OldClass->StoreCount;
assert(OldClass->StoreCount >= 0);
++NewClass->StoreCount;
assert(NewClass->StoreCount > 0);
}
- ValueToClass[V] = NewClass;
+ ValueToClass[I] = NewClass;
// See if we destroyed the class or need to swap leaders.
if (OldClass->Members.empty() && OldClass != InitialClass) {
if (OldClass->DefiningExpr) {
@@ -1078,25 +1109,48 @@ void NewGVN::moveValueToNewCongruenceCla
<< " from table\n");
ExpressionToClass.erase(OldClass->DefiningExpr);
}
- } else if (OldClass->RepLeader == V) {
+ } else if (OldClass->RepLeader == I) {
// When the leader changes, the value numbering of
// everything may change due to symbolization changes, so we need to
// reprocess.
- OldClass->RepLeader = *(OldClass->Members.begin());
+ DEBUG(dbgs() << "Leader change!\n");
+ ++NumGVNLeaderChanges;
+ // We don't need to sort members if there is only 1, and we don't care about
+ // sorting the initial class because everything either gets out of it or is
+ // unreachable.
+ if (OldClass->Members.size() == 1 || OldClass == InitialClass) {
+ OldClass->RepLeader = *(OldClass->Members.begin());
+ } else if (OldClass->NextLeader.first) {
+ ++NumGVNAvoidedSortedLeaderChanges;
+ OldClass->RepLeader = OldClass->NextLeader.first;
+ OldClass->NextLeader = {nullptr, ~0U};
+ } else {
+ ++NumGVNSortedLeaderChanges;
+ // TODO: If this ends up to slow, we can maintain a dual structure for
+ // member testing/insertion, or keep things mostly sorted, and sort only
+ // here, or ....
+ std::pair<Value *, unsigned> MinDFS = {nullptr, ~0U};
+ for (const auto X : OldClass->Members) {
+ auto DFSNum = InstrDFS.lookup(X);
+ if (DFSNum < MinDFS.second)
+ MinDFS = {X, DFSNum};
+ }
+ OldClass->RepLeader = MinDFS.first;
+ }
markLeaderChangeTouched(OldClass);
}
}
// Perform congruence finding on a given value numbering expression.
-void NewGVN::performCongruenceFinding(Value *V, const Expression *E) {
- ValueToExpression[V] = E;
+void NewGVN::performCongruenceFinding(Instruction *I, const Expression *E) {
+ ValueToExpression[I] = E;
// This is guaranteed to return something, since it will at least find
// INITIAL.
- CongruenceClass *VClass = ValueToClass[V];
- assert(VClass && "Should have found a vclass");
+ CongruenceClass *IClass = ValueToClass[I];
+ assert(IClass && "Should have found a IClass");
// Dead classes should have been eliminated from the mapping.
- assert(!VClass->Dead && "Found a dead class");
+ assert(!IClass->Dead && "Found a dead class");
CongruenceClass *EClass;
if (const auto *VE = dyn_cast<VariableExpression>(E)) {
@@ -1118,13 +1172,13 @@ void NewGVN::performCongruenceFinding(Va
NewClass->RepLeader =
lookupOperandLeader(SI->getValueOperand(), SI, SI->getParent());
} else {
- NewClass->RepLeader = V;
+ NewClass->RepLeader = I;
}
assert(!isa<VariableExpression>(E) &&
"VariableExpression should have been handled already");
EClass = NewClass;
- DEBUG(dbgs() << "Created new congruence class for " << *V
+ DEBUG(dbgs() << "Created new congruence class for " << *I
<< " using expression " << *E << " at " << NewClass->ID
<< " and leader " << *(NewClass->RepLeader) << "\n");
DEBUG(dbgs() << "Hash value was " << E->getHashValue() << "\n");
@@ -1140,36 +1194,31 @@ void NewGVN::performCongruenceFinding(Va
assert(!EClass->Dead && "We accidentally looked up a dead class");
}
}
- bool ClassChanged = VClass != EClass;
- bool LeaderChanged = LeaderChanges.erase(V);
+ bool ClassChanged = IClass != EClass;
+ bool LeaderChanged = LeaderChanges.erase(I);
if (ClassChanged || LeaderChanged) {
DEBUG(dbgs() << "Found class " << EClass->ID << " for expression " << E
<< "\n");
if (ClassChanged)
-
- moveValueToNewCongruenceClass(V, VClass, EClass);
-
-
- markUsersTouched(V);
- if (auto *I = dyn_cast<Instruction>(V)) {
- if (MemoryAccess *MA = MSSA->getMemoryAccess(I)) {
- // If this is a MemoryDef, we need to update the equivalence table. If
- // we determined the expression is congruent to a different memory
- // state, use that different memory state. If we determined it didn't,
- // we update that as well. Right now, we only support store
- // expressions.
- if (!isa<MemoryUse>(MA) && isa<StoreExpression>(E) &&
- EClass->Members.size() != 1) {
- auto *DefAccess = cast<StoreExpression>(E)->getDefiningAccess();
- setMemoryAccessEquivTo(MA, DefAccess != MA ? DefAccess : nullptr);
- } else {
- setMemoryAccessEquivTo(MA, nullptr);
- }
- markMemoryUsersTouched(MA);
+ moveValueToNewCongruenceClass(I, IClass, EClass);
+ markUsersTouched(I);
+ if (MemoryAccess *MA = MSSA->getMemoryAccess(I)) {
+ // If this is a MemoryDef, we need to update the equivalence table. If
+ // we determined the expression is congruent to a different memory
+ // state, use that different memory state. If we determined it didn't,
+ // we update that as well. Right now, we only support store
+ // expressions.
+ if (!isa<MemoryUse>(MA) && isa<StoreExpression>(E) &&
+ EClass->Members.size() != 1) {
+ auto *DefAccess = cast<StoreExpression>(E)->getDefiningAccess();
+ setMemoryAccessEquivTo(MA, DefAccess != MA ? DefAccess : nullptr);
+ } else {
+ setMemoryAccessEquivTo(MA, nullptr);
}
+ markMemoryUsersTouched(MA);
}
- } else if (StoreInst *SI = dyn_cast<StoreInst>(V)) {
+ } else if (auto *SI = dyn_cast<StoreInst>(I)) {
// There is, sadly, one complicating thing for stores. Stores do not
// produce values, only consume them. However, in order to make loads and
// stores value number the same, we ignore the value operand of the store.
Copied: llvm/branches/release_40/test/Transforms/NewGVN/pr31613.ll (from r291968, llvm/trunk/test/Transforms/NewGVN/pr31613.ll)
URL: http://llvm.org/viewvc/llvm-project/llvm/branches/release_40/test/Transforms/NewGVN/pr31613.ll?p2=llvm/branches/release_40/test/Transforms/NewGVN/pr31613.ll&p1=llvm/trunk/test/Transforms/NewGVN/pr31613.ll&r1=291968&r2=292307&rev=292307&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/NewGVN/pr31613.ll (original)
+++ llvm/branches/release_40/test/Transforms/NewGVN/pr31613.ll Tue Jan 17 18:12:25 2017
@@ -85,7 +85,7 @@ define void @e() {
; CHECK: l:
; CHECK-NEXT: br label [[R:%.*]]
; CHECK: q:
-; CHECK-NEXT: [[M:%.*]] = load [[STRUCT_A*:%.*]], [[STRUCT_A**:%.*]] null
+; CHECK-NEXT: [[M:%.*]] = load %struct.a*, %struct.a** null
; CHECK-NEXT: br label [[R]]
; CHECK: r:
; CHECK-NEXT: switch i32 undef, label [[N:%.*]] [
@@ -95,7 +95,7 @@ define void @e() {
; CHECK-NEXT: store i32 undef, i32* [[F]], !g !0
; CHECK-NEXT: br label [[H]]
; CHECK: n:
-; CHECK-NEXT: [[O:%.*]] = load [[STRUCT_A*]], [[STRUCT_A**]] null
+; CHECK-NEXT: [[O:%.*]] = load %struct.a*, %struct.a** null
; CHECK-NEXT: ret void
;
%f = alloca i32
More information about the llvm-branch-commits
mailing list