[PATCH] D105802: [LoopFlatten] Fix missed LoopFlatten opportunity

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:175
+    Value *LatchValue = InductionPHI->getIncomingValueForBlock(Latch);
+    if (match(LatchValue,
+              m_c_Add(m_Specific(InductionPHI), m_ConstantInt<1>()))) {
----------------
SjoerdMeijer wrote:
> fhahn wrote:
> > Those kinds of patterns are very fragile in general. Would it be possible to use SCEV instead, which makes it easy to analyse the induction variables & trip counts in a more robust way?
> You're absolutely right. In fact, there's a quite some pattern matching going on for things like getLoopTest, getLoopIncrement, etc., that you'd probably expect to be present in a loop util helper function or something like that, but they aren't. The pattern matching makes some things easier though (as we are looking for some specific patterns). I feel that moving some things to helpers and using SCEV is major surgery, and if we promise not to add more pattern matching after this, I was hoping to postpone this surgery. I.e., we are on a little mission to get LoopFlatten enabled by default (we have this enabled for years now downstream), and before we do that we wanted to fix 2 minor things:
> - this minor extension for a case which you expect to trigger,
> - and we need to strengthen an overflow check.
> 
> After this, I think a nice follow up project is indeed to see if we can move code to helpers and use SCEV.
>  I was hoping to postpone this surgery. I.e., we are on a little mission to get LoopFlatten enabled by default 

I understand the motivation, but I am not sure it is the best way forward. If SCEV is the the right tool to use, I think it would be great to make the switch before enabling it by default; I am not too familiar with the details, but it looks like the pattern matching of induction variables and compares to get the loop trip counts seems like something that SCEV already provides.

After it is enabled by default, the motivation for a substantial refactoring might be smaller than before enabling it. When enabling it by default, it is also beneficial for the pass to be as general as possible, so fundamental problems can be flushed out early and the compile-time/code-size impact is also representative on a large set of inputs.

Also, not using SCEV makes it harder to reason about the pass, because we can't rely on trusting SCEV to correctly handle all the difficult cases with respect to overflows and such when it comes to trip counts and inductions. We instead need to verify the manual patterns and their interaction with the dependent code. For example, it seems like using SCEV would allow us to be more confident when reasoning about some of the issues in the comments below.


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

https://reviews.llvm.org/D105802