Page MenuHomePhabricator

[tablegen] Optionally format tablegen targets with clang-format
Needs ReviewPublic

Authored by dsanders on Feb 23 2017, 6:20 AM.

Details

Summary

While it's useful to make a reasonable effort to format tablegen output in a
sensible way, it's also easy to get overly bogged down in the formatting
concerns.

This patch adds a way for tablegen-erated intermediate files to opt-in to being
filtered through clang-format when it's available. Files that opt-in to this
will fall back on the output that comes directly from tablegen.

To opt-in, invoke tablegen() using the FORMAT option like so:

tablegen(LLVM AArch64GenRegisterBank.inc FORMAT OPTIONS -gen-register-bank)

Event Timeline

dsanders created this revision.Feb 23 2017, 6:20 AM
qcolombet edited edge metadata.Mar 24 2017, 6:59 PM

Hi Daniel,

That's interesting.
I am a bit concerned about making GISel special in terms of how the output looks. I can see the argument but I am also wondering if we don't what to have a control on that. For instance, in SDISel, each entry in the array is on its own line and clang-format could break that.

Ideally I would like we have the same (post) processing for every tablegen file but given I don't see that happening, (at least in the near future) I would lean toward properly emitting the output from the start.

Cheers,
-Quentin

My ideal would be version of raw_ostream that allows clang-format style formatting to be switched on and off at will but that doesn't seem to be available and it would require llvm-tblgen to link to (probably old) clang libraries.

For AArch64GenGlobalISel.inc in particular, the main aim is to defer any significant work on pretty-printing until after the content of the file has settled down. The current formatting is difficult to read and debug but fixing it is an intrusive change that's wasted effort given that we intend to replace the bulk of it with an array like SelectionDAG's.

rovka edited edge metadata.Mar 30 2017, 4:41 AM

Hi Daniel,

That's interesting.
I am a bit concerned about making GISel special in terms of how the output looks. I can see the argument but I am also wondering if we don't what to have a control on that. For instance, in SDISel, each entry in the array is on its own line and clang-format could break that.

Is that the case for many of the files that we generate? If not, maybe we can make it opt-out instead?
Alternatively, clang-format accepts some configuration options, is there any combination of those that would make the output of SDISel look ok when clang-formatted?

Ideally I would like we have the same (post) processing for every tablegen file but given I don't see that happening, (at least in the near future) I would lean toward properly emitting the output from the start.

Cheers,
-Quentin

Sorry for being slow to get back to this one. I've been prioritising the other patches.

I am a bit concerned about making GISel special in terms of how the output looks. I can see the argument but I am also wondering if we don't what to have a control on that. For instance, in SDISel, each entry in the array is on its own line and clang-format could break that.

Is that the case for many of the files that we generate? If not, maybe we can make it opt-out instead?
Alternatively, clang-format accepts some configuration options, is there any combination of those that would make the output of SDISel look ok when clang-formatted?

I think it's beneficial to some of them, for example MatchTable0 in the AsmMatcher is a lot easier to read after clang-format. However there's definitely some things clang-format shouldn't touch. Here's a couple examples:
AArch64GenAsmWriter.inc: AsmStrs isn't particularly readable either way but the comments are difficult to spot after clang-format
AArch64GenDAGISel.inc: Tablegen uses indentation to show the scopes but clang-format destroys this information. GlobalISel will probably end up doing something similar in the long run.

If clang-format has a way to protect certain regions then we could add the appropriate annotations and generally enable it.