[PATCH] D111066: Disable "[SCEV] Prove implications of different type via truncation"

Tue Oct 5 01:24:43 PDT 2021

bjope added a comment.

In D111066#3041655 <https://reviews.llvm.org/D111066#3041655>, @mkazantsev wrote:

> This patch is one year old. It is extremely surprising that you are trying to disable it rather than find the root cause.
>
> I don't believe this is infinite recursion. Before all, infinite recursion should end up with stack overflow and not timeout. I can believe there is some exponential explosion happening, but in this case you need to find how exactly it happens and fix the acutal reason. The fact that this patch triggers something very slow doesn't convince me that this patch is guilty, and this test doesn't allow to figure it out easily.
>
> Could you please check (at least, but not limited to):
>
> - Do we end up creating some HUGE SCEVs in your test?
> - Do we end up making the same implication query millions of times?
> - Do we end up walking huge dom trees for execution of some queries?
>
> Before we decide to switch something off, I want to know why exactly.

Hi @mkazantsev,

I have tried debugging PR51869 a bit, but I don't really know this code. I've wrote a bit about it in https://bugs.llvm.org/show_bug.cgi?id=51869, and added you as susbscriber, hoping to get some feedback or reactions. But since there was no reactions I did not add much info (I thought that maybe the author of the code at least could take a quick look to see that it wasn't an obvious mistake. I for example see that there are a couple of checks related to LHS before calling sKnownPredicate(ICmpInst::ICMP_ULE, FoundLHS, MaxValue), but nothing for RHS and it is the sKnownPredicate(ICmpInst::ICMP_ULE, FoundRHS, MaxValue) call that didn't seem to return (at least not with the reproducer in the PR51869). If I remember correctly I could not see any deep recursion, but there were lots of calls to ScalarEvolution::isImpliedCond( and I suspected some kind of ping-pong effect.

We've seen timeouts in several different test cases during the last year. And bisecting points at your commit every time. However, it was quite recently we managed to reduce one of those test cases down to something smaller (~5000 lines of IR, running on a single pass, without any downstream intrinsics and without any unusual address spaces etc). I think a collegue of mine has tried to reach out for help, and we have added some comments in https://reviews.llvm.org/D89548 trying to get some reactions. This ticket was kind of another attempt to get some help/feedback. Nice to see that it got some interest. However, this ticket is about disabling the analysis that is causing bad time complexity, and perhaps we should discuss the actual problem with PR51869 in bugzilla.

Btw, since the only test case that seem to validate that we actually get some transforms given that this feature is turned on is the test case using `-scalar-evolution-use-expensive-range-sharpening` which is disabled by default. So we do not really have any lit tests verifying that this extra analysis is working as intended, except for that special test case. That could actually be another reason to disable this, IMHO.

Anyway, I want to switch this off because it has been causing lots of trouble and I do not know exactly why. If I knew why, then hopefully that could be fixed instead. My idea was to leave that until someone bothered about PR51869 (or until someone though it would be worth trying to enable this feature again (because right now it seem a bit broken when it comes to time complexity). I'd be happy to see a proper solution to the problem though. So the I figure the goal should be to enable `-scalar-evolution-prove-implications-via-truncation` again later. But I think I need some help debugging and figuring out why we start to see these timeout problems given your patch.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111066/new/

https://reviews.llvm.org/D111066