[PATCH] D78054: Returning scalar function when VF=1

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

Hi @masoud.ataei - almost there!

I have added a couple of comments that you need to address, inlined in the patch.

I also have two minor requests:

1. update the commit message title mentioning the VFDatabase, something like `[VFDatabase] Scalar functions are vector functions with VF =1`.
2. There is nothing about scalarization in this patch, so I guess you can rename the ll test to something like `vectorizeVFone.ll`?

Thank you (again) for your  patience!

Kind regards,

Francesco



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4399
     }
-      assert(VectorF && "Can't create vector function.");
-
----------------
Sorry - I didn't mean to remove this assert. We need to check that sure we don't end up with a null pointer when generating VectorF in line 4384. I like the idea of having assert specifically for the two cases (intrinsics and VFDatabase), as it helps debugging, so instead of restoring this assert, please add another assert on line 4385 (after `VectorF = Intrinsic::getDeclaration(M, ID, TysForDecl);` that says something along the lines of "Can't retrieve vector intrinsic.".


================
Comment at: llvm/test/Transforms/LoopVectorize/scalarizeVFOne.ll:8
+define void @foo(double* %A, double* %C, %type* %B) #0 {
+; CHECK-NOT: Can't create vector function
+entry:
----------------
I wouldn't check the assert being raised. In term of testing for this patch I am happy with the unittest you have written. For the sake of protecting the high level test that made you run into this problem, I suggest you modifying the checks of this test to make sure the loop is vectorized with VF = 1 (I guess you just need to make sure no vector instructions are generated...)


================
Comment at: llvm/unittests/Analysis/VectorUtilsTest.cpp:546
+  VFShape scalarShape = VFShape::getScalarShape(*CI);
+  EXPECT_EQ((new VFDatabase(*CI))->getVectorizedFunction(scalarShape),
+            M->getFunction("g"));
----------------
May I ask why not just ` EXPECT_EQ(VFDatabase(*CI).getVectorizedFunction(scalarShape), ...`? I am missing the reason why you had to use `new` here.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78054