[PATCH] D132261: [SLP]Do not reduce repeated values, use scalar red ops instead.

[Ivan Kelarev via Phabricator via llvm-commits]

ikelarev added a comment.

Hi Alexey,

It turned out that in some cases the transformation introduced in this change doesn't work properly. Let's consider this code:

  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
  target triple = "x86_64-unknown-linux-gnu"
  
  define i32 @main() {
  entry:
    %sq = alloca [64 x i32], i32 0, align 16
    
    %0 = getelementptr inbounds [64 x i32], ptr %sq, i64 0, i64 1
    %elt_1 = load i32, ptr %0, align 4
    %1 = getelementptr [64 x i32], ptr %sq, i64 0, i64 2
    %elt_2 = load i32, ptr %1, align 8
    %2 = getelementptr [64 x i32], ptr %sq, i64 0, i64 3
    %elt_3 = load i32, ptr %2, align 4
    %3 = getelementptr [64 x i32], ptr %sq, i64 0, i64 4
    %elt_4 = load i32, ptr %3, align 16  
    
    %4 = add i32 %elt_2, %elt_3
    %5 = add i32 %4, %elt_2
    %6 = add i32 %5, %elt_1  
    %7 = add i32 %6, %elt_4
    %8 = add i32 %7, %elt_3
    %9 = add i32 %8, %elt_2
    %10 = add i32 %9, %elt_1
    
    %call = tail call i32 (ptr, ...) null(ptr null, i32 %10)
    ret i32 0
  }

It's easy to see that %10 fianl value is 2*%elt_1 + 3*%elt_2 + 2*%elt_3 + %elt_4. But actually we get this code:

  $ opt -S test.ll -passes=slp-vectorizer
  ...
  define i32 @main() {
  entry:
    %sq = alloca [64 x i32], i32 0, align 16
    %0 = getelementptr inbounds [64 x i32], ptr %sq, i64 0, i64 1
    %1 = load <4 x i32>, ptr %0, align 4
    %2 = mul <4 x i32> %1, <i32 3, i32 2, i32 2, i32 1>
    %3 = call i32 @llvm.vector.reduce.add.v4i32(<4 x i32> %2)
    %call = tail call i32 (ptr, ...) null(ptr null, i32 %3)
    ret i32 0
  }

<i32 3, i32 2, i32 2, i32 1> value doesn't look correct, it should be <i32 2, i32 3, i32 2, i32 1>. Running the test with -slp-optimize-identity-hor-reduction-ops=false gives us this code:

  $ opt -S test.ll -passes=slp-vectorizer -slp-optimize-identity-hor-reduction-ops=false
  ...
  define i32 @main() {
  entry:
    %sq = alloca [64 x i32], i32 0, align 16
    %0 = getelementptr inbounds [64 x i32], ptr %sq, i64 0, i64 1
    %1 = load <4 x i32>, ptr %0, align 4
    %2 = shufflevector <4 x i32> %1, <4 x i32> poison, <8 x i32> <i32 1, i32 1, i32 1, i32 2, i32 2, i32 0, i32 0, i32 3>
    %3 = call i32 @llvm.vector.reduce.add.v8i32(<8 x i32> %2)
    %call = tail call i32 (ptr, ...) null(ptr null, i32 %3)
    ret i32 0
  }

<i32 1, i32 1, i32 1, i32 2, i32 2, i32 0, i32 0, i32 3> consist of two zeros and three ones which is correct which means that the incorrectness was introduced with this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132261