[PATCH] D106852: [SCEV] Fix getAddExpr for adding loop invariants into start of some AddRec

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Spent some more time thinking about this, and updated my running summary (https://github.com/preames/public-notes/blob/master/llvm-loop-opt-ideas.rst#scev-wrap-flags) with my thinking on how to fix the root problem.

However, I do want to explicitly note that I'm open to incrementalism here.  This *particular* patch is addressing a *particular* instance of our flags problem.  I am 100% open to fixing this issue in isolation.  (See inline comments)



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2778
       SmallVector<const SCEV *, 4> AddRecOps(AddRec->operands());
-      // This follows from the fact that the no-wrap flags on the outer add
-      // expression are applicable on the 0th iteration, when the add recurrence
-      // will be equal to its start value.
-      AddRecOps[0] = getAddExpr(LIOps, Flags, Depth + 1);
+      // TODO: If we could prove that the 0th iteration of a loop is guaranteed
+      // we could use inferred flags. This follows from the fact that
----------------
This comment is subtle, and almost correct, but not quite.  We'd have to prove both that the loop loop is always taken, and that the values are defined in the function scope.  (a.g. a GEP off a global pointer would require a non-function scope)


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2788
+      SCEV::NoWrapFlags FlagsForNewStart = Flags;
+      if (UsedLoops.size())
+        FlagsForNewStart = StrengthenNoWrapFlags(
----------------
efriedma wrote:
> I'm not sure I see the connection between UsedLoops.size() and the treatment of nowrap flags.
As with Eli, I don't see why the loops used is relevant.  Take a look at the interesting example from my writeup, and I think you'll see this check is irrelevant.

I'm guessing that this was an attempt to prevent the need for widespread test changes.  Can you confirm that and give a feel for how bad they are?  (Autoupdate is your friend...)

The place I expect this to matter the most is in trip count computation.  Given that, I'd be really curious to see if rebasing this over Eli's D106331 helps reduce that test diff.  That change should get some of the context sensitivity lost here back, and might very well cut down on the impact.  




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

https://reviews.llvm.org/D106852