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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9660
+
+  return (all_of(LVL.getReductionVars(), [&](auto &Reduction) -> bool {
+    RecurrenceDescriptor RdxDesc = Reduction.second;
----------------
sdesmalen wrote:
> Why do all of the reductions have to be ordered for the LV to be able to vectorize FP math?
> (e.g. if there is an integer reduction and an ordered FP reduction, it would now choose not to vectorize based on this condition)
I guess it might be worth adding a test for this too then, i.e. having a loop with both an integer and FP reduction and ensure we vectorise with ordered reductions.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9783-9784
 
-  if (!Requirements.canVectorizeFPMath(Hints)) {
+  if (!Requirements.canVectorizeFPMath(Hints) &&
+      !canVectorizeOrderedFPMath(LVL, Hints)) {
     ORE->emit([&]() {
----------------
sdesmalen wrote:
> This condition is a bit odd. Should `canVectorizeOrderedFPMath` just contain the call to `Requirements.canVectorizeFPMath` instead? i.e. in order to vectorize ordered FP math, it must at least be able to vectorize FP math.
I think the existing canVectorizeFPMath function is badly named because it actually checks for reordering:

```  bool canVectorizeFPMath(const LoopVectorizeHints &Hints) const {
    return !ExactFPMathInst || Hints.allowReordering();
  }```

so the logic in Kerry's patch is something like this:

1. Is this an exact FP math instruction? If not -> vectorise, else
2. Do hints permit reordering? If so -> vectorise, else
3. Can we vectorise with ordered reductions? If not -> emit remark.

It probably is possible to combine these into a single LoopVectorizationLegality::canVectorizeFPMath function that does all the above, since that class does have access to the Requirements I think.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101836