[PATCH] D70097: [SCEV] Add missing cache queries

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev accepted this revision.
mkazantsev added a comment.
This revision is now accepted and ready to land.

Looks good. Sorry for long delay.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2455
 
+  if (SCEV *S = std::get<0>(findExistingSCEVInCache(scAddExpr, Ops))) {
+    static_cast<SCEVAddExpr *>(S)->setNoWrapFlags(Flags);
----------------
ekatz wrote:
> loladiro wrote:
> > I know we do this in a few other places, but why is this legal? Isn't it possible we're still using the old SCEV somewhere, but without the flags?
> That's a question I also had.. I am not an expert of this code, but I wanted to be consistent with all other places, where this flag is set on the cached value.
> Maybe someone more familiar with the code can elaborate.
> I guess you can raise that question in llvm-dev, but if it is a bug, then we should open a separate one - specific for this.
This has been a weird exception from SCEV immutability for a long enough time. The idea is that all SCEVs should be the same in all places of the program. It means that if in one place we can guarantee nsw for some addition, it should affect other places too. There have been discussions about how to get rid of it, but afaik we don't currently have a conclusive decision. The best way of action here is to follow the existing model.


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

https://reviews.llvm.org/D70097