<div dir="ltr">Hi Dan,<div><br></div><div>We had quite a few regressions due to this on both arm and x86-64. The extreme case was MultiSource/Benchmarks/FreeBench/fourinarow/fourinarow, which regressed by 20% on x86-64 at -O3 -flto.</div>
<div><br></div><div>Is it possible to fix this, or revert it until the performance issue can be worked out?</div><div><br></div><div>Cheers,</div><div>Lang.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Mar 17, 2014 at 12:57 PM, Dan Gohman <span dir="ltr"><<a href="mailto:dan433584@gmail.com" target="_blank">dan433584@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: djg<br>
Date: Mon Mar 17 14:57:04 2014<br>
New Revision: 204076<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=204076&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=204076&view=rev</a><br>
Log:<br>
Use range metadata instead of introducing selects.<br>
<br>
When GlobalOpt has determined that a GlobalVariable only ever has two values,<br>
it would convert the GlobalVariable to a boolean, and introduce SelectInsts<br>
at every load, to choose between the two possible values. These SelectInsts<br>
introduce overhead and other unpleasantness.<br>
<br>
This patch makes GlobalOpt just add range metadata to loads from such<br>
GlobalVariables instead. This enables the same main optimization (as seen in<br>
test/Transforms/GlobalOpt/integer-bool.ll), without introducing selects.<br>
<br>
The main downside is that it doesn't get the memory savings of shrinking such<br>
GlobalVariables, but this is expected to be negligible.<br>
<br>
Added:<br>
llvm/trunk/test/Transforms/GlobalOpt/twostore-gv-range.ll<br>
Modified:<br>
llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp<br>
llvm/trunk/test/Transforms/GlobalOpt/integer-bool.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp?rev=204076&r1=204075&r2=204076&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp?rev=204076&r1=204075&r2=204076&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp Mon Mar 17 14:57:04 2014<br>
@@ -1588,18 +1588,16 @@ static bool OptimizeOnceStoredGlobal(Glo<br>
return false;<br>
}<br>
<br>
-/// TryToShrinkGlobalToBoolean - At this point, we have learned that the only<br>
+/// TryToAddRangeMetadata - At this point, we have learned that the only<br>
/// two values ever stored into GV are its initializer and OtherVal. See if we<br>
-/// can shrink the global into a boolean and select between the two values<br>
-/// whenever it is used. This exposes the values to other scalar optimizations.<br>
-static bool TryToShrinkGlobalToBoolean(GlobalVariable *GV, Constant *OtherVal) {<br>
+/// can annotate loads from it with range metadata describing this.<br>
+/// This exposes the values to other scalar optimizations.<br>
+static bool TryToAddRangeMetadata(GlobalVariable *GV, Constant *OtherVal) {<br>
Type *GVElType = GV->getType()->getElementType();<br>
<br>
- // If GVElType is already i1, it is already shrunk. If the type of the GV is<br>
- // an FP value, pointer or vector, don't do this optimization because a select<br>
- // between them is very expensive and unlikely to lead to later<br>
- // simplification. In these cases, we typically end up with "cond ? v1 : v2"<br>
- // where v1 and v2 both require constant pool loads, a big loss.<br>
+ // If GVElType is already i1, it already has a minimal range. If the type of<br>
+ // the GV is an FP value, pointer or vector, don't do this optimization<br>
+ // because range metadata is currently only supported on scalar integers.<br>
if (GVElType == Type::getInt1Ty(GV->getContext()) ||<br>
GVElType->isFloatingPointTy() ||<br>
GVElType->isPointerTy() || GVElType->isVectorTy())<br>
@@ -1611,81 +1609,53 @@ static bool TryToShrinkGlobalToBoolean(G<br>
if (!isa<LoadInst>(U) && !isa<StoreInst>(U))<br>
return false;<br>
<br>
- DEBUG(dbgs() << " *** SHRINKING TO BOOL: " << *GV);<br>
-<br>
- // Create the new global, initializing it to false.<br>
- GlobalVariable *NewGV = new GlobalVariable(Type::getInt1Ty(GV->getContext()),<br>
- false,<br>
- GlobalValue::InternalLinkage,<br>
- ConstantInt::getFalse(GV->getContext()),<br>
- GV->getName()+".b",<br>
- GV->getThreadLocalMode(),<br>
- GV->getType()->getAddressSpace());<br>
- GV->getParent()->getGlobalList().insert(GV, NewGV);<br>
-<br>
Constant *InitVal = GV->getInitializer();<br>
assert(InitVal->getType() != Type::getInt1Ty(GV->getContext()) &&<br>
- "No reason to shrink to bool!");<br>
+ "No reason to add range metadata!");<br>
+<br>
+ // The MD_range metadata only supports absolute integer constants.<br>
+ if (!isa<ConstantInt>(InitVal) || !isa<ConstantInt>(OtherVal))<br>
+ return false;<br>
<br>
- // If initialized to zero and storing one into the global, we can use a cast<br>
- // instead of a select to synthesize the desired value.<br>
- bool IsOneZero = false;<br>
- if (ConstantInt *CI = dyn_cast<ConstantInt>(OtherVal))<br>
- IsOneZero = InitVal->isNullValue() && CI->isOne();<br>
-<br>
- while (!GV->use_empty()) {<br>
- Instruction *UI = cast<Instruction>(GV->user_back());<br>
- if (StoreInst *SI = dyn_cast<StoreInst>(UI)) {<br>
- // Change the store into a boolean store.<br>
- bool StoringOther = SI->getOperand(0) == OtherVal;<br>
- // Only do this if we weren't storing a loaded value.<br>
- Value *StoreVal;<br>
- if (StoringOther || SI->getOperand(0) == InitVal) {<br>
- StoreVal = ConstantInt::get(Type::getInt1Ty(GV->getContext()),<br>
- StoringOther);<br>
+ DEBUG(dbgs() << " *** ADDING RANGE METADATA: " << *GV);<br>
+<br>
+ for (User *U : GV->users()) {<br>
+ Instruction *UI = cast<Instruction>(U);<br>
+ if (LoadInst *LI = dyn_cast<LoadInst>(UI)) {<br>
+ // If we already have a range, don't add a new one, so that GlobalOpt<br>
+ // terminates. In theory, we could merge the two ranges.<br>
+ if (LI->getMetadata(LLVMContext::MD_range))<br>
+ return false;<br>
+ // Add range metadata to the load. Range metadata can represent multiple<br>
+ // ranges, but they must be discontiguous, so we have two cases: the case<br>
+ // where the values are adjacent, in which case we add one range, and the<br>
+ // case where they're not, in which case we add two.<br>
+ APInt Min = cast<ConstantInt>(InitVal)->getValue();<br>
+ APInt Max = cast<ConstantInt>(OtherVal)->getValue();<br>
+ if (Max.slt(Min))<br>
+ std::swap(Min, Max);<br>
+ APInt Min1 = Min + 1;<br>
+ APInt Max1 = Max + 1;<br>
+ if (Min1 == Max) {<br>
+ Value *Vals[] = {<br>
+ ConstantInt::get(GV->getContext(), Min),<br>
+ ConstantInt::get(GV->getContext(), Max1),<br>
+ };<br>
+ MDNode *MD = MDNode::get(LI->getContext(), Vals);<br>
+ LI->setMetadata(LLVMContext::MD_range, MD);<br>
} else {<br>
- // Otherwise, we are storing a previously loaded copy. To do this,<br>
- // change the copy from copying the original value to just copying the<br>
- // bool.<br>
- Instruction *StoredVal = cast<Instruction>(SI->getOperand(0));<br>
-<br>
- // If we've already replaced the input, StoredVal will be a cast or<br>
- // select instruction. If not, it will be a load of the original<br>
- // global.<br>
- if (LoadInst *LI = dyn_cast<LoadInst>(StoredVal)) {<br>
- assert(LI->getOperand(0) == GV && "Not a copy!");<br>
- // Insert a new load, to preserve the saved value.<br>
- StoreVal = new LoadInst(NewGV, LI->getName()+".b", false, 0,<br>
- LI->getOrdering(), LI->getSynchScope(), LI);<br>
- } else {<br>
- assert((isa<CastInst>(StoredVal) || isa<SelectInst>(StoredVal)) &&<br>
- "This is not a form that we understand!");<br>
- StoreVal = StoredVal->getOperand(0);<br>
- assert(isa<LoadInst>(StoreVal) && "Not a load of NewGV!");<br>
- }<br>
+ Value *Vals[] = {<br>
+ ConstantInt::get(GV->getContext(), Min),<br>
+ ConstantInt::get(GV->getContext(), Min1),<br>
+ ConstantInt::get(GV->getContext(), Max),<br>
+ ConstantInt::get(GV->getContext(), Max1),<br>
+ };<br>
+ MDNode *MD = MDNode::get(LI->getContext(), Vals);<br>
+ LI->setMetadata(LLVMContext::MD_range, MD);<br>
}<br>
- new StoreInst(StoreVal, NewGV, false, 0,<br>
- SI->getOrdering(), SI->getSynchScope(), SI);<br>
- } else {<br>
- // Change the load into a load of bool then a select.<br>
- LoadInst *LI = cast<LoadInst>(UI);<br>
- LoadInst *NLI = new LoadInst(NewGV, LI->getName()+".b", false, 0,<br>
- LI->getOrdering(), LI->getSynchScope(), LI);<br>
- Value *NSI;<br>
- if (IsOneZero)<br>
- NSI = new ZExtInst(NLI, LI->getType(), "", LI);<br>
- else<br>
- NSI = SelectInst::Create(NLI, OtherVal, InitVal, "", LI);<br>
- NSI->takeName(LI);<br>
- LI->replaceAllUsesWith(NSI);<br>
}<br>
- UI->eraseFromParent();<br>
}<br>
<br>
- // Retain the name of the old global variable. People who are debugging their<br>
- // programs may expect these variables to be named the same.<br>
- NewGV->takeName(GV);<br>
- GV->eraseFromParent();<br>
return true;<br>
}<br>
<br>
@@ -1839,11 +1809,10 @@ bool GlobalOpt::ProcessInternalGlobal(Gl<br>
DL, TLI))<br>
return true;<br>
<br>
- // Otherwise, if the global was not a boolean, we can shrink it to be a<br>
- // boolean.<br>
+ // Otherwise, if the global was not a boolean, we can add range metadata.<br>
if (Constant *SOVConstant = dyn_cast<Constant>(GS.StoredOnceValue)) {<br>
if (GS.Ordering == NotAtomic) {<br>
- if (TryToShrinkGlobalToBoolean(GV, SOVConstant)) {<br>
+ if (TryToAddRangeMetadata(GV, SOVConstant)) {<br>
++NumShrunkToBool;<br>
return true;<br>
}<br>
<br>
Modified: llvm/trunk/test/Transforms/GlobalOpt/integer-bool.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GlobalOpt/integer-bool.ll?rev=204076&r1=204075&r2=204076&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GlobalOpt/integer-bool.ll?rev=204076&r1=204075&r2=204076&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/GlobalOpt/integer-bool.ll (original)<br>
+++ llvm/trunk/test/Transforms/GlobalOpt/integer-bool.ll Mon Mar 17 14:57:04 2014<br>
@@ -1,20 +1,21 @@<br>
; RUN: opt < %s -S -globalopt -instcombine | FileCheck %s<br>
-;; check that global opt turns integers that only hold 0 or 1 into bools.<br>
+;; check that global opt annotates loads from global variales that only hold 0 or 1<br>
+;; such that instcombine can optimize accordingly.<br>
<br>
@G = internal addrspace(1) global i32 0<br>
; CHECK: @G<br>
; CHECK: addrspace(1)<br>
-; CHECK: global i1 false<br>
+; CHECK: global i32 0<br>
<br>
define void @set1() {<br>
store i32 0, i32 addrspace(1)* @G<br>
-; CHECK: store i1 false<br>
+; CHECK: store i32 0<br>
ret void<br>
}<br>
<br>
define void @set2() {<br>
store i32 1, i32 addrspace(1)* @G<br>
-; CHECK: store i1 true<br>
+; CHECK: store i32 1<br>
ret void<br>
}<br>
<br>
<br>
Added: llvm/trunk/test/Transforms/GlobalOpt/twostore-gv-range.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GlobalOpt/twostore-gv-range.ll?rev=204076&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GlobalOpt/twostore-gv-range.ll?rev=204076&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/GlobalOpt/twostore-gv-range.ll (added)<br>
+++ llvm/trunk/test/Transforms/GlobalOpt/twostore-gv-range.ll Mon Mar 17 14:57:04 2014<br>
@@ -0,0 +1,52 @@<br>
+; RUN: opt < %s -S -globalopt | FileCheck %s<br>
+;; check that global opt annotates loads from global variales that have<br>
+;; constant values stored to them.<br>
+<br>
+@G = internal global i32 5<br>
+@H = internal global i32 7<br>
+@I = internal global i32 17<br>
+@J = internal global i32 29<br>
+@K = internal global i32 31<br>
+<br>
+define void @set() {<br>
+ store i32 6, i32* @G<br>
+ store i32 13, i32* @H<br>
+ store i32 16, i32* @I<br>
+ store i32 29, i32* @J<br>
+ store i32 -37, i32* @K<br>
+ ret void<br>
+}<br>
+<br>
+define i32 @getG() {<br>
+; CHECK: %t = load i32* @G, !range [[G:![0-9]+]]<br>
+ %t = load i32* @G<br>
+ ret i32 %t<br>
+}<br>
+define i32 @getH() {<br>
+; CHECK: %t = load i32* @H, !range [[H:![0-9]+]]<br>
+ %t = load i32* @H<br>
+ ret i32 %t<br>
+}<br>
+<br>
+define i32 @getI() {<br>
+; CHECK: %t = load i32* @I, !range [[I:![0-9]+]]<br>
+ %t = load i32* @I<br>
+ ret i32 %t<br>
+}<br>
+<br>
+define i32 @getJ() {<br>
+; CHECK: ret i32 29<br>
+ %t = load i32* @J<br>
+ ret i32 %t<br>
+}<br>
+<br>
+define i32 @getK() {<br>
+; CHECK: %t = load i32* @K, !range [[K:![0-9]+]]<br>
+ %t = load i32* @K<br>
+ ret i32 %t<br>
+}<br>
+<br>
+; CHECK: [[G]] = metadata !{i32 5, i32 7}<br>
+; CHECK: [[H]] = metadata !{i32 7, i32 8, i32 13, i32 14}<br>
+; CHECK: [[I]] = metadata !{i32 16, i32 18}<br>
+; CHECK: [[K]] = metadata !{i32 -37, i32 -36, i32 31, i32 32}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>