[PATCH] D124646: [SCEV] Support modelling of same base pointer `select`s in more complex than most trivial cases (when there is a base variable offset)

[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 nice, but I'd suggest some restructuring.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6001
 
+static void factorCommonOperandsOutOfAddExprs(
+    ScalarEvolution *SE, const SCEV *LHSExpr, const SCEV *RHSExpr,
----------------
Should it be a method of `ScalarEvolution`? I looks like it can be reused somewhere.

Should we make it `bool` and return `false` if there are no common expressions?



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6036
+  }
+
+  CommonExpr = SE->getAddExpr(CommonOps);
----------------
I'd rather bail here if we detected that no common ops were found just to save CT on SCEV simplification.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6040
+  NewRHSExpr = SE->getAddExpr(NewRHSOps);
+}
+
----------------
Thinking more of it, as a follow-up maybe: if both old LHS and RHS had no-wrap flags, then all new expressions and Common expr should also preserve them. Need to think more, but maybe it's worth doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124646