[llvm] r204076 - Use range metadata instead of introducing selects.

Lang Hames lhames at gmail.com
Fri Mar 21 23:21:15 PDT 2014


Hi Dan,

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.

Is it possible to fix this, or revert it until the performance issue can be
worked out?

Cheers,
Lang.


On Mon, Mar 17, 2014 at 12:57 PM, Dan Gohman <dan433584 at gmail.com> wrote:

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


More information about the llvm-commits mailing list