[PATCH] D88460: Strlen loop idiom recognition

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1691
+  }
+  if (!ResInst)
+    return false;
----------------
anjankgk wrote:
> efriedma wrote:
> > anjankgk wrote:
> > > efriedma wrote:
> > > > anjankgk wrote:
> > > > > efriedma wrote:
> > > > > > I'm not sure how ResInst is connected to the rest of this transform; why are we transforming the operand of some random PHI node?
> > > > > I added a comment for clarity - 
> > > > >   // Finally, we find the phi node that corresponds to the distance between the
> > > > >   // pointers and replace it's uses by the call to strlen in the transformed
> > > > >   // code.
> > > > > 
> > > > The patch doesn't appear to prove that anywhere.
> > > I am getting the `sub` instruction saved in ResInst and replacing that in line# 1689. Is that something that you prefer to be done in a different way?
> > How do you know it's a sub instruction?
> I agree. The current code is actually just looking for the phi node from the predecessor (`LoopExitBB`) block. I think, I should add a code to ensure that is actually a `sub` instruction. However, I am afraid that that could again lead to a pattern-matching kind of code which was initially posted.
What happens if we ignore ResInst, and rewrite LCSSAPhi instead?  That still eliminates the inner loop; later passes should optimize the resulting arithmetic.


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

https://reviews.llvm.org/D88460