[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:71-75
+      CurrentAction = CC->getName().str();
+      // Call upon the creation of a map entry from the void!
+      // We want an entry in AssignedRegsMap for every action, even if that
+      // entry is empty.
+      AssignedRegsMap[CurrentAction] = {};
----------------
Can we sink these two statements
```
CurrentAction = CC->getName().str();
AssignedRegsMap[CurrentAction] = {};
```
into the definition of `EmitCallingConv`?


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:106-108
+                                 std::string Name =
+                                     Class.first->getNameInitAsString();
+                                 return StringRef(Name).startswith("CCIfSwift");
----------------
```
StringRef Name = Class.first->getNameInitAsString();
return Name.startswith("CCIfSwift");
```
Not sure if that would fit on one line if it was a single statement?


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:332-353
+restart:
+  for (EntryTy &Entry : DelegateToMap) {
+    StringRef RegName = Entry.first();
+    std::set<std::string> &Registers = Entry.second;
+    if (!Registers.empty())
+      continue;
+
----------------
i.e. I _think_ you could avoid the goto here with:

```
for (auto *It = DelgateToMap.begin(), E = DelegateToMap.end(); Entry != E; ++E) {
  StringRef Regname = It->first();
  ...
  DelegateToMap.erase(RegName.str());
}
```
if DelegateToMap was a std::map.
https://stackoverflow.com/a/54004916
(Iterator invalidation being the weak point to range-for)


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:391-392
+    ListSeparator LS;
+    for (const std::string &Reg : Registers)
+      O << LS << Reg;
+
----------------
If `Registers` is empty, do we need to insert an explicit `0` like we do for the non-swift AssignedRegsMap? Or do no work if `RegName` is empty?

The two cases seem pretty similar otherwise; am curious if they can be DRY'd up?


================
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;
+
----------------
void wrote:
> nickdesaulniers wrote:
> > void wrote:
> > > nickdesaulniers wrote:
> > > > 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?
> > > I *really* hate adding headers that aren't necessary. Is this some kind of style guide violation?
> > It's definitely a google3 style violation.
> > https://google.github.io/styleguide/cppguide.html#Include_What_You_Use
> > 
> > Looks like it's not though for LLVM:
> > https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible
> > > You must include all of the header files that you are using — you can include them either directly or indirectly through another header file.
> > There's also https://llvm.org/docs/CodingStandards.html#include-style, but that doesn't seem to make a call either way either.
> > 
> > OK, I'll let that slide, but the comment still stands about using containers from ADT. We have a StringMap and StringSet
> > https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h
> > https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringset-h
> > Looks like it's not though for LLVM:
> 
> Right. LLVM does things very differently from Google.
> OK, I'll let that slide, but the comment still stands about using containers from ADT. We have a StringMap and StringSet

Though, looking at that goto...

https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h

>> StringMap iteration order, however, is not guaranteed to be deterministic, so any uses which require that should instead use a std::map.

We iterate all three of these; I'm guessing it might be nice to do so deterministically? Otherwise rebuilding LLVM might produce a different order of these registers?

Further, I think std::map provides iterator invalidation guarantees on erasure that might help you avoid `goto` if you used the old fashioned non-range for loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125421