<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Dec 25, 2016 at 2:23 PM, Daniel Berlin via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: dannyb<br>
Date: Sun Dec 25 16:23:49 2016<br>
New Revision: 290525<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=290525&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=290525&view=rev</a><br>
Log:<br>
Value number stores and memory states so we can detect when memory states are equivalent (IE store of same value to memory).<br>
<br>
Reviewers: davide<br>
<br>
Subscribers: llvm-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D28084" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D28084</a><br>
<br>
Added:<br>
llvm/trunk/test/Transforms/<wbr>NewGVN/storeoverstore.ll<br>
Modified:<br>
llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=290525&r1=290524&r2=290525&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/Scalar/NewGVN.cpp?<wbr>rev=290525&r1=290524&r2=<wbr>290525&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/<wbr>Scalar/NewGVN.cpp Sun Dec 25 16:23:49 2016<br>
@@ -27,6 +27,7 @@<br>
#include "llvm/ADT/Hashing.h"<br>
#include "llvm/ADT/MapVector.h"<br>
#include "llvm/ADT/PostOrderIterator.h"<br>
+#include "llvm/ADT/STLExtras.h"<br>
#include "llvm/ADT/SmallPtrSet.h"<br>
#include "llvm/ADT/SmallSet.h"<br>
#include "llvm/ADT/SparseBitVector.h"<br>
@@ -184,6 +185,13 @@ class NewGVN : public FunctionPass {<br>
DenseMap<Value *, CongruenceClass *> ValueToClass;<br>
DenseMap<Value *, const Expression *> ValueToExpression;<br>
<br>
+ // A table storing which memorydefs/phis represent a memory state provably<br>
+ // equivalent to another memory state.<br>
+ // We could use the congruence class machinery, but the MemoryAccess's are<br>
+ // abstract memory states, so they can only ever be equivalent to each other,<br>
+ // and not to constants, etc.<br>
+ DenseMap<MemoryAccess *, MemoryAccess *> MemoryAccessEquiv;<br>
+<br>
// Expression to class mapping.<br>
typedef DenseMap<const Expression *, CongruenceClass *> ExpressionClassMap;<br>
ExpressionClassMap ExpressionToClass;<br>
@@ -219,7 +227,7 @@ class NewGVN : public FunctionPass {<br>
// DFS info.<br>
DenseMap<const BasicBlock *, std::pair<int, int>> DFSDomMap;<br>
DenseMap<const Value *, unsigned> InstrDFS;<br>
- std::vector<Instruction *> DFSToInstr;<br>
+ std::vector<Value *> DFSToInstr;<br>
<br>
// Deletion info.<br>
SmallPtrSet<Instruction *, 8> InstructionsToErase;<br>
@@ -284,6 +292,10 @@ private:<br>
}<br>
void initializeCongruenceClasses(<wbr>Function &F);<br>
<br>
+ // Value number an Instruction or MemoryPhi.<br>
+ void valueNumberMemoryPhi(MemoryPhi *);<br>
+ void valueNumberInstruction(<wbr>Instruction *);<br>
+<br>
// Symbolic evaluation.<br>
const Expression *checkSimplificationResults(<wbr>Expression *, Instruction *,<br>
Value *);<br>
@@ -296,6 +308,7 @@ private:<br>
const BasicBlock *);<br>
const Expression *performSymbolicPHIEvaluation(<wbr>Instruction *,<br>
const BasicBlock *);<br>
+ bool setMemoryAccessEquivTo(<wbr>MemoryAccess *From, MemoryAccess *To);<br>
const Expression *<wbr>performSymbolicAggrValueEvalua<wbr>tion(Instruction *,<br>
const BasicBlock *);<br>
<br>
@@ -310,6 +323,7 @@ private:<br>
void processOutgoingEdges(<wbr>TerminatorInst *, BasicBlock *);<br>
bool isOnlyReachableViaThisEdge(<wbr>const BasicBlockEdge &) const;<br>
Value *findConditionEquivalence(<wbr>Value *, BasicBlock *) const;<br>
+ MemoryAccess *lookupMemoryAccessEquiv(<wbr>MemoryAccess *) const;<br>
<br>
// Elimination.<br>
struct ValueDFS;<br>
@@ -663,6 +677,11 @@ Value *NewGVN::lookupOperandLeader(<wbr>Value<br>
return V;<br>
}<br>
<br>
+MemoryAccess *NewGVN::<wbr>lookupMemoryAccessEquiv(<wbr>MemoryAccess *MA) const {<br>
+ MemoryAccess *Result = MemoryAccessEquiv.lookup(MA);<br>
+ return Result ? Result : MA;<br>
+}<br>
+<br>
LoadExpression *NewGVN::createLoadExpression(<wbr>Type *LoadType, Value *PointerOp,<br>
LoadInst *LI, MemoryAccess *DA,<br>
const BasicBlock *B) {<br>
@@ -704,8 +723,22 @@ const StoreExpression *NewGVN::createSto<br>
const Expression *NewGVN::<wbr>performSymbolicStoreEvaluation<wbr>(Instruction *I,<br>
const BasicBlock *B) {<br>
StoreInst *SI = cast<StoreInst>(I);<br>
- const Expression *E = createStoreExpression(SI, MSSA->getMemoryAccess(SI), B);<br>
- return E;<br>
+ // If this store's memorydef stores the same value as the last store, the<br>
+ // memory accesses are equivalent.<br>
+ // Get the expression, if any, for the RHS of the MemoryDef.<br>
+ MemoryAccess *StoreAccess = MSSA->getMemoryAccess(SI);<br>
+ MemoryAccess *StoreRHS = lookupMemoryAccessEquiv(<br>
+ cast<MemoryDef>(StoreAccess)-><wbr>getDefiningAccess());<br>
+ const Expression *OldStore = createStoreExpression(SI, StoreRHS, B);<br>
+ // See if this store expression already has a value, and it's the same as our<br>
+ // current store.<br>
+ CongruenceClass *CC = ExpressionToClass.lookup(<wbr>OldStore);<br>
+ if (CC &&<br>
+ CC->RepLeader == lookupOperandLeader(SI-><wbr>getValueOperand(), SI, B)) {<br>
+ setMemoryAccessEquivTo(<wbr>StoreAccess, StoreRHS);<br>
+ return OldStore;<br>
+ }<br>
+ return createStoreExpression(SI, StoreAccess, B);<br>
}<br>
<br>
const Expression *NewGVN::<wbr>performSymbolicLoadEvaluation(<wbr>Instruction *I,<br>
@@ -734,8 +767,9 @@ const Expression *NewGVN::performSymboli<br>
}<br>
}<br>
<br>
- const Expression *E = createLoadExpression(<br>
- LI->getType(), LI->getPointerOperand(), LI, DefiningAccess, B);<br>
+ const Expression *E =<br>
+ createLoadExpression(LI-><wbr>getType(), LI->getPointerOperand(), LI,<br>
+ lookupMemoryAccessEquiv(<wbr>DefiningAccess), B);<br>
return E;<br>
}<br>
<br>
@@ -752,6 +786,29 @@ const Expression *NewGVN::performSymboli<br>
return nullptr;<br>
}<br>
<br>
+// Update the memory access equivalence table to say that From is equal to To,<br>
+// and return true if this is different from what already existed in the table.<br>
+bool NewGVN::<wbr>setMemoryAccessEquivTo(<wbr>MemoryAccess *From, MemoryAccess *To) {<br>
+ auto LookupResult = MemoryAccessEquiv.insert({<wbr>From, nullptr});<br>
+ bool Changed = false;<br>
+ // If it's already in the table, see if the value changed.<br>
+ if (LookupResult.second) {<br>
+ if (To && LookupResult.first->second != To) {<br>
+ // It wasn't equivalent before, and now it is.<br>
+ LookupResult.first->second = To;<br>
+ Changed = true;<br>
+ } else if (!To) {<br>
+ // It used to be equivalent to something, and now it's not.<br>
+ MemoryAccessEquiv.erase(<wbr>LookupResult.first);<br>
+ Changed = true;<br>
+ }<br>
+ } else if (To) {<br>
+ // It wasn't in the table, but is equivalent to something.<br>
+ LookupResult.first->second = To;<br>
+ Changed = true;<br>
+ }<br>
+ return Changed;<br>
+}<br>
// Evaluate PHI nodes symbolically, and create an expression result.<br>
const Expression *NewGVN::<wbr>performSymbolicPHIEvaluation(<wbr>Instruction *I,<br>
const BasicBlock *B) {<br>
@@ -1214,11 +1271,17 @@ void NewGVN::cleanupTables() {<br>
BlockInstRange.clear();<br>
TouchedInstructions.clear();<br>
DominatedInstRange.clear();<br>
+ MemoryAccessEquiv.clear();<br>
}<br>
<br>
std::pair<unsigned, unsigned> NewGVN::assignDFSNumbers(<wbr>BasicBlock *B,<br>
unsigned Start) {<br>
unsigned End = Start;<br>
+ if (MemoryAccess *MemPhi = MSSA->getMemoryAccess(B)) {<br>
+ InstrDFS[MemPhi] = End++;<br>
+ DFSToInstr.emplace_back(<wbr>MemPhi);<br>
+ }<br>
+<br>
for (auto &I : *B) {<br>
InstrDFS[&I] = End++;<br>
DFSToInstr.emplace_back(&I);<br>
@@ -1241,6 +1304,61 @@ void NewGVN::updateProcessedCount(<wbr>Value<br>
}<br>
#endif<br>
}<br>
+// Evaluate MemoryPhi nodes symbolically, just like PHI nodes<br>
+void NewGVN::valueNumberMemoryPhi(<wbr>MemoryPhi *MP) {<br>
+ // If all the arguments are the same, the MemoryPhi has the same value as the<br>
+ // argument.<br>
+ // Filter out unreachable blocks from our operands.<br>
+ auto Filtered = make_filter_range(MP-><wbr>operands(), [&](const Use &U) {<br>
+ return ReachableBlocks.count(MP-><wbr>getIncomingBlock(U));<br>
+ });<br>
+<br>
+ assert(Filtered.begin() != Filtered.end() &&<br>
+ "We should not be processing a MemoryPhi in a completely "<br>
+ "unreachable block");<br>
+<br>
+ // Transform the remaining operands into operand leaders.<br>
+ // FIXME: mapped_iterator should have a range version.<br>
+ auto LookupFunc = [&](const Use &U) {<br>
+ return lookupMemoryAccessEquiv(cast<<wbr>MemoryAccess>(U));<br>
+ };<br>
+ auto MappedBegin = map_iterator(Filtered.begin(), LookupFunc);<br>
+ auto MappedEnd = map_iterator(Filtered.end(), LookupFunc);<br>
+<br>
+ // and now check if all the elements are equal.<br>
+ // Sadly, we can't use std::equals since these are random access iterators.<br></blockquote><div><br></div><div>Can you clarify this? std::equal accepts input iterators (<a href="http://en.cppreference.com/w/cpp/algorithm/equal">http://en.cppreference.com/w/cpp/algorithm/equal</a>). Looking at the code, it doesn't seem like std::equal would be a good fit anyway (the code doesn't seem to be comparing two ranges; but one element against an entire range). std::all_of seems like a good choice. Maybe just delete that part of the comment?</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ MemoryAccess *AllSameValue = *MappedBegin;<br>
+ ++MappedBegin;<br>
+ bool AllEqual = std::all_of(<br>
+ MappedBegin, MappedEnd,<br>
+ [&AllSameValue](const MemoryAccess *V) { return V == AllSameValue; });<br>
+<br>
+ if (AllEqual)<br>
+ DEBUG(dbgs() << "Memory Phi value numbered to " << *AllSameValue << "\n");<br>
+ else<br>
+ DEBUG(dbgs() << "Memory Phi value numbered to itself\n");<br>
+<br>
+ if (setMemoryAccessEquivTo(MP, AllEqual ? AllSameValue : nullptr))<br>
+ markMemoryUsersTouched(MP);<br>
+}<br>
+<br>
+// Value number a single instruction, symbolically evaluating, performing<br>
+// congruence finding, and updating mappings.<br>
+void NewGVN::<wbr>valueNumberInstruction(<wbr>Instruction *I) {<br>
+ DEBUG(dbgs() << "Processing instruction " << *I << "\n");<br>
+ if (I->use_empty() && !I->getType()->isVoidTy()) {<br>
+ DEBUG(dbgs() << "Skipping unused instruction\n");<br>
+ if (isInstructionTriviallyDead(I, TLI))<br>
+ markInstructionForDeletion(I);<br>
+ return;<br>
+ }<br>
+ if (!I->isTerminator()) {<br>
+ const Expression *Symbolized = performSymbolicEvaluation(I, I->getParent());<br>
+ performCongruenceFinding(I, Symbolized);<br>
+ } else {<br>
+ processOutgoingEdges(dyn_cast<<wbr>TerminatorInst>(I), I->getParent());<br>
+ }<br>
+}<br>
<br>
// This is the main transformation entry point.<br>
bool NewGVN::runGVN(Function &F, DominatorTree *_DT, AssumptionCache *_AC,<br>
@@ -1304,8 +1422,15 @@ bool NewGVN::runGVN(Function &F, Dominat<br>
// Walk through all the instructions in all the blocks in RPO.<br>
for (int InstrNum = TouchedInstructions.find_<wbr>first(); InstrNum != -1;<br>
InstrNum = TouchedInstructions.find_next(<wbr>InstrNum)) {<br>
- Instruction *I = DFSToInstr[InstrNum];<br>
- BasicBlock *CurrBlock = I->getParent();<br>
+ Value *V = DFSToInstr[InstrNum];<br>
+ BasicBlock *CurrBlock = nullptr;<br>
+<br>
+ if (Instruction *I = dyn_cast<Instruction>(V))<br>
+ CurrBlock = I->getParent();<br>
+ else if (MemoryPhi *MP = dyn_cast<MemoryPhi>(V))<br>
+ CurrBlock = MP->getBlock();<br>
+ else<br>
+ llvm_unreachable("DFSToInstr gave us an unknown type of instruction");<br>
<br>
// If we hit a new block, do reachability processing.<br>
if (CurrBlock != LastBlock) {<br>
@@ -1323,22 +1448,16 @@ bool NewGVN::runGVN(Function &F, Dominat<br>
}<br>
updateProcessedCount(<wbr>CurrBlock);<br>
}<br>
- DEBUG(dbgs() << "Processing instruction " << *I << "\n");<br>
- if (I->use_empty() && !I->getType()->isVoidTy()) {<br>
- DEBUG(dbgs() << "Skipping unused instruction\n");<br>
- if (isInstructionTriviallyDead(I, TLI))<br>
- markInstructionForDeletion(I);<br>
- TouchedInstructions.reset(<wbr>InstrNum);<br>
- continue;<br>
- }<br>
- updateProcessedCount(I);<br>
<br>
- if (!I->isTerminator()) {<br>
- const Expression *Symbolized = performSymbolicEvaluation(I, CurrBlock);<br>
- performCongruenceFinding(I, Symbolized);<br>
+ if (MemoryPhi *MP = dyn_cast<MemoryPhi>(V)) {<br>
+ DEBUG(dbgs() << "Processing MemoryPhi " << *MP << "\n");<br>
+ valueNumberMemoryPhi(MP);<br>
+ } else if (Instruction *I = dyn_cast<Instruction>(V)) {<br>
+ valueNumberInstruction(I);<br>
} else {<br>
- processOutgoingEdges(dyn_cast<<wbr>TerminatorInst>(I), CurrBlock);<br>
+ llvm_unreachable("Should have been a MemoryPhi or Instruction");<br>
}<br>
+ updateProcessedCount(V);<br>
// Reset after processing (because we may mark ourselves as touched when<br>
// we propagate equalities).<br>
TouchedInstructions.reset(<wbr>InstrNum);<br>
<br>
Added: llvm/trunk/test/Transforms/<wbr>NewGVN/storeoverstore.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/NewGVN/storeoverstore.ll?rev=290525&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/NewGVN/<wbr>storeoverstore.ll?rev=290525&<wbr>view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/Transforms/<wbr>NewGVN/storeoverstore.ll (added)<br>
+++ llvm/trunk/test/Transforms/<wbr>NewGVN/storeoverstore.ll Sun Dec 25 16:23:49 2016<br>
@@ -0,0 +1,80 @@<br>
+; RUN: opt -newgvn -S < %s | FileCheck %s<br>
+; RUN: opt -passes=newgvn -S -o - %s | FileCheck %s<br>
+<br>
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:<wbr>32:64-S128"<br>
+<br>
+;; All the loads in this testcase are useless, but it requires understanding that repeated<br>
+;; stores of the same value do not change the memory state to eliminate them.<br>
+<br>
+define i32 @foo(i32*, i32) {<br>
+; CHECK-LABEL: @foo<br>
+ store i32 5, i32* %0, align 4<br>
+ %3 = icmp ne i32 %1, 0<br>
+ br i1 %3, label %4, label %7<br>
+<br>
+; <label>:4: ; preds = %2<br>
+; CHECK-NOT: load<br>
+ %5 = load i32, i32* %0, align 4<br>
+; CHECK-NOT: add<br>
+ %6 = add nsw i32 5, %5<br>
+ br label %7<br>
+<br>
+; <label>:7: ; preds = %4, %2<br>
+ %.0 = phi i32 [ %6, %4 ], [ 5, %2 ]<br>
+; CHECK: phi i32 [ 10, %4 ], [ 5, %2 ]<br>
+ store i32 5, i32* %0, align 4<br>
+; CHECK-NOT: icmp<br>
+ %8 = icmp ne i32 %1, 0<br>
+; CHECK: br i1 %3<br>
+ br i1 %8, label %9, label %12<br>
+<br>
+; <label>:9: ; preds = %7<br>
+; CHECK-NOT: load<br>
+ %10 = load i32, i32* %0, align 4<br>
+; CHECK: add nsw i32 %.0, 5<br>
+ %11 = add nsw i32 %.0, %10<br>
+ br label %12<br>
+<br>
+; <label>:12: ; preds = %9, %7<br>
+ %.1 = phi i32 [ %11, %9 ], [ %.0, %7 ]<br>
+ ret i32 %.1<br>
+}<br>
+<br>
+;; This is similar to the above, but it is a conditional store of the same value<br>
+;; which requires value numbering MemoryPhi properly to resolve.<br>
+define i32 @foo2(i32*, i32) {<br>
+; CHECK-LABEL: @foo2<br>
+ store i32 5, i32* %0, align 4<br>
+ %3 = icmp ne i32 %1, 0<br>
+ br i1 %3, label %4, label %7<br>
+<br>
+; <label>:4: ; preds = %2<br>
+; CHECK-NOT: load<br>
+ %5 = load i32, i32* %0, align 4<br>
+; CHECK-NOT: add<br>
+ %6 = add nsw i32 5, %5<br>
+ br label %8<br>
+<br>
+; <label>:7: ; preds = %2<br>
+ store i32 5, i32* %0, align 4<br>
+ br label %8<br>
+<br>
+; <label>:8: ; preds = %7, %4<br>
+; CHECK: phi i32 [ 10, %4 ], [ 5, %5 ]<br>
+ %.0 = phi i32 [ %6, %4 ], [ 5, %7 ]<br>
+; CHECK-NOT: icmp<br>
+ %9 = icmp ne i32 %1, 0<br>
+; CHECK: br i1 %3<br>
+ br i1 %9, label %10, label %13<br>
+<br>
+; <label>:10: ; preds = %8<br>
+; CHECK-NOT: load<br>
+ %11 = load i32, i32* %0, align 4<br>
+; CHECK: add nsw i32 %.0, 5<br>
+ %12 = add nsw i32 %.0, %11<br>
+ br label %13<br>
+<br>
+; <label>:13: ; preds = %10, %8<br>
+ %.1 = phi i32 [ %12, %10 ], [ %.0, %8 ]<br>
+ ret i32 %.1<br>
+}<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>