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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks for taking a look. I will update this soon...



================
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:
> 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?
> 
You read my mind. I had it called preferInLoopReductionSelect originally, but I changed it because it is really a different concept to the inloop reductions. Inloop reductions are about placing a vecreduce in the loops as opposed to after it. This patch is about being able to transform `add; select` into a predicated `add`, which can make the select more efficient in the loop.

Hopefully that would make it useful in different architectures too, if they have the ability to predicate the add.


================
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;
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > 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?
> > 
> You read my mind. I had it called preferInLoopReductionSelect originally, but I changed it because it is really a different concept to the inloop reductions. Inloop reductions are about placing a vecreduce in the loops as opposed to after it. This patch is about being able to transform `add; select` into a predicated `add`, which can make the select more efficient in the loop.
> 
> Hopefully that would make it useful in different architectures too, if they have the ability to predicate the add.
Yeah. I was trying to avoid having to add an option for it, and it wouldn't do anything without a target hook. I'll add one though and move the TTI part out.


================
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");
----------------
SjoerdMeijer wrote:
> Is this an unrelated change (the deletions)?
The idea is that we can now handle any reductions in tail predicated loops, not just integers. But we still have an option to disable them.  I have to check that they actually do OK all the time though. I know predicated VFMA was missing until now. I'm not sure if any others are needed...


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

https://reviews.llvm.org/D84741