[PATCH] D101836: [LoopVectorize] Enable strict reductions when allowReordering() returns false

[Kerry McLaughlin via Phabricator via llvm-commits]

kmclaughlin added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/strict-fadd.ll:3
+; RUN: opt < %s -loop-vectorize -mtriple aarch64-unknown-linux-gnu -enable-strict-reductions=false -hints-allow-reordering=true  -S 2>%t | FileCheck %s --check-prefix=CHECK-UNORDERED
+; RUN: opt < %s -loop-vectorize -mtriple aarch64-unknown-linux-gnu -enable-strict-reductions=true  -hints-allow-reordering=false -S 2>%t | FileCheck %s --check-prefix=CHECK-ORDERED
 
----------------
sdesmalen wrote:
> Can you also add a RUN line for `-enable-strict-reductions=true  -hints-allow-reordering=true` (which I think can reuse prefix CHECK-UNORDERED)
Hi @sdesmalen, I haven't added another RUN line for `-enable-strict-reductions=true -hints-allow-reordering=true` as this will cause failures with the fadd_multiple test.
For most of the tests, the output where both flags are true will match the CHECK-ORDERED lines, since we will always use strict reductions where possible if this flag is set. For fadd_multiple, we cannot use strict reductions and so the value of `-hints-allow-reordering` will change whether or not the test vectorizes.
As we discussed previously, I will follow this up with a patch which ensures we only choose strict reductions if we do not allow reordering. At this point I can add a RUN line as you've suggested and reuse the `CHECK-UNORDERED` prefix.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/strict-fadd.ll:621
+  %exitcond.not = icmp eq i64 %iv.next, %N
+  br i1 %exitcond.not, label %for.end, label %for.body
+
----------------
david-arm wrote:
> david-arm wrote:
> > Are these new tests missing hints that the other tests seem to use? I just wondered if it was better to be consistent here that's all. The reason I mention this is because I was expecting the UNORDERED case to vectorise due to the `-hints-allow-reordering=true` flag.
> I think I see now @kmclaughlin - you're testing the productisation of `-enable-strict-reductions` so you were adding some tests deliberately without hints, which also makes sense. In this case I'd also be happy if you left these tests as they are and just added some comments explaining why we expect the CHECK-UNORDERED case to not vectorise.
Hi @david-arm, I've added a comment above these tests to explain why the CHECK-UNORDERED case shouldn't vectorize.


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

https://reviews.llvm.org/D101836