This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] Improve algorithm for inheriting class template arguments and fields
ClosedPublic

Authored by Paul-C-Anagnostopoulos on Jan 15 2021, 1:29 PM.

Details

Summary

This revision improves the algorithm that processes a class's template arguments when a record inherits from the class.

Currently the algorithm copies all the template argument defaults into the new record's field list, replaces the values that were specified in the class invocation, copies the values into the resolver map, and then deletes the template arguments from the field list. This new algorithm copies the template arguments directly into the resolver map and then replaces the default values with the specified ones.

This revision relies on a previous revision that made it so field definitions can be tagged as template arguments. It paves the way for a future revision that will handle assert statements in class definitions.

Diff Detail

Event Timeline

Paul-C-Anagnostopoulos requested review of this revision.Jan 15 2021, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 1:29 PM

A lot of the changes are in a function named 'AddSubClass', which is a misnomer. It's adding a parent class, not a subclass. Does anyone mind if I rename the function as part of this revision, or should I do it in a separate revision?

craig.topper added inline comments.Jan 15 2021, 4:19 PM
llvm/include/llvm/TableGen/Record.h
1999

This strongly assumes that Key is in the map with a non-null value. if it isn't the map, it will get inserted with a null V.

Maybe better to make this const, and use Map.find(Key) and assert that it doesn't return Map.end().

llvm/lib/TableGen/TGParser.cpp
2612

Please run clang-format

2628

llvm_unreachable

llvm/test/TableGen/self-reference-typeerror.td
12

nvalid?

dblaikie added inline comments.Jan 15 2021, 4:36 PM
llvm/lib/TableGen/TGParser.cpp
2628

Or possibly drop the prior "if" and change it to an assert there.

ie change this:

...
else if (x) {
  ...
} else
  llvm_unreachable(...);

to this:

else {
  assert(x);
  ...
}
llvm/include/llvm/TableGen/Record.h
1999

Good point. In the one use so far, it is in the map, but we don't want to assume that.

llvm/lib/TableGen/TGParser.cpp
2612

I've never run it. Doesn't it reformat the entire file?

llvm/test/TableGen/self-reference-typeerror.td
12

So it doesn't matter whether the first word of the message is capitalized.

craig.topper added inline comments.Jan 15 2021, 4:42 PM
llvm/lib/TableGen/TGParser.cpp
2612

You can run it on only the things changed by your commit by doing this

git diff -U0 --no-color HEAD^ | clang-format-diff.py -i -p1

I think clang-format-diff is in clang/tools/clang-format in your source tree. There's a -binary option to give it the path to the clang-format binary if its not in your path.

https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

I incorporated the various suggestions.

I had no luck running clang-format-diff, but I will work on it for future revisions.

A little bump to get approval.

lattner accepted this revision.Jan 19 2021, 9:23 AM

This looks fine to me in a quick review, but you're the best technical expert here Paul.

llvm/include/llvm/TableGen/Record.h
2003

plz remove commented out code.

llvm/lib/TableGen/TGParser.cpp
2628

Yeah I'd recommend dblaikie's pattern so you can use assert.

This revision is now accepted and ready to land.Jan 19 2021, 9:23 AM
foad added a subscriber: foad.Jan 20 2021, 8:53 AM
foad added inline comments.
llvm/lib/TableGen/TGParser.cpp
2612

If clang/tools/clang-format/git-clang-format is on your path then you can run git clang-format HEAD^ to only reformat lines that are touched by your topmost commit, then git commit -a --amend --no-edit to incorporate the changes that clang-format made if you like.