[PATCH] D84741: [LV][ARM] Allow tail folded reduction selects to remain in the loop

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1261
+  ///
+  /// As opposed to to normal scheme of p = phi (0, a) which allows the select
+  /// to be pulled out of the loop. If the select(.., add, ..) can e predicated
----------------
typo: to to


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1262
+  /// As opposed to to normal scheme of p = phi (0, a) which allows the select
+  /// to be pulled out of the loop. If the select(.., add, ..) can e predicated
+  /// by the target, this can lead to cleaner code generation.
----------------
typo: e -> be


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1264
+  /// by the target, this can lead to cleaner code generation.
+  bool preferPredicatedReductionSelect(unsigned Opcode, Type *Ty,
+                                       ReductionFlags Flags) const;
----------------
Probably best to do the TTI changes separately. Just kind of echoing what people told me last time...but I guess it is more convenient in case of reverts of the LV part for example.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1264
+  /// by the target, this can lead to cleaner code generation.
+  bool preferPredicatedReductionSelect(unsigned Opcode, Type *Ty,
+                                       ReductionFlags Flags) const;
----------------
SjoerdMeijer wrote:
> Probably best to do the TTI changes separately. Just kind of echoing what people told me last time...but I guess it is more convenient in case of reverts of the LV part for example.
Bikeshedding names, so ignore if you don't think it is a good fit.
"InLoop" is used in helpers above. I was thinking if `preferInLoopReductionSelect` would be more consistent and clear.

Then I was wondering the next thing.... can we not simply use `preferInLoopReduction`? Is that not more or less the same, or can it be the same?



================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:1560
 
-  for (auto *I : LiveOuts) {
-    if (!I->getType()->isIntegerTy()) {
-      LLVM_DEBUG(dbgs() << "Don't tail-predicate loop with non-integer "
-                           "live-out value\n");
-      return false;
-    }
-    if (I->getOpcode() != Instruction::Add) {
-      LLVM_DEBUG(dbgs() << "Only add reductions supported\n");
-      return false;
-    }
-    if (IntReductionsDisabled) {
-      LLVM_DEBUG(dbgs() << "Integer add reductions not enabled\n");
-      return false;
-    }
+  if (ReductionsDisabled && !LiveOuts.empty()) {
+    LLVM_DEBUG(dbgs() << "Reductions not enabled\n");
----------------
Is this an unrelated change (the deletions)?


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

https://reviews.llvm.org/D84741