[PATCH] D120906: [ARM][AArch64] generate subtarget feature flags [NFC]

[Tomas Matheson via Phabricator via llvm-commits]

tmatheson added inline comments.


================
Comment at: llvm/utils/TableGen/SubtargetEmitter.cpp:1816
+    const bool IsBool = (Value == "false" || Value == "true") &&
+                        !StringRef(Attribute).contains('[');
+    if (!IsBool)
----------------
skan wrote:
> A question: Why we need to check `[` here?
This is to exclude BitVector fields which have attributes like `ReserveRegister[i]`


================
Comment at: llvm/utils/TableGen/SubtargetEmitter.cpp:1817
+                        !StringRef(Attribute).contains('[');
+    if (!IsBool)
+      continue;
----------------
skan wrote:
> Just checking `IsBool` could not handle some X86 features ...
> ```
> bool hasCMov() const { return HasCMov || X86SSELevel >= SSE1 || is64Bit(); }
> bool useAA() const override { return UseAA; }
> bool hasLAHFSAHF() const { return HasLAHFSAHF64 || !is64Bit(); }
> ```
IMO these functions should be renamed to make it clear they do more than just check a feature field.


================
Comment at: llvm/utils/TableGen/SubtargetEmitter.cpp:1821
+    // Some features default to true, with values set to false if enabled.
+    const char *Default = Value == "false" ? "true" : "false";
+
----------------
skan wrote:
> I'm afraid this assumption is not correct. The default value is orthogonal to the value set by a feature.
It is correct for ARM/AArch64, which I would argue is sensible.


================
Comment at: llvm/utils/TableGen/SubtargetEmitter.cpp:1826
+
+    OS << "GET_SUBTARGETINFO_MACRO(" << Attribute << ", " << Default << ", "
+       << Getter << ")\n";
----------------
skan wrote:
> The comments is missing is the generated header file.
The comments have been moved to the SubtargetFeature declarations in the tablegen files. It would be better if they were available on the getter/fields but that can't be accomplished using a macro.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120906