[PATCH] D63035: LoopDistribute/LAA: Add tests to catch regressions

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

As I understand the language reference <https://llvm.org/docs/LangRef.html#volatile-memory-accesses>, ("The optimizers may change the order of volatile operations relative to non-volatile operations.") the loop can be distributed as long as there all volatile operations stay in the same loop. I this is meant to test the current implementation, can you add a comment to make this clear (such as "TODO: distribution of volatile may be possible under some circumstance, but the current implementation does not touch them")



================
Comment at: test/Transforms/LoopDistribute/basic-with-memchecks.ll:120-125
+; CHECK: {{^}}for.body:
+; CHECK: load i32
+; CHECK: load i32
+; CHECK: load volatile i32
+; CHECK: load i32
+; CHECK: br i1 %exitcond
----------------
This might not sufficiently check that no distribution occured. The `br i1 %exitcond` does match `br i1 %exitcond.ldist1`. Is there a guaranteed that the `ldist` loops are added before the original `%for.body`/`%exitcond` pair?


================
Comment at: test/Transforms/LoopDistribute/basic-with-memchecks.ll:173-174
+
+; Make sure this isn't treated as vectorizable even though the loop is
+; readonly due to the volatile.
+; CHECK-LABEL: @f_with_volatile_readonly_loop(
----------------
[nit] To avoid some ambiguity understanding the sentence: "Make sure the loop is not vectorized due to the volatile, even though it has no memory writes"


================
Comment at: test/Transforms/LoopVectorize/write-only.ll:27
+
+; CHECK-LABEL: @read_mod_write_single_ptr_volatile_store(
+; CHECK-NOT: store <4 x float>
----------------
Add comment about what this is supposed to test? Such as "Ensure that volatile stores are not vectorized"


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

https://reviews.llvm.org/D63035