This is an archive of the discontinued LLVM Phabricator instance.

TableGen: Generalize record types to fix typeIsConvertibleTo et al.
ClosedPublic

Authored by nhaehnle on Feb 23 2018, 7:54 AM.

Details

Summary

Allow RecordRecTy to represent the type "subtype of N superclasses",
where N may be zero. Furthermore, generate RecordRecTy instances only
with actual classes in the list.

Keeping track of multiple superclasses is required to resolve the type
of a list correctly in some cases. The old code relied on the incorrect
behavior of typeIsConvertibleTo, and an earlier version of this change
relied on a modified ordering of superclasses (it was committed in
r325884 and then reverted because unfortunately some of clang-tblgen's
backends depend on the ordering).

Previously, the DefInit for each Record would have a RecordRecTy of
that Record as its type. Now, all defs with the same superclasses will
share the same type.

This allows us to be more consistent with standard expectations about
type checks involving records:

  • typeIsConvertibleTo actually requires the LHS to be a subtype of the RHS
  • resolveTypes will return the least supertype of given record types in all cases
  • different record types in the two branches of an !if are handled correctly

Add a test that used to be accepted without flagging the obvious type
error.

Change-Id: Ib366db1a4e6a079f1a0851e469b402cddae76714

Diff Detail

Event Timeline

nhaehnle created this revision.Feb 23 2018, 7:54 AM
tra added a comment.Feb 23 2018, 3:52 PM

My brain is fried by now. I'll get back to this patch on Monday.

lib/TableGen/Record.cpp
191–195

return llvm::any_of(...) ?

205–209

return llvm::all_of(...) ?

nhaehnle marked 2 inline comments as done.Feb 25 2018, 8:22 AM

No worries :) I very much appreciate the time you're taking to go over these changes!

lib/TableGen/Record.cpp
191–195

Good idea, done. Also changed some variable names, hope that helps.

205–209

Done.

nhaehnle updated this revision to Diff 135828.Feb 25 2018, 8:23 AM
nhaehnle marked 2 inline comments as done.

Use any_of/all_of.

tra added inline comments.Feb 26 2018, 2:58 PM
lib/TableGen/Record.cpp
136

TableGen is a library. Having static cache of RecordRecTy will be a problem if the same library is used to process more than one independent tablegen input. Perhaps it should live somewhere in the RecordKeeper?

210

This could use a brief description of what this function does (or, perhaps a more descriptive name). It's hard to tell that resolveRecordTypes returns a record type built from common ancestors of both types.

228

This could use an explanation.

If I understand it correctly, we assume that getSuperClasses() returns the list of superclasses as a forest, flattened in preorder. When we drop 1 + SC->getSuperClasses().size() we remove SC and all its superclasses and we'll descend further when we eventually pop SC off the Stack. It's a neat arrangement, but this is rather fragile as it seems to depend on the details of the implementation. At least I don't see anywhere in the TableGen headers that getSuperClasses guarantees any particular order. At the very least there should be some comments explaining what's going on.

1410–1415

Perhaps it's worth extracting this into a getDirectSuperClasses() function? It would make resolveRecordTypes() easier to understand.

nhaehnle updated this revision to Diff 136982.Mar 5 2018, 5:40 AM
nhaehnle marked 3 inline comments as done.

Extract getDirectSuperClasses, move RecordRecTy pool to RecordKeeper.

nhaehnle added inline comments.Mar 5 2018, 5:41 AM
lib/TableGen/Record.cpp
136

I've moved the pool to the RecordKeeper. Keep in mind that the whole of the TableGen frontend leaks objects in those pools, but at least with this change we can no longer run into the situation where a RecordRecTy is re-used after the underlying Record(s) have been freed and then a new Record was allocated with the same pointer.

210

Added a comment.

1410–1415

Good idea, done.

tra accepted this revision.Mar 5 2018, 4:38 PM
tra added inline comments.
lib/TableGen/Record.cpp
1637–1645

This could still benefit from a few comments regarding expected layout of classes in the array. Function tells us *what* this function is expected top produce, but it's still relatively hard to tell why it does things the way it does.

This revision is now accepted and ready to land.Mar 5 2018, 4:38 PM
This revision was automatically updated to reflect the committed changes.
nhaehnle marked an inline comment as done.Mar 6 2018, 5:54 AM
nhaehnle added inline comments.
lib/TableGen/Record.cpp
1637–1645

I thought the comment on the SuperClasses member variable is enough, but I suppose it doesn't hurt, so I added another small comment.