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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added inline comments.
This revision is now accepted and ready to land.


================
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] = {};
----------------
void wrote:
> 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?
Whatever.  Code style is an inseparable part of code review.  Otherwise it makes my review approval just a rubber stamp. And if it's just a rubber stamp, why even waste my time asking for review?


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:106-108
+                                 std::string Name =
+                                     Class.first->getNameInitAsString();
+                                 return StringRef(Name).startswith("CCIfSwift");
----------------
void wrote:
> 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.
I still think the pair of expressions need not make any mention of `std::string` if you just construct a `StringRef` from the get go.


================
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;
+
----------------
void wrote:
> 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.
I still think this could be rewritten to avoid the use of `goto`.


================
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:
> > > > 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).
What about my concern about deterministic iteration order?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125421