Skip to content

[TableGen] Resolve arguments with fields in records #107829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

optimisan
Copy link
Contributor

Previously, instantiating an anonymous record happened in two steps:

  1. Resolve the arguments with a MapResolver
  2. Resolve the fields with a RecordResolver.

In 1, the MapResolver cannot resolve references to fields and in 2 the RecordResolver cannot resolve references to arguments. This was leading to incomplete resolution in llvm/test/TableGen/record-recursion.td (simplified test-case from #91050 ) like so:

  1. MapResolver fails to resolve the if condition since it references the field v.
  2. Resolving SumTillN<!sub(n, 1)>.ret) fails as it references the argument n and the RecordResolver doesn't know about it.

This patch merges the two steps into one where arguments and fields are resolved by the RecordResolver itself.

@optimisan optimisan marked this pull request as ready for review September 9, 2024 09:35

public:
explicit RecordResolver(Record &R) : Resolver(&R) {}
explicit RecordResolver(Record &R, MapResolver *ArgumentResolver = nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to just make this a required field?

Copy link
Contributor Author

@optimisan optimisan Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be ideal since the two-step resolution happens everywhere (in def R : C<n> as well).

This limitation of inaccessible arguments has forced the !if resolution to forego short-circuiting and resolve the MHS and RHS regardless of the condition. And so, the following case fails with a stack overflow:

class C<int n> {
    int v = n; 
    // !if ignores v==0 and evaluates C<n-1>.ret everytime
    // Ideally should short-circuit to 3
    int ret = !if(!eq(v, 0), 3, C<!sub(n, 1)>.ret);
}
def R : C<0>;

I think Record should contain arguments from each sub-class along with their fields as usual. Will see how that goes. With that, this patch would not be necessary since Record itself would have access to its arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TableGen] llvm-tblgen crash error when using class as subroutine with recursion
2 participants