[PATCH] D22942: [SCEV] Fix runtime error caused by ValueOffsetPair

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Just to be clear, you're going to squash this with https://reviews.llvm.org/rL276136 and check in the result?


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:274
@@ -272,1 +273,3 @@
     ///
+    /// Note the func is different from getRelatedExistingExpansion in that
+    /// it only gets "existing" value in IR. If the `ValueOffsetPair`
----------------
s/func/function

Also, I'm not sure if we need to mention `getRelatedExistingExpansion` etc. here.  I'd just directly state that it gives an expansion for `S` and nothing more.

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:282
@@ +281,3 @@
+
+    /// \brief Try to find ValueOffsetPair for S. The func is mainly used
+    /// to check whether S can be expanded cheaply.
----------------
s/find/find the/ or s/find/find a/
s/func/function/

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:282
@@ +281,3 @@
+
+    /// \brief Try to find ValueOffsetPair for S. The func is mainly used
+    /// to check whether S can be expanded cheaply.
----------------
sanjoy wrote:
> s/find/find the/ or s/find/find a/
> s/func/function/
We don't use `\brief` in new code.

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:284
@@ +283,3 @@
+    /// to check whether S can be expanded cheaply.
+    /// If only return is non-None, we know we can codegen the `ValueOffsetPair`
+    /// into a suitable expansion identical with S so that S can be expanded
----------------
s/If only return is non-None/If this returns a non-None value/

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1998
@@ -1986,3 +1997,3 @@
       At = &ExitingBB->back();
-    if (!findExistingExpansion(
-            SE.getAddExpr(S, SE.getConstant(S->getType(), 1)), At, L))
+    if (!getRelatedExistingExpansion(
+             SE.getAddExpr(S, SE.getConstant(S->getType(), 1)), At, L)
----------------
I don't think you need the `hasValue`.  Applies to all the places where you've used the `Optional` as a boolean.

================
Comment at: test/Analysis/ScalarEvolution/pr28705.ll:2
@@ +1,3 @@
+; PR28705
+; RUN: opt < %s -indvars -debug-only=indvars -S | FileCheck %s
+
----------------
Why do you need `debug-only`?


Repository:
  rL LLVM

https://reviews.llvm.org/D22942