This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] Add !getdagarg and !getdagname
ClosedPublic

Authored by hliao on May 26 2023, 6:24 PM.

Details

Summary
  • This patch proposes to add !getdagarg and !getdagnames bang operators as the inverse operation of !dag. They allow us to examine arguments of a given dag.

Diff Detail

Event Timeline

hliao created this revision.May 26 2023, 6:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 6:24 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
hliao requested review of this revision.May 26 2023, 6:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 6:24 PM
hliao updated this revision to Diff 526443.May 29 2023, 8:01 AM

Rebase and ping for review

It's not quite clear whether you mean this patch to be an alternative to D151457, or to go together with it. The two changes would clearly conflict, but one of your comments in the other change (https://reviews.llvm.org/D151457#4374848) made it sound as if you meant to do both?

If they're alternatives, then I prefer this one, because I love the extra feature of looking up a dag argument by its name instead of its index. I can see that being really useful.

llvm/lib/TableGen/Record.cpp
1301

This error message schema leads to the slightly weird error seen in your test:

error: !getdagarg index is out of range 0...3: 3

I'd normally interpret the notation "0...3" as being inclusive at both ends, so it's confusing to be told that 3 is not in the range 0...3.

To avoid this wording difficulty, perhaps it's better to break up the error so that a negative index gets its own message (it's illegal no matter how many arguments the dag has), and a positive one gets a message like "!getdagarg index 3 out of range (dag has 3 arguments)"?

llvm/lib/TableGen/TGLexer.h
53

It isn't clear why this section had to be reformatted at all in this patch, but if you are going to put each enum entry on its own line, you could at least break up the comments in the same way so that each one has a comment with its own symbol.

llvm/test/TableGen/getsetop.td
99

Needs some more tests:

  • total mismatch of types, e.g. !getdagarg<int> applied to an argument that's a string, or !getdagarg<Foo> applied to a record type that's not an instance of class Foo at all.
  • demonstrate retrieving a type by its superclass but not subclass
hliao added a comment.May 30 2023, 9:11 AM

It's not quite clear whether you mean this patch to be an alternative to D151457, or to go together with it. The two changes would clearly conflict, but one of your comments in the other change (https://reviews.llvm.org/D151457#4374848) made it sound as if you meant to do both?

If they're alternatives, then I prefer this one, because I love the extra feature of looking up a dag argument by its name instead of its index. I can see that being really useful.

Originally, I thought to provide this element access functionality as complementary to the !getdagargs. But, I realized later that the element accessor is more convenient to be used. Also, !getdagargs could be implemented through !getdagarg with the help of !forall and !size, even though a little bit verbose. That makes the D151457 redundant. I prefer this one and want to check your comment and suggestion. I will cancel D151457 once we all agree on that.

hliao added inline comments.May 30 2023, 9:14 AM
llvm/lib/TableGen/TGLexer.h
53

The pre-merge check fails now once clang-format generates different changes from my original change cause we add XDagGetArg and XDagGetName. I could revert that part of change or make such formatting NFC change in advance before this patch.

hliao updated this revision to Diff 526720.May 30 2023, 11:04 AM

More tests

hliao updated this revision to Diff 526767.May 30 2023, 1:22 PM

Revise the out-of-range error message. Now, the range is from 0 to (numargs-1).

hliao marked an inline comment as done.May 30 2023, 1:22 PM

Thanks for the updates. I only have two nitpicks left now.

llvm/lib/TableGen/Record.cpp
1301

I see that you've done a simpler thing than I suggested, by just writing 0...(n-1) in place of 0...n.

But this leads to a bad error message when n=0. Remember that a dag node need not have any arguments at all:

<stdin>:1:14: error: !getdagarg index is out of range 0...4294967295: 0
def foo; def test { dag x = (foo); int y = !getdagarg<int>(x, 0); }
             ^
llvm/lib/TableGen/TGLexer.h
53

I don't mind whether you do it in this commit, or separately, or not at all. But if you do it, please convert line pairs like this

minus,
plus, // - +

into a form where the comments make sense:

minus, // -
plus, // +
hliao updated this revision to Diff 527025.May 31 2023, 7:12 AM

Revise error message following reviewer's comment

hliao marked 2 inline comments as done.May 31 2023, 7:12 AM
simon_tatham accepted this revision.May 31 2023, 7:33 AM

LGTM. Thanks for the fixes!

This revision is now accepted and ready to land.May 31 2023, 7:33 AM
This revision was landed with ongoing or failed builds.May 31 2023, 7:54 AM
This revision was automatically updated to reflect the committed changes.