[PATCH] D21091: AArch64: refactor sysreg handling (new TableGen backend!)

[Ahmed Bougacha via llvm-commits]

ab added a comment.

Another round of nitpicks; this generally looks good to me.

> For the "Code" thing, I decided to promote it from syntactic sugar in TableGen so that we don't need the extra indirection. I've combined the lib/TableGen patch here, but it's separate in my repo and I can easily create a separate review thread if you'd prefer.


Eh, it's fine here (with a separate commit).

> This was actually inspired by trying to make Clang's diagnostics better (specifically warning when a 64-bit register is used for arm_rsr or arm_wsr), so any ideas on how best to share it would be welcome (best I've come up with it putting the AArch64-specific file in include/llvm/Target and rebuilding it from Clang).


I suppose it could go in lib/Support, like the TargetParser ?

> The other nasty hack is DBGDTRTX_EL0 and DBGDTRRX_EL0 which share an encoding but are written differently in MRS/MSR. I just completely hacked around that in InstPrinter, deciding the extra infrastructure for a generic solution wasn't worth implementing.


I agree.


================
Comment at: lib/Target/AArch64/AArch64.td:135-140
@@ -134,2 +134,8 @@
 //===----------------------------------------------------------------------===//
+// Named operands for MRS/MSR/TLBI/...
+//===----------------------------------------------------------------------===//
+
+include "AArch64SystemOperands.td"
+
+//===----------------------------------------------------------------------===//
 // AArch64 Processors supported.
----------------
The file looks independent from the rest of the target; use a separate top-level .td?  CMake might make it tricky though..

================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:32
@@ +31,3 @@
+class SearchableTableEmitter {
+private:
+  RecordKeeper &Records;
----------------
Unnecessary private

================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:50-51
@@ +49,4 @@
+  std::string getName(Record *R) {
+    if (R->isValueUnset("Name"))
+      return R->getName();
+    return R->getValueAsString("Name");
----------------
Should this be an error?

================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:59-61
@@ +58,5 @@
+    else if (BitsInit *BI = dyn_cast<BitsInit>(I)) {
+      std::ostringstream OS;
+      OS << "0x" << std::hex << getAsInt(BI);
+      return OS.str();
+    }
----------------
utohexstr() ?

================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:68
@@ +67,3 @@
+    }
+    return "???";
+  }
----------------
Also an error?

================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:93-95
@@ +92,5 @@
+        llvm_unreachable("bitfield too large to search");
+      std::ostringstream OS;
+      OS << "uint" << NumBits << "_t";
+      return OS.str();
+    }
----------------
utostr() and + ?

================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:165
@@ +164,3 @@
+  if (isa<BitsInit>(SearchTable[0].first)) {
+    std::sort(SearchTable.begin(), SearchTable.end(),
+              [this](SearchTableEntry &LHS, SearchTableEntry &RHS) {
----------------
std::stable_sort

================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:229-230
@@ +228,4 @@
+void SearchableTableEmitter::emitLookupDeclaration(StringRef Name,
+                                                  StringRef Field, Init *I,
+                                                  raw_ostream &OS) {
+  bool IsIntegral = isa<BitsInit>(I);
----------------
format/alignment (here and elsewhere)


Repository:
  rL LLVM

http://reviews.llvm.org/D21091