[llvm] r296992 - [SCEV] Decrease the recursion threshold for CompareValueComplexity

[Hans Wennborg via llvm-commits]

Merged to 4.0 in r297164.

On Mon, Mar 6, 2017 at 11:43 AM, Hans Wennborg <hans at chromium.org> wrote:
> Thanks for jumping in and fixing. Yes, we should probably make time to
> take this. I've put it on my radar.
>
> On Sun, Mar 5, 2017 at 4:04 PM, Sanjoy Das
> <sanjoy at playingwithpointers.com> wrote:
>> Hi Hans,
>>
>> Do we have time to pick this for 4.0 (once the buildbots have cycled
>> through this change)?  It is a low risk fix for PR32142.
>>
>> -- Sanjoy
>>
>>
>> On March 5, 2017 at 4:01:12 PM, Sanjoy Das via llvm-commits
>> (llvm-commits at lists.llvm.org) wrote:
>>> Author: sanjoy
>>> Date: Sun Mar 5 17:49:17 2017
>>> New Revision: 296992
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=296992&view=rev
>>> Log:
>>> [SCEV] Decrease the recursion threshold for CompareValueComplexity
>>>
>>> Fixes PR32142.
>>>
>>> r287232 accidentally increased the recursion threshold for
>>> CompareValueComplexity from 2 to 32. This change reverses that change
>>> by introducing a separate flag for CompareValueComplexity's threshold.
>>>
>>> Modified:
>>> llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>>> llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp
>>>
>>> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=296992&r1=296991&r2=296992&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
>>> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Sun Mar 5 17:49:17 2017
>>> @@ -132,10 +132,15 @@ static cl::opt AddOpsInlineThr
>>> cl::desc("Threshold for inlining multiplication operands into a SCEV"),
>>> cl::init(500));
>>>
>>> -static cl::opt
>>> - MaxCompareDepth("scalar-evolution-max-compare-depth", cl::Hidden,
>>> - cl::desc("Maximum depth of recursive compare complexity"),
>>> - cl::init(32));
>>> +static cl::opt MaxSCEVCompareDepth(
>>> + "scalar-evolution-max-scev-compare-depth", cl::Hidden,
>>> + cl::desc("Maximum depth of recursive SCEV complexity comparisons"),
>>> + cl::init(32));
>>> +
>>> +static cl::opt MaxValueCompareDepth(
>>> + "scalar-evolution-max-value-compare-depth", cl::Hidden,
>>> + cl::desc("Maximum depth of recursive value complexity comparisons"),
>>> + cl::init(2));
>>>
>>> static cl::opt
>>> MaxAddExprDepth("scalar-evolution-max-addexpr-depth", cl::Hidden,
>>> @@ -496,7 +501,7 @@ static int
>>> CompareValueComplexity(SmallSet, 8> &EqCache,
>>> const LoopInfo *const LI, Value *LV, Value *RV,
>>> unsigned Depth) {
>>> - if (Depth > MaxCompareDepth || EqCache.count({LV, RV}))
>>> + if (Depth > MaxValueCompareDepth || EqCache.count({LV, RV}))
>>> return 0;
>>>
>>> // Order pointer values after integer values. This helps SCEVExpander form
>>> @@ -583,7 +588,7 @@ static int CompareSCEVComplexity(
>>> if (LType != RType)
>>> return (int)LType - (int)RType;
>>>
>>> - if (Depth > MaxCompareDepth || EqCacheSCEV.count({LHS, RHS}))
>>> + if (Depth > MaxSCEVCompareDepth || EqCacheSCEV.count({LHS, RHS}))
>>> return 0;
>>> // Aside from the getSCEVType() ordering, the particular ordering
>>> // isn't very important except that it's beneficial to be consistent,
>>>
>>> Modified: llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp?rev=296992&r1=296991&r2=296992&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp (original)
>>> +++ llvm/trunk/unittests/Analysis/ScalarEvolutionTest.cpp Sun Mar 5 17:49:17
>>> 2017
>>> @@ -465,7 +465,7 @@ TEST_F(ScalarEvolutionsTest, Commutative
>>> });
>>> }
>>>
>>> -TEST_F(ScalarEvolutionsTest, SCEVCompareComplexity) {
>>> +TEST_F(ScalarEvolutionsTest, CompareSCEVComplexity) {
>>> FunctionType *FTy =
>>> FunctionType::get(Type::getVoidTy(Context), std::vector(), false);
>>> Function *F = cast(M.getOrInsertFunction("f", FTy));
>>> @@ -532,6 +532,42 @@ TEST_F(ScalarEvolutionsTest, SCEVCompare
>>> EXPECT_NE(nullptr, SE.getSCEV(Acc[0]));
>>> }
>>>
>>> +TEST_F(ScalarEvolutionsTest, CompareValueComplexity) {
>>> + IntegerType *IntPtrTy = M.getDataLayout().getIntPtrType(Context);
>>> + PointerType *IntPtrPtrTy = IntPtrTy->getPointerTo();
>>> +
>>> + FunctionType *FTy =
>>> + FunctionType::get(Type::getVoidTy(Context), {IntPtrTy, IntPtrTy}, false);
>>> + Function *F = cast(M.getOrInsertFunction("f", FTy));
>>> + BasicBlock *EntryBB = BasicBlock::Create(Context, "entry", F);
>>> +
>>> + Value *X = &*F->arg_begin();
>>> + Value *Y = &*std::next(F->arg_begin());
>>> +
>>> + const int ValueDepth = 10;
>>> + for (int i = 0; i < ValueDepth; i++) {
>>> + X = new LoadInst(new IntToPtrInst(X, IntPtrPtrTy, "", EntryBB), "",
>>> + /*isVolatile*/ false, EntryBB);
>>> + Y = new LoadInst(new IntToPtrInst(Y, IntPtrPtrTy, "", EntryBB), "",
>>> + /*isVolatile*/ false, EntryBB);
>>> + }
>>> +
>>> + auto *MulA = BinaryOperator::CreateMul(X, Y, "", EntryBB);
>>> + auto *MulB = BinaryOperator::CreateMul(Y, X, "", EntryBB);
>>> + ReturnInst::Create(Context, nullptr, EntryBB);
>>> +
>>> + // This test isn't checking for correctness. Today making A and B resolve to
>>> + // the same SCEV would require deeper searching in CompareValueComplexity,
>>> + // which will slow down compilation. However, this test can fail (with LLVM's
>>> + // behavior still being correct) if we ever have a smarter
>>> + // CompareValueComplexity that is both fast and more accurate.
>>> +
>>> + ScalarEvolution SE = buildSE(*F);
>>> + auto *A = SE.getSCEV(MulA);
>>> + auto *B = SE.getSCEV(MulB);
>>> + EXPECT_NE(A, B);
>>> +}
>>> +
>>> TEST_F(ScalarEvolutionsTest, SCEVAddExpr) {
>>> Type *Ty32 = Type::getInt32Ty(Context);
>>> Type *ArgTys[] = {Type::getInt64Ty(Context), Ty32};
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>