[PATCH] D107806: [LoopFlatten] Fix assertion failure

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:97
+static bool
+setLoopComponent(Value *&RHS, Value *&TripCount, BinaryOperator *&Increment,
+                 SmallPtrSetImpl<Instruction *> &IterationInstructions) {
----------------
Nit: perhaps rename `RHS` to `TC` so that it's a bit more descriptive.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:99
+                 SmallPtrSetImpl<Instruction *> &IterationInstructions) {
+  // This is a helper function for use in findLoopComponents which sets the loop
+  // trip count.
----------------
Nit: don't think this comment adds much, and don't mind if you delete it. I think the function is small and its name descriptive enough.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:194
+    return setLoopComponent(RHS, TripCount, Increment, IterationInstructions);
+  else {
+    ConstantInt *ConstantRHS = dyn_cast<ConstantInt>(RHS);
----------------
Nit: I think we can get rid of the `else` now.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:179
+  const SCEV *SCEVRHS = SE->getSCEV(RHS);
+  if (SCEVRHS != SCEVTripCount) {
+    ConstantInt *ConstantRHS = dyn_cast<ConstantInt>(RHS);
----------------
RosieSumpter wrote:
> SjoerdMeijer wrote:
> > This has become quite complicated logic. I am wondering if we can handle the simple case first to reduce indentation and improve readability:
> > 
> >   if (SCEVRHS == SCEVTripCount)
> >     return setLoopComponents(RHS, Increment);
> > 
> > where `setLoopComponents` is a new helper that includes the code at lines 229 - 233 and sets `TripCount`.
> > 
> > 
> > 
> > 
> I've added a helper - hopefully it's what you had in mind, but I'm happy to change it if not
Thanks, that's exactly what I had in mind. 

I just added a few nits here and there, while I am going to take your patch now and run testing.


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

https://reviews.llvm.org/D107806