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

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/utils/TableGen/SubtargetEmitter.cpp:1816
+    const bool IsBool = (Value == "false" || Value == "true") &&
+                        !StringRef(Attribute).contains('[');
+    if (!IsBool)
----------------
A question: Why we need to check `[` here?


================
Comment at: llvm/utils/TableGen/SubtargetEmitter.cpp:1817
+                        !StringRef(Attribute).contains('[');
+    if (!IsBool)
+      continue;
----------------
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(); }
```


================
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";
+
----------------
I'm afraid this assumption is not correct. The default value is orthogonal to the value set by a feature.


================
Comment at: llvm/utils/TableGen/SubtargetEmitter.cpp:1826
+
+    OS << "GET_SUBTARGETINFO_MACRO(" << Attribute << ", " << Default << ", "
+       << Getter << ")\n";
----------------
The comments is missing is the generated header file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120906