[PATCH] D149659: [AArch64][NFC] Refactor the tail-folding option

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:45-46
 private:
-  uint8_t Bits = 0; // Currently defaults to disabled.
+  std::string UnparsedOptionString;
+  SmallVector<StringRef, 4> TailFoldTypes;
 
----------------
There is probably little value in having the copied std::string here with a SmallVector of StringRefs for the comma-separated options. If you're re-parsing the string each time to query what options are set, then you can just as well split the original StringRef `TailFoldingOptionLoc` at that point and make `SmallVector<StringRef> TailFoldTypes` local to the parsing function.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:49
+  uint8_t getBits(uint8_t DefaultBits) const {
+    if (!TailFoldTypes.size())
+      return DefaultBits;
----------------
I'm still not entirely happy with the concept of having to re-parse the full string every time this information is queried, but if that only happens very irregularly, I guess the overhead is only minimal.

Just as a suggestion, one thing you could do is to parse the string for the `reductions/recurrences/reverse` early on and store that in a EnableBits + DisableBits mask, and have this separate from parsing of `simple/disabled/all/default`. Then, at the point of querying the tail folding options, you can do something like:

  uint8_t InitialBits = ... // either all zero, all ones, or SimpleBits or DefaultBits (queried from the target)
  Bits = (InitialBits | EnableBits) ^ DisableBits;

It does mean that it would not be allowed to have a string such as `reductions+default`, because `default/all/disabled/simple` all need to come before the other toggle on/off options. Also, it might make make sense to also make it illegal to write `disabled+simple+all` because only one of those options should be set.

This way the syntax would become:
  (disabled|all|default|simple)[+(reductions|recurrences|reverse|noreductions|norecurrences|noreverse)]*


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:3533
 
-  return (TailFoldingKindLoc & Required) == Required;
+  return TailFoldingOptionLoc.satisfies(ST->getSVETailFoldingDefaultOpts(),
+                                        Required);
----------------
I find this interface a little confusing and wonder if instead you can make `TailFoldingOptionLoc` a `std::string`, and then do:

  TailFoldingOption Opt(TailFoldingOptionLoc, ST->getSVETailFoldingDefaultOpts());
  return Opt.satisfies(Required);

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149659