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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

Other than my request for a FIXME, I'm happy with the patch. LGTM!



================
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
 
----------------
kmclaughlin wrote:
> 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.
Fair enough, thanks for explaining. Can you just add a FIXME above the `cl::opt` for `-enable-strict-reductions` that this flag reverses the default behaviour we have now when hints are passed?


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

https://reviews.llvm.org/D101836