[llvm] r294181 - [SCEV] limit recursion depth and operands number in getAddExpr

[Fukalov, Daniil via llvm-commits]

Hi, Chandler,

The initial issue I fixed was “soft hang behavior” of SCEV on a quite big IR. I didn’t find a way to make a test/unittest that will check something like amount of time an input IR is processed. The test I added will “soft hang” without recursion depth limit or with a limit set to high value. Would you please advise what approach should I use to check such issues? Or if “very slow test” is a good criteria to check such a regression, what estimated time for test will be supposed as “not regressed”?

Thanks in advance!
From: Chandler Carruth [mailto:chandlerc at gmail.com]
Sent: Tuesday, February 07, 2017 12:39 AM
To: Chandler Carruth; Sanjoy Das
Cc: Fukalov, Daniil; llvm-commits
Subject: Re: [llvm] r294181 - [SCEV] limit recursion depth and operands number in getAddExpr

On Mon, Feb 6, 2017 at 11:41 AM Chandler Carruth <chandlerc at gmail.com<mailto:chandlerc at gmail.com>> wrote:
On Mon, Feb 6, 2017 at 11:37 AM Sanjoy Das via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
Let's revert for now then?  Once we figure out some other way to test
this, we'll re-land.

I'm happy with that or with blindly throttling the test. I don't know how serious the issue being fixed is.

Seeing no response yet and after an IRC discussion with Sanjoy, I've reduced the iterations in the test to 10 and added a FIXME to figure out how to really test what this patch is intending to test. Landed in r294241.



On Feb 6, 2017 11:27, "Chandler Carruth via llvm-commits"
<llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
>
> On Mon, Feb 6, 2017 at 11:22 AM Chandler Carruth <chandlerc at gmail.com<mailto:chandlerc at gmail.com>> wrote:
>>
>> This adds a unittest that *with optimizations* takes over 20 seconds to run. That seems excessive.
>
>
> Specifically...
>
>>
>> On Mon, Feb 6, 2017 at 4:49 AM Daniil Fukalov via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
>>>
>>> +static cl::opt<unsigned>
>>> +    MaxAddExprDepth("scalar-evolution-max-addexpr-depth", cl::Hidden,
>>> +                    cl::desc("Maximum depth of recursive AddExpr"),
>>> +                    cl::init(32));
>
>
> The recursion limit is 32...
>
>
>>>
>>> +  for (int i = 0; i < 1000; i++) {
>>> +    Mul1 = BinaryOperator::CreateMul(Add2, Add1, "", EntryBB);
>>> +    Add1 = Add2;
>>> +    Add2 = BinaryOperator::CreateAdd(Mul1, Add1, "", EntryBB);
>>> +  }
>
>
> And the test builds IR with 1000 adds and 1000 muls. It isn't clear at all that this is a reasonable way to unittest something.
>
> What's worse, when I make this 1000 be 10 instead, nothing fails despite transitioning from above to below the recursion limit above. So I'm not really sure what this is testing at all?

I don't think it is supposed to fail that way.  The buggy behavior is
to, with an infinite (say) recursion limit, take an exponential amount
of time to construct the SCEV expression.

-- Sanjoy

>
> Anyways, this is making each of my test runs over 50% slower (18s -> 26s) and I suspect several build bots are similarly impacted.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170207/72d1e29b/attachment-0001.html>