[PATCH] D76669: [LoopVectorize] Fix crash on "getNoopOrZeroExtend cannot truncate!" (PR45259)

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, with some suggestions to improve the test case. Please try to reduce the test case to the things required for the crash only. For example, the function below should be enough (includes a bit of renaming and also use for %t3.i, to make it less prone to being optimized away).

  define i8 @widget(i8* %arr, i8 %t9) {
  bb:
    br label %bb6
  
  bb6:
    %t1.0 = phi i8* [ %arr, %bb ], [ null, %bb6 ]
    %c = call i1 @cond()
    br i1 %c, label %for.preheader, label %bb6
  
  for.preheader:
    br label %for.body
  
  for.body:
    %iv = phi i8 [ %iv.next, %for.body ], [ 0, %for.preheader ]
    %iv.next = add i8 %iv, 1
    %ptr = getelementptr inbounds i8, i8* %arr, i8 %iv.next
    %t3.i = icmp slt i8 %iv.next, %t9
    %t3.i8 = zext i1 %t3.i to i8
    store i8 %t3.i8, i8* %ptr
    %ec = icmp eq i8* %t1.0, %ptr
    br i1 %ec, label %for.exit, label %for.body
  
  for.exit:
    %iv.next.lcssa = phi i8 [ %iv.next, %for.body ]
    ret i8 %iv.next.lcssa
  }
  
  declare i1 @cond()



================
Comment at: llvm/test/Transforms/LoopVectorize/pr45259.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -loop-vectorize -S | FileCheck %s
----------------
I think for the test it would be better to not auto-generate the checks. Here we mostly care about vectorizing without crashing. It should be enough to check if there's a vector body for the right loop with some vector instructions. 

The auto-generated checks seems to verbose here and also prone to changes when small things change, potentially causing unnecessary large diffs on further changes.




================
Comment at: llvm/test/Transforms/LoopVectorize/pr45259.ll:5
+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"
+
----------------
Tests directly in `llvm/test/Transforms/LoopVectorize/` should be target independent. If the x86 triple is required, please move it to the X86 subdirectory. But it should not be required. As we are not interested in cost-modeling here I think, you should be able to instead pass something like  `-force-vector-width=4 -force-vector-interleave=1` and remove the triple.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr45259.ll:9
+
+define dso_local void @widget() local_unnamed_addr {
+; CHECK-LABEL: @widget(
----------------
nit: dso_local, local_unnamed_addr not required


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

https://reviews.llvm.org/D76669