[PATCH] D130618: [AArch64][LoopVectorize] Enable tail-folding by default for SVE

[David Sherwood via Phabricator via llvm-commits]

david-arm marked 4 inline comments as done.
david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:62-63
+        add(TFAll);
       else if (TailFoldType == "default")
-        Bits = 0; // Currently defaults to never tail-folding.
+        NeedsDefault = true;
       else if (TailFoldType == "simple")
----------------
paulwalker-arm wrote:
> sdesmalen wrote:
> > This class seems to be mixing two concepts:
> > * Features that the loop requires for tail folding (i.e. recurrences/reductions/anything-else).
> > * What the target wants as the default and the parsing-logic for user-visible toggles.
> > 
> > This makes it quite tricky to follow the logic, especially the need for both `NeedsDefault` + `DefaultBits` and Add/RemoveBits`.
> > 
> > Perhaps it would make more sense to have two classes:
> > 
> >   class TailFoldingKind;    // This encodes which features the loop requires
> >   class TailFoldingOption;  // This encodes the default (which itself will be a TailFoldingKind object), and has logic to parse strings.
> > 
> > Then you can add a method such as `TailFoldingKind TailFoldingOption::getSupportedMode() const { .. }` that you can use to query if the target's mode satisfies the required TailFoldingKind from the Loop.
> This doesn't quite work because you've lost the position of the default value compared to the explicitly enabled/disabled options.  It's almost as if you want to defer parsing until when the data is required and then have something like `TailFoldingKindLoc.parseWithDefault(getTailFoldingDefaultForCPU()) `.  I think if you can do this, many of the other changes in the patch become necessary.
I've tried to separate out the two concepts into two different classes, but I've kept the DefaultBits as part of the new TailFoldingOption class.


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

https://reviews.llvm.org/D130618