[PATCH] D129560: [AArch64] Add target hook for preferPredicateOverEpilogue

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:3034-3042
+  if (!(TailFoldingKindLoc & TailFoldingKind::TFReductions) &&
+      LVL->getReductionVars().size())
+    return false;
+
+  if (!(TailFoldingKindLoc & TailFoldingKind::TFRecurrences) &&
+      LVL->getFirstOrderRecurrences().size())
+    return false;
----------------
paulwalker-arm wrote:
> david-arm wrote:
> > paulwalker-arm wrote:
> > > I don't think this works now we have the expanded bitfield.  I think you need logic like:
> > > ```
> > > TailFoldingKind Required = 0;
> > > if (LVL->getReductionVars().size())
> > >   Required.add(TailFoldingKind::TFReductions)
> > > if (LVL->getReductionVars().size())
> > >   Required.add(TailFoldingKind::TFRecurrences)
> > > if (!Required)
> > >   Required.add(TailFoldingKind::TFSimple)
> > > 
> > > return TailFoldingKindLoc & Required
> > > ```
> > Hmm, the existing code I have does work since the tests I added all pass so I don't believe there is a bug. With the existing version if the user didn't request reductions and the loop contains at least one reduction then `!(TailFoldingKindLoc & TailFoldingKind::TFReductions) && LVL->getReductionVars().size()` is true and so we return false. This is the same as your suggested code because `Required != TailFoldingKindLoc` in that case. We then return true at the end if either: a) the loop is 'simple', or b) the user has explicitly permitted tail-folding with reductions and/or recurrences.
> > 
> > Having said that, I'm happy to give your suggestion a try if you think it reads better!
> If `TailFoldingKind::TFReductions` is set and the loop contains no reduction then tail folding will be enabled regardless of whether `TailFoldingKind::TFSimple` is set.  Which I think is wrong?
Oh I see what you mean now. In which case this patch is missing a test. :)


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

https://reviews.llvm.org/D129560