[PATCH] D68956: [IndVars] Fix a miscompile in off-by-default loop predication implementation

[Philip Reames via Phabricator via llvm-commits]

reames marked 5 inline comments as done.
reames added inline comments.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2825
+    if (!DT->dominates(ExitingBlocks[i-1], ExitingBlocks[i]))
+      return Changed;
+
----------------
ebrevnov wrote:
> This is not incorrect but too conservative. If for say last exit we are optimizing has index k, then it's enough to check that exit k dominates all consecutive blocks. Saying that I don't think it will make any difference in practice since back edge taken count won't be computed anyway.
Completely agreed.  I thought my comment indicated that, do I need to clarify something?


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2829
+  // after all exit[i] such j > i.
+  for (unsigned i = 0; i < ExitingBlocks.size(); i++)
+    if (BadExit(ExitingBlocks[i])) {
----------------
xbolva00 wrote:
> e = ExitingBlocks.size()
> 
> i < e
Done, though, this is a micro-optimization at best.  And with a decent compiler, not even that.  

(It's common and idiomatic in LLVM code, just noting the reason I don't particularly like it for the record)


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

https://reviews.llvm.org/D68956