[PATCH] D151457: [TableGen] Add !getdagargs and !getdagnames

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

Hey @hliao  - thank you for this patch. Apparently we had a similar idea D151702 <https://reviews.llvm.org/D151702> :)

I just have a couple of comments.

Thanks!

Francesco



================
Comment at: llvm/docs/TableGen/ProgRef.rst:1718-1720
+    from the given *dag* node. Due to limitations of the type system, if not
+    all arguments are not of the specified type or derived from the specified
+    base class, ``?`` is returned instead.
----------------
You might want to rephrase this sentence. It can be interpreted as "we return ? as the overall result of the operation if any of the arguments cannot be cast to `<type>`", which is not what you do in your tests, you return `?` only for the argument that cannot be cast to `type`.

Having said that, I'd prefer to avoid this behaviour, and opt for an error, for 2 reasons:

1. If we require the user to specify <type>, we can assume that the user to know what they are doing. Prompting an error would be informative, and could prevent sessions of debugging yablegen failures that might happen quite far away from where the real issue is.
2. At the end of the day, if we decide to return `?` for arguments that cannot be cast, we could avoid to require the type argument in the operator, and just get it from the first element of the list, or programmatically find a common ancestor in the types of the arguments.

In my version I opted for the error: https://reviews.llvm.org/differential/changeset/?ref=4198690

Of course, the whole discussion around this fails if you have uses cases in which you "need" to return `?` (I didn't have those...)


================
Comment at: llvm/docs/TableGen/ProgRef.rst:1722
+
+``!getdagnames<``\ *type*\ ``>(``\ *dag*\ ``)``
+    This operator produces the list of argument names from the given *dag*
----------------
Why do you need to specify `<type>` in here? Isn't it obvious a list of strings will be returned? Or do we get issues with those arguments whose name is unset and need to be returned as `?`.


================
Comment at: llvm/lib/TableGen/TGLexer.h:38
   // Tokens with no info.
-    minus, plus,        // - +
-    l_square, r_square, // [ ]
-    l_brace, r_brace,   // { }
-    l_paren, r_paren,   // ( )
-    less, greater,      // < >
-    colon, semi,        // : ;
-    comma, dot,         // , .
-    equal, question,    // = ?
-    paste,              // #
-    dotdotdot,          // ...
+  minus,
+  plus, // - +
----------------
Do you need to `clang-format` these file? I preferred the original formatting. Please consider reverting back to the original format, just add the two new enums for the operators you have added in the list for bang operators.


================
Comment at: llvm/test/TableGen/getsetop.td:72
+
+  dag in1 = (foo 1:$a, 2:$b, 3:$c);
+  // CHECK: list<int> in1Args = [1, 2, 3];
----------------
Would it make sense to have these tests in a separate file, specifically for the two new operators?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151457