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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer 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>()))) {
----------------
fhahn wrote:
> 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.
Alright, fair enough, let's then first see what using SCEV exactly means and would involve.

> 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.

Yeah, the trip count is the easy part, but it's also the other loop components like the increment, test, etc., which are not provided by existing APIs. Thus, with major surgery, I probably also meant getting rid of `findLoopComponents` and adding something similar to Loop or LoopUtils, possibly using SCEV more. 

This little pattern match extension here might still be innocent and the way to go, but like I said, let's review SCEV usage first then.


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

https://reviews.llvm.org/D105802