[PATCH] Refactor identification of reductions and expose them as utility functions

Renato Golin renato.golin at linaro.org
Thu Apr 16 05:47:58 PDT 2015


Hi Karthik,

This is looking a lot better, thanks!

I have two main comment inline, and I'm assuming it passes all check-all tests and maybe the test-suite in at least one major platform.

Also, please remember to add "NFC" (meaning non-functional-change) to your commit message, since this is just the refactoring part of your patch.

Finally, it'd be good to see how your pass looks like with this change, to make sure we won't need additional refactorings to this piece of code. Can you add a new diff to the loop interchange pass that assumes these changes are applied?

cheers,
--renato


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:46
@@ +45,3 @@
+/// This enum represents the kinds of reductions that we support.
+enum ReductionKind {
+  RK_NoReduction,   ///< Not a reduction.
----------------
These enums now have llvm namespace. While RK clearly means "reduction kind" in the context of the loop vectorizer, it doesn't in a general context. I'm not sure what's best in this case, to add a namespace or move it inside some of the structs below. @jmolloy?

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:194
@@ +193,3 @@
+
+  // This instruction is allowed to have out-of-loop users.
+
----------------
Change this comment here to state that the exit instruction will be saved as you create the reduction descriptor below.

http://reviews.llvm.org/D9046

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list