[PATCH] D119121: [test-suite] Add unit tests for vectorizer memory runtime checks.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: SingleSource/UnitTests/Vectorizer/CMakeLists.txt:1
+list(APPEND CXXFLAGS -std=c++17)
 llvm_singlesource()
----------------
Meinersbur wrote:
> Can we let cmake select the flag? 
> 
> https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html
That looks better, thanks!


================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:7
+// vectorized versions of a loop requiring runtime checks on the same inputs
+// with pointers to the same buffer using various offsets. Fails if they do not
+// produce the same results.
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > Something that would have made me understand the purpose easer.
> I just noticed the added "from/to same memory buffer" would be redundant.
Thanks, I added `between reads and writes`. I hope this makes it clearer.


================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:24
+                  int Offset) {
+  if (!std::equal(&Reference[0], &Reference[0] + NumElements, &Tmp[0])) {
+    std::cerr << "Miscompare with offset " << Offset << "\n";
----------------
Meinersbur wrote:
> I assume it is not possible for NaNs to appear?
At the moment it is only used with integer types. I think `uniform_int_distribution` also only works with integer types, but I think. we can skip testing with floating point types .


================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:57-58
+  unsigned N = 100;
+  // Make sure we have enough extra elements so we can be liberal with offsets.
+  unsigned NumArrayElements = N * 8;
+  auto CheckWithOffset = [&](int Offset) {
----------------
Meinersbur wrote:
> I'd have preferred some explicit expression that gives confidence in that we don't accidently go out of range e.g. when increasing the span for offsets ant to know when they actually overlap.
> 
Agreed, but Im not sure how to best provide such an expression, combined with enforcing it when specifying the test loops. I've left it as is for now, but I'm more than happy to adjust it if there's a better alternative.


================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:60-62
+    std::unique_ptr<Ty[]> Input1(new Ty[NumArrayElements]);
+    std::unique_ptr<Ty[]> Reference(new Ty[NumArrayElements]);
+    std::unique_ptr<Ty[]> ToCheck(new Ty[NumArrayElements]);
----------------
Meinersbur wrote:
> It might be more predictable if reusing the same allocation. Otherwise each allocation may have a different alignment and effectively some offsets (relative to virtual address space, eg. page boundaries) are skipped. In case the vectorizer adds a prologue to ensure vector memory accesses are aligned.
> 
Sounds good, I moved it out to `checkOverlappingMemoryOneRuntimeCheck`. Was that what you had in mind?


================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:71
+    // Run scalar function to generate reference output.
+    auto *ReferenceStart = &Reference[0] + NumArrayElements / 2;
+    ScalarFn(ReferenceStart + Offset, ReferenceStart, N);
----------------
Meinersbur wrote:
> [style] No almost-always-auto
Fixed, thanks!


================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:75
+    // Run vector function to generate output to check.
+    auto *StartPtr = &ToCheck[0] + NumArrayElements / 2;
+    VectorFn(StartPtr + Offset, StartPtr, N);
----------------
Meinersbur wrote:
> 
Fixed, thanks!


================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:82-83
+
+  for (int i = -200; i <= 200; i++)
+    CheckWithOffset(i);
+}
----------------
Meinersbur wrote:
> Do we really need that many offsets? The trip count is just 100 so everything outside [-100,100] already seems redundant.
Good point! Originally the tests used larger trip counts, but now [-100, 100] should be sufficient to cover all cases.


================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:218
+    DEFINE_SCALAR_AND_VECTOR_FN2(
+      _Pragma("loop unroll(disable)")
+      for (unsigned i = 0; i < TC; i += 2) {
----------------
Meinersbur wrote:
> 
Updated, thanks!


Repository:
  rT test-suite

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

https://reviews.llvm.org/D119121