[PATCH] D99569: [LoopVectorize] Fix bug where predicated loads/stores were dropped

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 12 04:39:40 PDT 2021


fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1515
+  bool isPredicatedInst(Instruction *I,
+                        ElementCount VF = ElementCount::getFixed(1)) {
     if (!blockNeedsPredication(I->getParent()))
----------------
I'm not sure about the default. Why does it make sense to default to VF = 1?  Can all users of the function instead pass the right VF?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:1
+; RUN: opt -loop-vectorize -dce -instcombine \
+; RUN:     -force-vector-width=1 \
----------------
as you are only checking specifically for small IR snippets of the predicated blocks, do we actually need `-dce -instcombine`? Running those potentially make the test more fragile, because it may be impacted by changes to `-dce -instcombine`.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:5
+; RUN:     -debug-only=loop-vectorize \
+; RUN:     -S -o - 2>%t < %s | FileCheck %s
+; RUN: FileCheck --check-prefix=DBG %s < %t
----------------
can we instead just do `2>&1 | FileCheck %s` in one go? Splitting them up makes it a bit more inconvenient to reproduce a failure, because 2 lines need to copied/executed?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:21
+;
+;   1   void foo(int *restrict data1, int *restrict data2)
+;   2   {
----------------
this seems already out of sync with the IR below.... Shouldn't the `restrict` be translated to `noalias` in the function definition below? FWIW I think it would be more helpful if you'd add a comment to the test saying this checks that the store in the `if.then` block gets properly predicated if VF = 1 or something like that, as the IR is already quite compact :)


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:45
+;
+; DBG: LV: Scalarizing and predicating:  store i32 %1, i32* %arrayidx, align 4
+
----------------
Is this check crucial? Seems like the IR checks already check this and we could remove it and execute it even with asserts.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:56
+  %1 = load i32, i32* %arrayidx2, align 4
+  %cmp = icmp sgt i32 %0, %1
+  br i1 %cmp, label %if.then, label %if.end
----------------
do we need 2 loads here or could one be removed and compare to a constant?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:72
+
+; This is _not_ an SVE-specific test -- see comment at the top of this file for
+; an explanation of why +sve is used here.
----------------
If it's not SVE specific, can we have 2 run lines, one with `-mattr=+sve` and one without?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99569



More information about the llvm-commits mailing list