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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for refactoring this @david-arm, this seems like a step in the right direction.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:44
+class TailFoldingOption {
 private:
+  uint8_t InitialBits, EnableBits, DisableBits;
----------------
nit: All members of a class are private by default, so you can remove this.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:45-46
 private:
-  uint8_t Bits = 0; // Currently defaults to disabled.
+  uint8_t InitialBits, EnableBits, DisableBits;
+  bool NeedsDefault;
+
----------------
Can you add a comment explaining why these are required?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:48-50
+  void setInitialBits(uint8_t Bits) { InitialBits = Bits; }
+
+  void setNeedsDefault(void) { NeedsDefault = true; }
----------------
Can you make this part of the constructor? e.g. 

  TailFoldingOption(bool NeedsDefault = true) { ... }
  TailFoldingOption(uint8_t InitBits) { ... }

When you make `NeedsDefault = true` the default, then you can do away with:

    if (Val.empty()) {
      setNeedsDefault();
      return;
    }

in `operator=`


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:87
     StringRef(Val).split(TailFoldTypes, '+', -1, false);
+    int Count = 0;
     for (auto TailFoldType : TailFoldTypes) {
----------------
Could you split the parsing in two steps, such that you first parse only the "disabled|all|simple|default", and only then start iterating the individual toggles, e.g.

  if (TailFoldTypes[0] == "disabled")
    ...
  else if (TailFoldTypes[0] == "all")
    ...
  else if (TailFoldTypes[0] == "simple")
    ...
  else if (TailFoldTypes[0] == "default")
    ...
  else
    report_fatal_error(..)
  
  for (auto TailFoldType : ArrayRef<StringRef>(TailFoldTypes).drop_front()) {
    if (TailFoldType == "reductions")
      setEnableBit(TFReductions);
    ...
  } 



================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:541
+///   TFAll:         All
+enum TailFoldingOpts : uint8_t {
+  TFDisabled = 0x00,
----------------
nit: Can you put this in a namespace, e.g.

  namespace TailFoldingOpts {
    enum TailFoldingOptsImpl : uint8_t {
      Disabled = 0x00;
      Simple = 0x01;
      ...
    };
  };

That makes it a bit easier to use these, e.g.

  TailFoldingOpts::Disabled
  TailFoldingOpts::Simple
  ...


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

https://reviews.llvm.org/D149659