Page MenuHomePhabricator

[TableGen] Better error checking for TIED_TO constraints.

Authored by simon_tatham on Oct 29 2018, 7:09 AM.



There are quite strong constraints on how you can use the TIED_TO
constraint between MC operands, many of which are currently not
checked until compiler run time.

MachineVerifier enforces that operands can only be tied together in
pairs (no three-way ties), and MachineInstr::tieOperands enforces that
one of the tied operands must be an output operand (def) and the other
must be an input operand (use).

Now we check these at TableGen time, so that if you violate any of
them in a new instruction definition, you find out immediately,
instead of having to wait until you compile something that makes code
generation hit one of those assertions.

Also in this commit, all the error reports in ParseConstraint now
include the name and source location of the def where the problem
happened, so that if you do trigger any of these errors, it's easier
to find the part of your TableGen input where you made the mistake.

The trunk sources already build successfully with this additional
error check, so I think no in-tree target has any of these problems.

Diff Detail


Event Timeline

simon_tatham created this revision.Oct 29 2018, 7:09 AM

Yay for better error messages! One comment about unfortunate variable naming...

262–269 ↗(On Diff #171492)

The naming of SrcOp and DestOp should be reversed. It's quite confusing as-is ;)

simon_tatham added inline comments.Oct 31 2018, 4:53 AM
262–269 ↗(On Diff #171492)

Yes, I see what you mean – now that we're enforcing that one of the operands is written and the other is read, it makes particularly little sense to call them 'source' and 'destination' in that order :-) I'll fix it.

OK, here's a second draft which names the variables more sensibly. Perhaps slight overkill, in that I've used LHSOp and RHSOp for the variables ordered as they are in the source constraints string, and then DestOp and SrcOp once they're reordered to (what should be) a def and a use.

simon_tatham marked 2 inline comments as done.Oct 31 2018, 8:20 AM
MatzeB accepted this revision.Oct 31 2018, 5:35 PM

LGTM. Looks sensible to me, nitpicks below.

I'm not an expert in this area of tablegen though, so maybe wait a few days before committing (unless someone else accepts as well or someone has a complaint).

255 ↗(On Diff #171927)

can this be a StringRef or is Name destroyed/changed on the way?

289 ↗(On Diff #171927)

I would prefer no auto when the type isn't immediately clear just by looking at the line at hand...

This revision is now accepted and ready to land.Oct 31 2018, 5:35 PM

Name is rewritten between creation of LHSOp and RHSOp, so you
can't just make both of them into StringRefs because LHSOp would be
invalidated by the change to Name.

But now you point it out, Name doesn't really need to exist, because
instead of extracting a substring from CStr and searching it using
find_first_of, you can just search CStr itself using
find_first_of with the optional start-position argument. Here's a
rewrite that removes Name completely and turns everything into

Also replaced the auto (after looking up what the type was myself :-).

I've just come back to this patch after being distracted for several weeks (sorry about that). I intend to actually commit it shortly, and this time I'll try not to forget :-)

This revision was automatically updated to reflect the committed changes.