[PATCH] D105632: [LV] Use lookThroughAnd with logical reductions

Thu Jul 8 10:58:48 PDT 2021

dmgreen added inline comments.

================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-inloop-pred.ll:1598
+; CHECK-NEXT:    [[TMP32:%.*]] = trunc <4 x i32> [[TMP31]] to <4 x i8>
+; CHECK-NEXT:    [[TMP33:%.*]] = call i8 @llvm.vector.reduce.and.v4i8(<4 x i8> [[TMP32]])
 ; CHECK-NEXT:    br i1 true, label [[DOT_CRIT_EDGE:%.*]], label [[SCALAR_PH]]
----------------
david-arm wrote:
> Given the reduction intrinsic is no longer in the loop is it worth moving this to a different file? Any thoughts @dmgreen @spatel?
I'm pretty sure these tests were added to make sure that the combination of inloop reductions and lookThroughAnd worked together correctly. If they still appear to be working, they should be OK to keep here to make sure that combo keeps working in the future.

================
Comment at: llvm/test/Transforms/LoopVectorize/trunc-reductions.ll:186
+  %ext = zext i16 %load to i32
+  %icmp = icmp sgt i32 %sum.02, %ext
+  %min = select i1 %icmp, i32 %sum.02, i32 %ext
----------------
Are we sure that smin/smax work correctly with this? We go from cutting off the top bits (potentially turning a signed number into an unsigned number) to not doing that.  Or maybe going from a signed min/max to a unsigned one, from the look of this test? I may be wrong, but I think something like this transform needs to be valid for the op: https://alive2.llvm.org/ce/z/5XJ4ZU.

umin/umax may still be OK, if the initial value from the phi is within range. But it doesn't reduce the vector width.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105632