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

[David Sherwood via Phabricator via llvm-commits]

david-arm marked 3 inline comments as done.
david-arm added a comment.

I've tried to address the various comments on the patch and maintain the existing functionality!



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:48-50
+  void setInitialBits(uint8_t Bits) { InitialBits = Bits; }
+
+  void setNeedsDefault(void) { NeedsDefault = true; }
----------------
sdesmalen wrote:
> 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=`
I don't think this helps because the constructor never gets invoked in any other way except the default `TailFoldingOption()`. The only time this ever gets set to something other than default values is when setting the option value, which occurs in `operator=`.

I've changed this code

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

to now emit an error because if the user has supplied the option "-msve-tail-folding=" I think we should treat that as user error.


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:541
+///   TFAll:         All
+enum TailFoldingOpts : uint8_t {
+  TFDisabled = 0x00,
----------------
paulwalker-arm wrote:
> sdesmalen wrote:
> > 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
> >   ...
> The modern way to do this is via an enum class?, i.e.
> ```
>   enum class TailFoldingOpt : uint8_t {
>     Disabled = 0x00;
>     Simple = 0x01;
>     ...
>   };
> ```
This turns out to be a huge pain because it prevents casting between the type and an integer. I tried also using LLVM_DECLARE_ENUM_AS_BITMASK, which didn't stop the compiler complaining. :)


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

https://reviews.llvm.org/D149659