Page MenuHomePhabricator

TableGen/SearchableTables: Support more generic enums and tables
ClosedPublic

Authored by nhaehnle on Jun 11 2018, 5:44 AM.

Details

Summary

This is essentially a rewrite of the backend which introduces TableGen
base classes GenericEnum, GenericTable, and SearchIndex. They allow
generating custom enums and tables with lookup functions using
separately defined records as the underlying database.

Also added as part of this change:

  • Lookup functions may use indices composed of multiple fields.
  • Instruction fields are supported similar to Intrinsic fields.
  • When the lookup key has contiguous numeric values, the lookup function will directly index into the table instead of using a binary search.

The existing SearchableTable functionality is internally mapped to the
new primitives.

Change-Id: I444f3490fa1dbfb262d7286a1660a2c4308e9932

Diff Detail

Repository
rL LLVM

Event Timeline

nhaehnle created this revision.Jun 11 2018, 5:44 AM
javed.absar added inline comments.Jun 11 2018, 3:21 PM
include/llvm/TableGen/SearchableTable.td
15 ↗(On Diff #150720)

nitpick - "Is is guarded by..."

javed.absar added inline comments.Jun 11 2018, 4:18 PM
include/llvm/TableGen/SearchableTable.td
77 ↗(On Diff #150720)

Maybe change to 'MyEnum' to be consistent with rest of the example.

tra added a comment.Jun 12 2018, 5:22 PM

Few nits. LGTM syntax-wise. I'm not very familiar with backend, so will defer to someone who has better idea.

include/llvm/TableGen/SearchableTable.td
22–26 ↗(On Diff #150720)

What's expected to happen if we have names foo and FOO?

utils/TableGen/SearchableTableEmitter.cpp
166–167 ↗(On Diff #150720)

Either there's one extra ' or one is missing. There's no closing quote for Field.Name.

660 ↗(On Diff #150720)

If would be good to check and warn about name clashes if we have records that differ by case only.

695 ↗(On Diff #150720)

Nit: I'd use auto *IndexRec or even Record *IndexRec, otherwise it makes one wonder whether we are creating unnecessary copies here and why it's not auto const &.

nhaehnle updated this revision to Diff 151331.Jun 14 2018, 4:50 AM
nhaehnle marked 4 inline comments as done.
  • Address review comments
  • Fix formatting
  • Fix IsContiguous optimization for non-primary search indices
nhaehnle added inline comments.Jun 14 2018, 4:50 AM
include/llvm/TableGen/SearchableTable.td
15 ↗(On Diff #150720)

Fixed, thanks.

22–26 ↗(On Diff #150720)

Nothing in particular. They will use the same include guard name, like the code that existed before.

Having an enum and a table guarded by the same guard is actually something that happens with the translation of the old SearchableTable, and it's why I've moved the corresponding #undefs to be at the end of the generated file.

So I'm not sure that is something that needs a warning either? I mean, everything should still work if you have two definitions that differ only by case, it's just that you then cannot selectively "GET" only one of them from C++.

77 ↗(On Diff #150720)

Done.

utils/TableGen/SearchableTableEmitter.cpp
166–167 ↗(On Diff #150720)

Fixed, thanks.

695 ↗(On Diff #150720)

Done.

Ping. Does this look alright?

tra added inline comments.Jun 18 2018, 11:49 AM
include/llvm/TableGen/SearchableTable.td
22–26 ↗(On Diff #150720)

Poking around at how we use generated files, it appears that including unintended bits may potentially be a problem.

E.g. ARM and AArch64 put individual records into their own namespaces. Accidentally putting multiple conflicting implementations may become a problem if/when, for example, someone attempts to pull both namespaces in with using, then structs/functions from both records will become ambiguous. It's a somewhat contrived scenario easily fixed by renaming an entity in .td, but I'd argue that if a tool provides a feature with constraints on its inputs, then it's the tool's duty to report when those constraints are violated. At the very least it should be clearly documented. Currently the doc is somewhat hard to read in that regard -- the example names GET_name_DECL use *lower*case name, but then say that the name will really be upper case and don't mention naming conflicts at all. The reader can deduce that making the name uppercase can lead to possible conflicts and unintended consequences down the road, but that's removed from the source of such error both in time and space.

Where possible, I'd prefer to make errors obvious as close to the source as we can manage. If that's not worth it, we should at least add a note in the docs that it's user's responsibility to guarantee non-clashing names.

nhaehnle updated this revision to Diff 151856.Jun 19 2018, 12:15 AM

Preserve the case in preprocessor guards for GenericEnum and GenericTable.

We'll still use upper case for SearchableTable, but that is just to
preserve compatibility and not change behavior.

nhaehnle marked an inline comment as done.Jun 19 2018, 12:17 AM
nhaehnle added inline comments.
include/llvm/TableGen/SearchableTable.td
22–26 ↗(On Diff #150720)

I'm changing it to just not make the name upper case for the new primitives. So for a table called MyTable, it'll be guarded by GET_MyTable_IMPL. That way, it's "impossible" to make mistakes.

I'm keeping the behavior of the old SearchableTable as-is though, I don't think it makes sense to change that.

tra accepted this revision.Jun 20 2018, 11:06 AM

LGTM in general. If anyone has comments on the back-end functionality, feel free to chime in.

include/llvm/TableGen/SearchableTable.td
22–26 ↗(On Diff #150720)

Works for me.

This revision is now accepted and ready to land.Jun 20 2018, 11:06 AM
This revision was automatically updated to reflect the committed changes.
RKSimon added a subscriber: RKSimon.Aug 9 2018, 6:09 AM
RKSimon added inline comments.
llvm/trunk/utils/TableGen/SearchableTableEmitter.cpp
437

@nhaehnle We are still seeing signed/unsigned warnings on MSVC builds - please fix this.

http://lab.llvm.org:8011/builders/lld-x86_64-win7/builds/25901/steps/build%20lld/logs/warnings%20%282%29

nhaehnle marked an inline comment as done.Aug 22 2018, 4:29 AM
nhaehnle added inline comments.
llvm/trunk/utils/TableGen/SearchableTableEmitter.cpp
437

Please see D51097.