[PATCH] D117450: [LoopInterchange] Support loop interchange with floating point reductions

[Michael Kruse via Phabricator via llvm-commits]

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

In D117450#3292701 <https://reviews.llvm.org/D117450#3292701>, @congzhe wrote:

> Thanks, I've updated the patch accordingly. I used `RD.getExactFPMathInst()` instead, since `needsExactFPMath()` does not belong to `RecurrenceDescriptor`.

Correct, unfortunately. I found `needsExactFPMath` more descriptive. If you agree, you could a definition just like `InstDesc` has.

> I do agree that it would be more fine-grained using `RD.getFastMathFlags()` and check against flags. I see SLP checks `noNaNs()` for `FMax/Fmin`, and loop vectorizer relies on `getExactFPMathInst()` and additional hints. I'm thinking maybe at the moment we could just rely on `RD.getExactFPMathInst()` to be safe, and we could make it finer-grained later.

Agreed.

LGTM. Please address the nitpicks before committing.

Thank you for your contribution!



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:885
 }
-
 // We currently support LCSSA PHI nodes in the outer loop exit, if their
----------------
[nit] unrelated whitespace change


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:898
   BasicBlock *LoopNestExit = OuterLoop->getUniqueExitBlock();
-  for (PHINode &PHI : LoopNestExit->phis()) {
-    //  FIXME: We currently are not able to detect floating point reductions
-    //         and have to use floating point PHIs as a proxy to prevent
-    //         interchanging in the presence of floating point reductions.
-    if (PHI.getType()->isFloatingPointTy())
-      return false;
+  for (PHINode &PHI : LoopNestExit->phis())
     for (unsigned i = 0; i < PHI.getNumIncomingValues(); i++) {
----------------
[style] In my interpretation of the [[ https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements | coding standard ]], the braces should stay here. See the "Use braces for the outer `if` since the nested `for` is braced." example.


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

https://reviews.llvm.org/D117450