[PATCH] D149659: [AArch64][NFC] Refactor the tail-folding option
Sander de Smalen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 12 03:04:03 PDT 2023
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
More information about the llvm-commits
mailing list