- This patch proposes to add !setdagarg and !setdagname bang operators to produce a new DAG node after replacing the specified argument value/name from the given input DAG node. E.g., !setdagarg((foo 1, 2), 0, "x") produces (foo "x", 2) and !setdagname((foo 1:$a, 2:$b), 1, "c") produces (foo 1:$a, 2:$c).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I don't understand the descriptions of these new operators. I don't think you mean that it replaces the argument value with the key.
As the naming suggests, it sets the argument name/value by the specified argument by either index or name (for argument value only.)
As the naming suggests, it sets the argument name/value by the specified argument by either index or name (for argument value only.)
I think Paul's point is that the use of English is confusing. "Replace X by Y" is normally taken to mean the same thing as "Replace X with Y", and both of them mean putting Y where X used to be. So "replacing the argument name by the specified index" suggests that the function will put an integer index where a name used to be, which makes no sense.
It looks as if what you actually mean is something like:
!setdagarg(dag, key, arg) produces a DAG node with the same operator and argument list as dag, except that the argument identified by key (which can be a string name or an integer index) has its value replaced by arg.
!setdagname(dag, index, name) produces a DAG node with the same operator and argument list as dag, except that the argument identified by index (which must be an integer index) has its name field replaced by name.
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.
llvm/docs/TableGen/ProgRef.rst | ||
---|---|---|
1825 | Perhaps also mention that it has the same argument list apart from your one change. | |
1826 | That comma is not standard English punctuation. It's better to remove it. | |
1831 | Same with this one. Also, later in the line, there's a mis-spaced comma (space before it, not after it). | |
llvm/lib/TableGen/Record.cpp | ||
1718–1750 | 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? |
not an issue of implementation, the only case where to use`!setdagname` by argument name should be renaming an argument. However, I would add that for completeness. Thanks for the suggestion.
Revise following reviewer's comments. Now, !setdagname also accepts name to lookup arguments
llvm/lib/TableGen/Record.cpp | ||
---|---|---|
1327–1328 | I'd intended to suggest that all of this code, not just the name lookup, could be factored out into a function that took an Init * and returned an optional<unsigned> argument number. It would handle both the integer and string cases, and construct an appropriate error message on failure (perhaps also given the name of the bang-operator it was being used by). Then you'd only need one if statement in this case handler, and also for the ones in SETDAGARG and SETDAGNAME. |
Thanks, that looks much nicer!
There are three nitpicks still unaddressed in the documentation change in ProgRef.rst, but after that, I'm happy.
llvm/docs/TableGen/ProgRef.rst | ||
---|---|---|
1832 | Add period after *arg* |
Perhaps also mention that it has the same argument list apart from your one change.