We are seeing a case where after the unroll threshold was increased in
295538, a loop is fully unrolled into very large constant expression
chains. These are recursively scanned, leading to timeouts and hence
failures. This prevents having to rescan already scanned operands.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
I've put together a little reproducer. The original case was from the preprocessed CSiBE lwip-0.5.3.preproc/src/core/memp.i source file, this shows the same thing going on:
struct ST {
struct ST* next;
};
static char global[LOOP*sizeof(struct ST)+1];
#define ALIGN(m, y) (m + ((m%y==0) ? 0 : m%y))
void func(void)
{
struct ST* s = (struct ST*)&global;
for(int j = 0; j < LOOP; j++) {
s->next = (struct ST*)ALIGN((unsigned)((char*)s+sizeof(struct ST)), 2);
s = s->next;
}
s->next = 0;
}Compiling with something like "clang -c simple.c -O3 -DLOOP=X" I get these compile times for various values of LOOP:
10 0.2
12 0.3
14 0.6
16 1.2
18 8.6
20 33.9
22 215.0
Comment Actions
The original code looks significantly dodgy to me. From a read of the code and the intent, I think your change is right (the original code always added {FoldedC, FoldedC} which is totally bogus).
However, I think a testcase should be possible?
James
Comment Actions
Hello,
Thanks for taking a look. The only test I could come up with would be one that times out given a high enough LOOP define in the above reproducer. This patch should otherwise be an NFC.
I don't see any other examples of tests we have that are expected to not timeout, but I've come up with something that should, with the patch, compile fine. I'm not sure if it's a great test (without the fix it will just hang) but at least it's something.
Dave
Comment Actions
Hi David,
I agree that a hanging test is not ideal. However, if it's a choice between that or silently regressing, I'd prefer a hanging test. At least people will *notice* if it starts taking an ice age to complete.
LGTM, but can you double check that the test is actually run? I thought LIT only inspected the top rows for RUN: lines and stopped when it found a non-RUN: line.
James
Comment Actions
Sorry for the delay, I was obtaining commit access and then busy with a few other things.
Adding a unittest is an interesting idea. I'm not sure if it would be a lot better though, as it would be testing a fairly minor implementation detail of the Constant Folding interface. The results of the constant folding should be the same, it will just be recursively called a lot less. This test case also has the advantage of making sure nothing else causes the same kind of execution time blow-up.
Unless you have any objections, I'll commit this later today. Let me know.
Dave