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

[Bill Wendling via Phabricator via llvm-commits]

void 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] = {};
----------------
nickdesaulniers wrote:
> Can we sink these two statements
> ```
> CurrentAction = CC->getName().str();
> AssignedRegsMap[CurrentAction] = {};
> ```
> into the definition of `EmitCallingConv`?
Okay, but could we keep the review to issues about the algorithm and not coding style please?


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


================
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;
+
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > 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)
> > for (auto *It = DelgateToMap.begin(), E = DelegateToMap.end(); Entry != E; ++E) {
> 
> Sorry, only renamed one of the expressions, should have been
> ```
> for (auto *It = DelgateToMap.begin(), E = DelegateToMap.end(); It != E; ++It) {
> ```
See comment about `goto` above.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:391-392
+    ListSeparator LS;
+    for (const std::string &Reg : Registers)
+      O << LS << Reg;
+
----------------
nickdesaulniers wrote:
> 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?
At one point, it was giving me a diagnostic about that type of initialization being a C++17 feature. I guess I changed the code enough that it went away. Done.


================
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;
+
----------------
nickdesaulniers wrote:
> 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.
It's not iterator invalidation that I'm concerned about. It needs to continually go through the list and process the sets in a specific way---that is the sets in 
`AssignedRegsMap` are modified with the newer information. It removes the entry from the map when we no longer need to consider it. There may be ways to get around using a `goto`, but they're much clunkier than simply using one (i.e. using a flag or a convoluted use of a worklist).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125421