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

[David Sherwood via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:49
+  uint8_t getBits(uint8_t DefaultBits) const {
+    if (!TailFoldTypes.size())
+      return DefaultBits;
----------------
sdesmalen wrote:
> 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)]*
I've tried to implement your suggestion as best I can!


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:3533
 
-  return (TailFoldingKindLoc & Required) == Required;
+  return TailFoldingOptionLoc.satisfies(ST->getSVETailFoldingDefaultOpts(),
+                                        Required);
----------------
sdesmalen wrote:
> 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?
Now that we no longer parse the string each time, I'm not sure if it still makes sense to refactor this way? I could in theory create a small, new wrapper class with a different name just so I can pass in the option, but wasn't sure if it was worth the extra code?


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

https://reviews.llvm.org/D149659