[PATCH] D46054: [TableGen] Add a general-purpose JSON backend.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

> Consider what to do about integer values that don't fit exactly into a 'double'. This code will simply emit them as decimal integer literals, which JSON parsers are within their rights to round to the nearest double precision float, losing data. Some JSON readers (e.g. Python json.load) will deliver accurate integer values anyway, but it might be better not to rely on that, and instead output very large integers in some other form, such as a JSON object containing an identifying type field and two doubles whose sum is the desired integer, or a string representation of the integer, or both.

Well. The one thing I do have a strong opinion on is that the representation of very large integers should not be different from the representation of small integers, because consumers would almost certainly get that wrong. So that leaves two options: string representations, or sending JavaScript to the hell it deserves and going with integers.

> Consider adding the new -dump-json option to clang-tblgen as well as llvm-tblgen. (As I understand it they wouldn't do anything differently, but it seems asymmetric not to have both of them support it. They both have -print-records, after all.)

Sure, that seems like a good idea.

> Consider providing a cut-down version, enabled by another option such as '-dump-simple-json', in which all the complicated parametric expression nodes like !add and !foreach and !foldl are simply not emitted, and replaced by some kind of small object indicating that a complex expression was elided. The motivation is that I expect a lot of uses for this system would only be interested in the output fields that consist of final well-defined values of primitive type, so constructing the complicated parts is a waste of both TableGen's time and the consumer's. But I'm not sure where the line should be drawn - DAG arguments might well still need to be output in full, for example, and type information might be omittable. There may be no one good answer.

I believe there is a good answer :)

Take a look at TGParser.cpp, checkConcrete. All valid final and needed records should pass that check. There are unfortunately still some exceptions, although it'd be nice if we could get rid of those actually.

So I would argue that you should only bother emitting what fits this definition of "concrete", i.e. don't have a "complex" JSON option at all. If you do encounter something that doesn't fit the definition of concrete, just output a JSON object `{ "kind": "complex", "code": <getAsString()> }` in its place.

On a related note, I'm not convinced you really want to print out class definitions. The benefit of printing class definitions with -print-records is that it can help you understand what's going on while you're writing class definitions, but the existing TableGen backends really don't care about class definitions. Since the idea here is to basically allow writing TableGen backends in a scripting language like Python, providing the class definitions is unlikely to be useful. It doesn't hurt, but I don't think it's a good motivation for building all this infrastructure for printing ! operations, for example.

> Decide where all this code should live. It might be better to move a lot of it into Record.cpp in the form of getAsJSONObject() methods or something like that. That would remove the risk of forgetting to update the JSON back end if a new node type is introduced - anyone forgetting to implement that method in any new subclass of Init or RecTy would be reminded by a compile error.

I'd rather keep it separate for orthogonality. Adding a new fundamental concrete data type is a rare enough occurence, and the worst that would happen with my suggestion above is that you get an unexpected "complex" object, which is not too difficult to track down.


Repository:
  rL LLVM

https://reviews.llvm.org/D46054