[PATCH] D151842: [TableGen] Add !setdagarg and !setdagname

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added a comment.

I wonder if it might make sense to allow `!setdagname` to take a string name argument, like `!setdagarg` and `!getdagarg` do?

Obviously, it makes no sense for `!getdagname`, because if you're trying to find the name of the argument called "foo", you already know the answer is "foo" (unless there isn't one at all, I suppose). But I can imagine that you might want to rename the "foo" argument to "bar", without first having to find out where it is.

If you abstract out the "look up by index or name" functionality as I suggest, then this wouldn't even be difficult to implement – just add another call to the same function.



================
Comment at: llvm/docs/TableGen/ProgRef.rst:1825
+``!setdagarg(``\ *dag*\ ``,``\ *key*\ ``,``\ *arg*\ ``)``
+    This operator produces a DAG node with the same operator as *dag*, but
+    replacing the value of the argument, specified by the *key*, which is
----------------
Perhaps also mention that it has the same argument list apart from your one change.


================
Comment at: llvm/docs/TableGen/ProgRef.rst:1826
+    This operator produces a DAG node with the same operator as *dag*, but
+    replacing the value of the argument, specified by the *key*, which is
+    either an integer index or a string name, with *arg*.
----------------
That comma is not standard English punctuation. It's better to remove it.


================
Comment at: llvm/docs/TableGen/ProgRef.rst:1831
+    This operator produces a DAG node with the same operator as *dag*, but
+    replacing the name of the argument, specified by the *index* ,with *name*.
+
----------------
Same with this one. Also, later in the line, there's a mis-spaced comma (space before it, not after it).


================
Comment at: llvm/lib/TableGen/Record.cpp:1716-1748
+      // Accessor by index
+      if (IntInit *Idx = dyn_cast<IntInit>(MHS)) {
+        int64_t Pos = Idx->getValue();
+        if (Pos < 0) {
+          // The index is negative.
+          PrintFatalError(CurRec->getLoc(), Twine("!setdagarg index ") +
+                                                std::to_string(Pos) +
----------------
This all looks very similar to the code in !getdagarg. Instead of cloning and hacking it, perhaps it would be better to factor out the code that maps `Idx` to an integer position, and call it from both cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151842