RE: [PATCH] Converting ‘sext of addrec’ to ‘addrec of sext’

[Nema, Ashutosh]

Thanks Sanjoy for review.
I'll work on your comments and come back.

Regards,
Ashutosh

-----Original Message-----
From: Sanjoy Das [mailto:sanjoy at playingwithpointers.com] 
Sent: Thursday, May 07, 2015 12:37 AM
To: Nema, Ashutosh; david.majnemer at gmail.com; sanjoy at playingwithpointers.com
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Converting ‘sext of addrec’ to ‘addrec of sext’

Comments inline.

This kind of change *definitely* needs several test cases.


REPOSITORY
  rL LLVM

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4332
@@ +4331,3 @@
+  case Instruction::SExt: {
+    // Converting 'sext of addRec' to 'addRec of sext'
+    // i.e. (sext i32 Addrec{2,+,2}<nuw><%for.body4> to i64)
----------------
This needs to go in `ScalarEvolution::getSignExtendExpr`.  There are a bunch of other rules for turning a sign extend of an add rec into an addrec there.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4339
@@ +4338,3 @@
+    if (AddRec && (AddRec->getNoWrapFlags(SCEV::FlagNSW) ||
+                   AddRec->getNoWrapFlags(SCEV::FlagNUW))) {
+      SmallVector<const SCEV *, 4> Operands;
----------------
I think you're also transforming `sext({S,+,X}<nuw>)` to `{sext S,+,sext X}` which isn't correct.

================
Comment at: test/Analysis/ScalarEvolution/infer-prestart-no-wrap.ll:14
@@ -13,3 +13,3 @@
 ; CHECK: %idx.inc.sext = sext i32 %idx.inc to i64
-; CHECK-NEXT: -->  {(1 + (sext i32 %start to i64)),+,1}<nsw><%loop>
+; CHECK-NEXT: -->  {(sext i32 (1 + %start) to i64),+,1}<nsw><%loop>
   %condition = icmp eq i32 %counter, 1
----------------
Note that these are regressions: we know that `%start` is `slt` `127` so we should be able to use the more canonical `{(1 + (sext i32 %start to i64)),+,1}<nsw><%loop>` here.

http://reviews.llvm.org/D9521

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/