[PATCH] D12520: [InstCombineCasts] Cast elimination for sext->lshr->trunc patterns

[Arnaud de Grandmaison via llvm-commits]

aadg added a subscriber: aadg.
aadg accepted this revision.
aadg added a reviewer: aadg.
aadg added a comment.
This revision is now accepted and ready to land.

Hi Jakub,

Thanks for looking into this. This looks good to me with some fixes addressed (see below).


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:504
@@ +503,3 @@
+  // conversion.
+  // It works because bits comming from sign extension have the same value as
+  // sign bit of the original value; performing ashr instead of lshr
----------------
Typo: s/comming/coming/

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:513
@@ +512,3 @@
+    // the original lshr aren't pulled into the value after truncation, so we
+    // can only shift by values smaller then the size of destanation type (in
+    // bits).
----------------
Typo: s/destanation/destination/

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:516-517
@@ +515,4 @@
+    if (Cst->getValue().ult(ASize)) {
+      // We can safely zero extend the result of ashr, since it'll get
+      // truncated.
+      Value *Shift = Builder->CreateAShr(A, Cst->getZExtValue());
----------------
Isn't this comment redundant with the previous one ?

================
Comment at: test/Transforms/InstCombine/cast.ll:1072-1073
@@ +1071,4 @@
+; CHECK-LABEL: @test86(
+; CHECK:  %s.1 = ashr i16 %v, 4
+; CHECK-NEXT: ret i16 %s.1
+}
----------------
Here you should pattern match the result, and reuse it when checking the return. This will make the test more robust if anything changes in the naming of the values.

================
Comment at: test/Transforms/InstCombine/cast.ll:1084-1085
@@ +1083,4 @@
+; CHECK-LABEL: @test87(
+; CHECK:  %a.1 = ashr i16 %v, 12
+; CHECK-NEXT: ret i16 %a.1
+}
----------------
Same. Use pattern matching.

================
Comment at: test/Transforms/InstCombine/cast.ll:1096-1099
@@ +1095,6 @@
+; CHECK-LABEL: @test88(
+; CHECK:  %a = sext i16 %v to i32
+; CHECK-NEXT: %s = ashr i32 %a, 18
+; CHECK-NEXT: %t = trunc i32 %s to i16
+; CHECK-NEXT: ret i16 %t
+}
----------------
Same.


Repository:
  rL LLVM

http://reviews.llvm.org/D12520