[PATCH] D125421: [TableGen] Add generation of argument register lists

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:29
+  std::map<std::string, std::set<std::string>> AssignedSwiftRegsMap;
+  std::map<std::string, std::set<std::string>> DelegateToMap;
+
----------------
I know the current code is using `std::string` without explicitly including `<string>`, but would you mind please cleaning that up here (in this patch)?

Also curious if we should be preferring containers from ADT?


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:73
+      CurrentAction = CC->getName().str();
+      (void)AssignedRegsMap[CurrentAction];
       EmitCallingConv(CC, O);
----------------
What is this code doing? Trying to create a key without an subsequent entry? Suspicious.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:102-107
+    SwiftAction = llvm::any_of(Action->getSuperClasses(),
+                               [](const std::pair<Record *, SMRange> &Class) {
+                                 std::string Name =
+                                     Class.first->getNameInitAsString();
+                                 return StringRef(Name).startswith("CCIfSwift");
+                               });
----------------
Can you clarify in the commit message why CCIfSwift needs special/distinct handling? Swift seems to be introducing a bit of complexity to the feature, and I don't yet understand precisely why.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:327
+void CallingConvEmitter::EmitArgRegisterLists(raw_ostream &O) {
+  using EntryTy = std::pair<std::string, std::set<std::string>>;
+
----------------
This is a common case for just using `auto &` in your range for loops.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:331-378
+  for (EntryTy Entry : DelegateToMap) {
+    if (!Entry.second.empty())
+      continue;
+
+    for (EntryTy Entry2 : DelegateToMap) {
+      if (Entry2.second.find(Entry.first) != Entry2.second.end()) {
+        AssignedRegsMap[Entry2.first].insert(
----------------
All of this code would be more readable if you took references to `Entry.first` and `Entry.second` and gave them meaningful identifiers. C++ code heavy on map entries makes it a PITA to keep track without looking at the type definition of the map what types the keys and values even are.  For example:

```
for (auto &Entry : AssignedRegsMap) {
  StringRef Regname = Entry.first;
  std::set<std::string> &Registers = Entry.second;

  if (Regname.empty())
    continue;

  O << "const MCRegister " << Regname << "_ArgRegs[] = { ";
  
  if (Registers.empty()) {
    ...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125421