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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion llvm/include/llvm/TableGen/Record.h
Original file line number Diff line number Diff line change
Expand Up @@ -2289,9 +2289,12 @@ class RecordResolver final : public Resolver {
DenseMap<Init *, Init *> Cache;
SmallVector<Init *, 4> Stack;
Init *Name = nullptr;
// Cache is for fields, ArgumentResolver is for arguments.
MapResolver *ArgumentResolver;

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.

: Resolver(&R), ArgumentResolver(ArgumentResolver) {}

void setName(Init *NewName) { Name = NewName; }

Expand Down
12 changes: 9 additions & 3 deletions llvm/lib/TableGen/Record.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2321,8 +2321,6 @@ DefInit *VarDefInit::instantiate() {
R.set(Arg->getName(), Arg->getValue());
}

NewRec->resolveReferences(R);

// Add superclasses.
ArrayRef<std::pair<Record *, SMRange>> SCs = Class->getSuperClasses();
for (const auto &SCPair : SCs)
Expand All @@ -2332,7 +2330,10 @@ DefInit *VarDefInit::instantiate() {
Class, SMRange(Class->getLoc().back(), Class->getLoc().back()));

// Resolve internal references and store in record keeper
NewRec->resolveReferences();
RecordResolver RR(*NewRec, &R);
RR.setFinal(true);
NewRec->resolveReferences(RR);

Records.addDef(std::move(NewRecOwner));

// Check the assertions.
Expand Down Expand Up @@ -3348,6 +3349,11 @@ Init *RecordResolver::resolve(Init *VarName) {
if (Val)
return Val;

if (ArgumentResolver && (Val = ArgumentResolver->resolve(VarName))) {
Cache[VarName] = Val;
return Val;
}

if (llvm::is_contained(Stack, VarName))
return nullptr; // prevent infinite recursion

Expand Down
11 changes: 11 additions & 0 deletions llvm/test/TableGen/record-recursion.td
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// RUN: llvm-tblgen %s | FileCheck %s

class SumTillN<int n> {
int v = !add(n, 0);
int ret = !if(!gt(v, 0), !add(v, SumTillN<!sub(n, 1)>.ret), v);
}

// CHECK: def SumTill4
// CHECK-NEXT: int v = 3;
// CHECK-NEXT: int ret = 6;
def SumTill4 : SumTillN<3>;
Loading