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

[Wei Mi via llvm-commits]

wmi added a comment.

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


I can checkin them consecutively. which way do you think is better?


================
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`
----------------
sanjoy wrote:
> 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.
I wanted to emphasize their difference since they have similar names but different usages.  

I simplified the comment to: /// Try to find existing LLVM IR value for S available at the point At. 

================
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:
> sanjoy wrote:
> > s/find/find the/ or s/find/find a/
> > s/func/function/
> We don't use `\brief` in new code.
Fixed.

================
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
----------------
sanjoy wrote:
> s/If only return is non-None/If this returns a non-None value/
Fixed.

================
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)
----------------
sanjoy wrote:
> I don't think you need the `hasValue`.  Applies to all the places where you've used the `Optional` as a boolean.
Oh, that looks better. Thanks.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D22942