[llvm] r275357 - [SCCP] Generalize tryToReplaceInstWithConstant to work also with arguments.

[Davide Italiano via llvm-commits]

On Thu, Jul 14, 2016 at 12:27 PM, Matthew Simpson
<mssimpso at codeaurora.org> wrote:
> Hi Davide,
>
> We're seeing large compile-time regressions with this change and LTO when
> compiling spec2000/2006. For example, the compile-time of spec2000/crafty
> roughly doubles. Would you mind taking a look? We're compiling with "-Ofast
> -flto -fuse-ld=gold -mcpu=kryo". Please let me know if you're unable to
> reproduce. Thanks!
>

Sorry about that, r275468 should help, please let me know if you still
have trouble!

> -- Matt
>
>
>
> On Wed, Jul 13, 2016 at 10:34 PM, Davide Italiano via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>>
>> On Wed, Jul 13, 2016 at 6:51 PM, Eli Friedman <eli.friedman at gmail.com>
>> wrote:
>> > On Wed, Jul 13, 2016 at 6:27 PM, Davide Italiano via llvm-commits
>> > <llvm-commits at lists.llvm.org> wrote:
>> >>
>> >> Author: davide
>> >> Date: Wed Jul 13 20:27:29 2016
>> >> New Revision: 275357
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=275357&view=rev
>> >> Log:
>> >> [SCCP] Generalize tryToReplaceInstWithConstant to work also with
>> >> arguments.
>> >>
>> >> Modified:
>> >>     llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
>> >>
>> >> Modified: llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SCCP.cpp?rev=275357&r1=275356&r2=275357&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/Transforms/Scalar/SCCP.cpp (original)
>> >> +++ llvm/trunk/lib/Transforms/Scalar/SCCP.cpp Wed Jul 13 20:27:29 2016
>> >> @@ -1510,16 +1510,16 @@ bool SCCPSolver::ResolvedUndefsIn(Functi
>> >>    return false;
>> >>  }
>> >>
>> >> -static bool tryToReplaceInstWithConstant(SCCPSolver Solver,
>> >> Instruction
>> >> *Inst,
>> >> -                                         bool shouldEraseFromParent) {
>> >> +template <typename ArgOrInst>
>> >> +static bool tryToReplaceWithConstant(SCCPSolver Solver, ArgOrInst *AI)
>> >> {
>> >
>> >
>> > Can you just use a Value* here?
>> >
>>
>> Sure, I'll change that.
>>
>> >>
>> >>    Constant *Const = nullptr;
>> >> -  if (Inst->getType()->isStructTy()) {
>> >> -    std::vector<LatticeVal> IVs =
>> >> Solver.getStructLatticeValueFor(Inst);
>> >> +  if (AI->getType()->isStructTy()) {
>> >> +    std::vector<LatticeVal> IVs = Solver.getStructLatticeValueFor(AI);
>> >>      if (std::any_of(IVs.begin(), IVs.end(),
>> >>                      [](LatticeVal &LV) { return LV.isOverdefined();
>> >> }))
>> >>        return false;
>> >>      std::vector<Constant *> ConstVals;
>> >> -    StructType *ST = dyn_cast<StructType>(Inst->getType());
>> >> +    StructType *ST = dyn_cast<StructType>(AI->getType());
>> >>      for (unsigned i = 0, e = ST->getNumElements(); i != e; ++i) {
>> >>        LatticeVal V = IVs[i];
>> >>        ConstVals.push_back(V.isConstant()
>> >> @@ -1528,18 +1528,23 @@ static bool tryToReplaceInstWithConstant
>> >>      }
>> >>      Const = ConstantStruct::get(ST, ConstVals);
>> >>    } else {
>> >> -    LatticeVal IV = Solver.getLatticeValueFor(Inst);
>> >> +    LatticeVal IV = Solver.getLatticeValueFor(AI);
>> >>      if (IV.isOverdefined())
>> >>        return false;
>> >> -
>> >> -    Const =
>> >> -        IV.isConstant() ? IV.getConstant() :
>> >> UndefValue::get(Inst->getType());
>> >> +    Const = IV.isConstant() ? IV.getConstant() :
>> >> UndefValue::get(AI->getType());
>> >>    }
>> >>    assert(Const && "Constant is nullptr here!");
>> >> -  DEBUG(dbgs() << "  Constant: " << *Const << " = " << *Inst << '\n');
>> >> +  DEBUG(dbgs() << "  Constant: " << *Const << " = " << *AI << '\n');
>> >>
>> >>    // Replaces all of the uses of a variable with uses of the constant.
>> >> -  Inst->replaceAllUsesWith(Const);
>> >> +  AI->replaceAllUsesWith(Const);
>> >> +  return true;
>> >> +}
>> >> +
>> >> +static bool tryToReplaceInstWithConstant(SCCPSolver Solver,
>> >> Instruction
>> >> *Inst,
>> >> +                                         bool shouldEraseFromParent) {
>> >> +  if (!tryToReplaceWithConstant(Solver, Inst))
>> >> +    return false;
>> >>
>> >>    // Delete the instruction.
>> >>    if (shouldEraseFromParent)
>> >> @@ -1766,17 +1771,8 @@ static bool runIPSCCP(Module &M, const D
>> >>          // TODO: Could use getStructLatticeValueFor to find out if the
>> >> entire
>> >>          // result is a constant and replace it entirely if so.
>> >>
>> >> -        LatticeVal IV = Solver.getLatticeValueFor(&*AI);
>> >> -        if (IV.isOverdefined()) continue;
>> >> -
>> >> -        Constant *CST = IV.isConstant() ?
>> >> -        IV.getConstant() : UndefValue::get(AI->getType());
>> >> -        DEBUG(dbgs() << "***  Arg " << *AI << " = " << *CST <<"\n");
>> >> -
>> >> -        // Replaces all of the uses of a variable with uses of the
>> >> -        // constant.
>> >> -        AI->replaceAllUsesWith(CST);
>> >> -        ++IPNumArgsElimed;
>> >> +        if (tryToReplaceWithConstant(Solver, &*AI))
>> >> +          ++IPNumArgsElimed;
>> >>        }
>> >>      }
>> >
>> >
>> > Testcase?
>> >
>>
>> Sorry if I failed to mention on this commit, but this is NFC as we
>> skip the replacement phase if the argument is of struct type (unless
>> I'm missing something).
>> My next patch, http://reviews.llvm.org/D22329 , changes the behaviour
>> and add test cases.
>>
>>
>> Thanks for your review!
>>
>> --
>> Davide
>>
>> "There are no solved problems; there are only problems that are more
>> or less solved" -- Henri Poincare
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>



-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare