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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I doubt that `  __attribute__((noinline)) ` suffices as an optimization barrier, IPO such as IPConstantPropagation/FunctionSpecialization can still take place. Even worse, the allocation are made within the check functions and the lambda being inlined such that the optimizer can see the allocation.

But do we even need the optimization barrier? How could the memory check be optimized away?

[not a change request] If we vary interleaving as well, we would not be restricted to the target architecture's vector width.



================
Comment at: SingleSource/UnitTests/Vectorizer/CMakeLists.txt:1
+list(APPEND CXXFLAGS -std=c++17)
 llvm_singlesource()
----------------
Can we let cmake select the flag? 

https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html


================
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.
----------------
Something that would have made me understand the purpose easer.


================
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";
----------------
I assume it is not possible for NaNs to appear?


================
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) {
----------------
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.



================
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]);
----------------
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.



================
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);
----------------
[style] No almost-always-auto


================
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);
----------------



================
Comment at: SingleSource/UnitTests/Vectorizer/runtime-checks.cpp:82-83
+
+  for (int i = -200; i <= 200; i++)
+    CheckWithOffset(i);
+}
----------------
Do we really need that many offsets? The trip count is just 100 so everything outside [-100,100] already seems redundant.


================
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) {
----------------



Repository:
  rT test-suite

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

https://reviews.llvm.org/D119121