This is an archive of the discontinued LLVM Phabricator instance.

AArch64: refactor sysreg handling (new TableGen backend!)
ClosedPublic

Authored by t.p.northover on Jun 7 2016, 12:10 PM.

Details

Summary

Hi,

I decided it was time to take a look at improving how we handle the special symbolic AArch64 sysreg names (and those of related instructions). I think the current implementation has a few issues:

  • Large-scale duplication between AArch64BaseInfo.h and AArch64BaseInfo.cpp
  • That weird Mapping class that I have no idea what I was on when I thought it was a good idea.
  • Searches are performed linearly through the entire list.
  • We print absolutely all registers in upper-case, even though some are canonically mixed case (SPSel for example).
  • The ARM ARM specifies sysregs in terms of 5 fields, but those are relegated to comments in our implementation, with a slightly opaque hex value indicating the canonical encoding LLVM will use.

So, with all that in mind I decided to do something about it. This patch adds a new TableGen backend: -gen-searchable-tables, which is quite a bit of code but seems like it ought to be useful elsewhere (though I didn't manage to think of any places immediately). That backend will emit a primary data table (reasonably generic, at the moment it can contain strings, ints and opaque code blobs) together with specified indexes (sorted for a binary search) and lookup functions.

This was actually inspired by trying to make Clang's diagnostics better (specifically warning when a 64-bit register is used for arm_rsr or arm_wsr), so any ideas on how best to share it would be welcome (best I've come up with it putting the AArch64-specific file in include/llvm/Target and rebuilding it from Clang).

The other nasty hack is DBGDTRTX_EL0 and DBGDTRRX_EL0 which share an encoding but are written differently in MRS/MSR. I just completely hacked around that in InstPrinter, deciding the extra infrastructure for a generic solution wasn't worth implementing.

So, any suggestions for improvements? Anyone think it's a terrible idea from the start?

Diff Detail

Repository
rL LLVM

Event Timeline

t.p.northover retitled this revision from to AArch64: refactor sysreg handling (new TableGen backend!).
t.p.northover updated this object.
t.p.northover added reviewers: rengolin, jmolloy.
t.p.northover set the repository for this revision to rL LLVM.
t.p.northover added a subscriber: llvm-commits.
ab added a subscriber: ab.Jun 7 2016, 12:24 PM

Ooh, shiny! Looks like a nice cleanup to me.

A couple high-level comments.

include/llvm/TableGen/SearchableTable.td
47–49

Hmm, why not use 'code' directly?

lib/Target/AArch64/AArch64SystemOperands.td
22–27

The MappingKind defs are a bit awkward; maybe inherit from MappingKind itself in AT/DB/..., get rid of InstanceClass, and pass the fields as template arguments?

That also lets you nicely factor out the similar Fields/SearchableFields/... here.

utils/TableGen/SearchableTableEmitter.cpp
11–13

Stale description

Thanks for looking Ahmed.

include/llvm/TableGen/SearchableTable.td
47–49

Oh, you'll love this one: code just gets mapped to a plain StringInit in TGParser.cpp. There's no way for a backend to tell what was intended, and I need at least some strings to be quoted.

I'll put a comment in about that.

lib/Target/AArch64/AArch64SystemOperands.td
22–27

I started off like that, but switched. I can't quite remember the details, I *think* it was because I couldn't come up with a clean way to tell what instances of MappingKind belonged to what table at the time.

But I could probably iterate through all classes and just handle subclasses of MappingKind. I'll see if I can make it work, because it would be a lot neater from a user's point of view.

This should fix the issues you spotted (thanks!).

For the "Code" thing, I decided to promote it from syntactic sugar in TableGen so that we don't need the extra indirection. I've combined the lib/TableGen patch here, but it's separate in my repo and I can easily create a separate review thread if you'd prefer.

Tim.

ab added a comment.Jun 20 2016, 2:20 PM

Nicer indeed!

Another quick pass and some minor details; I'll have a closer look later.

include/llvm/TableGen/Record.h
610

Worth a (yes, duplicated) comment, if only for consistency?

lib/Target/AArch64/AArch64SystemOperands.td
61–62

Any reason this isn't:

bits<4> Encoding = encoding;

?

utils/TableGen/SearchableTableEmitter.cpp
301–302

Wrapping

ab added a comment.Jun 22 2016, 3:38 PM

Another round of nitpicks; this generally looks good to me.

For the "Code" thing, I decided to promote it from syntactic sugar in TableGen so that we don't need the extra indirection. I've combined the lib/TableGen patch here, but it's separate in my repo and I can easily create a separate review thread if you'd prefer.

Eh, it's fine here (with a separate commit).

This was actually inspired by trying to make Clang's diagnostics better (specifically warning when a 64-bit register is used for arm_rsr or arm_wsr), so any ideas on how best to share it would be welcome (best I've come up with it putting the AArch64-specific file in include/llvm/Target and rebuilding it from Clang).

I suppose it could go in lib/Support, like the TargetParser ?

The other nasty hack is DBGDTRTX_EL0 and DBGDTRRX_EL0 which share an encoding but are written differently in MRS/MSR. I just completely hacked around that in InstPrinter, deciding the extra infrastructure for a generic solution wasn't worth implementing.

I agree.

lib/Target/AArch64/AArch64.td
135–140

The file looks independent from the rest of the target; use a separate top-level .td? CMake might make it tricky though..

utils/TableGen/SearchableTableEmitter.cpp
33

Unnecessary private

51–52

Should this be an error?

60–62

utohexstr() ?

69

Also an error?

94–96

utostr() and + ?

166

std::stable_sort

230–231

format/alignment (here and elsewhere)

t.p.northover marked 9 inline comments as done.

Thanks for the comments, I've uploaded a new version of the patch incorporating most of them.

lib/Target/AArch64/AArch64.td
135–140

Yes, I did try that originally but CMake really only wants one .td file per target. The issue becomes moot if move it to lib/Support or wherever.

utils/TableGen/SearchableTableEmitter.cpp
51–52

Should probably be removed entirely. It's a relic from an earlier implementation.

60–62

I thought it was weird we didn't have one!

ab accepted this revision.Jul 5 2016, 1:25 PM
ab added a reviewer: ab.

A few final nitpicks. Do you want to consider the lib/Support move separately? If so, LGTM.

utils/TableGen/SearchableTableEmitter.cpp
60

PrintFatalError? Here and elsewhere.

232

const&? Or is the copy intentional?

253

auto *?

Also, I'm not a huge fan of Almost Always Auto, so I'd just use Record*, but either is fine.

259

auto *?

306

Hoist out RK.getClass("SearchableTable") and use isSubClassOf(Record*) ?

This revision is now accepted and ready to land.Jul 5 2016, 1:25 PM
t.p.northover closed this revision.Jul 5 2016, 2:30 PM
t.p.northover marked 3 inline comments as done.

Thanks Ahmed. It should be committed (with most of your latest suggestions) as r274575 and r274576.

Tim.

utils/TableGen/SearchableTableEmitter.cpp
232

getAllDerivedDefinitions returns an std::vector by value so I think they're equivalent (via RVO).

253

I went with auto * to avoid an ugly line wrap. I've used Record * below though, where it's less obvious what the type is.

ab added inline comments.Jul 5 2016, 3:41 PM
utils/TableGen/SearchableTableEmitter.cpp
232

Ah, I meant for TableName

t.p.northover marked an inline comment as done.Jul 5 2016, 3:59 PM
t.p.northover added a subscriber: t.p.northover.

const&? Or is the copy intentional?

getAllDerivedDefinitions returns an std::vector by value so I think they're equivalent (via RVO).

Ah, I meant for TableName

That makes more sense. Fixed in r274584.

Tim.