Page MenuHomePhabricator

[TableGen][CGS] Print better errors on overlapping InstRW
ClosedPublic

Authored by jroelofs on Jul 10 2020, 2:01 PM.

Diff Detail

Event Timeline

jroelofs created this revision.Jul 10 2020, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2020, 2:01 PM
evandro added inline comments.Jul 10 2020, 2:26 PM
llvm/utils/TableGen/CodeGenSchedule.cpp
1093

Or rather, continuing from the previous line:

"\"" +
" at " + RWD->getLoc());
1129

Ditto.

jroelofs marked an inline comment as done.Jul 13 2020, 7:46 AM
jroelofs added inline comments.
llvm/utils/TableGen/CodeGenSchedule.cpp
1093

I considered that, but felt that the way the error messages get printed was better when each loc is emitted as a separate diagnostic (so it shows you a bit of relevant source code for each). If there were a proper DiagnosticsEngine I'd prefer to emit this second part as a note:, with the first as the error:, but TableGen diagnostics aren't well set up for that, and adding PrintFatalNote seemed weird.

jroelofs updated this revision to Diff 277441.Jul 13 2020, 8:22 AM
evandro added inline comments.Jul 13 2020, 11:41 AM
llvm/utils/TableGen/CodeGenSchedule.cpp
1093

However, I'd rather see the error messages following the same overall pattern: all information pertaining to an error can be found in the same line. It helps when filtering the output of TableGen's.

nhaehnle added inline comments.
llvm/utils/TableGen/CodeGenSchedule.cpp
1093

I tend to agree with Jon here. It is in line with the canonical way that compilers tend to report such errors nowadays. I'd rather TableGen move in that direction than do its own thing.

One issue though is that you're now no longer printing the Instrs value as a string. If the RWD record is generated as the result of an mdef, it may be hard to tell which record is really the problem. I'm not familiar enough with the scheduling models to know how problematic this is here, but it'd certainly be a problem for some other TableGen sub-dialects.

evandro added inline comments.Jul 20 2020, 1:50 PM
llvm/utils/TableGen/CodeGenSchedule.cpp
1093

I'd rather leave the overhaul of how TableGen reports error to a monolithic patch after discussion. I don't believe in moving one error message at a time towards a greater goal.

Yes, it is important to print the instruction value as a string. Thanks for catching that.

jroelofs updated this revision to Diff 279600.Jul 21 2020, 11:58 AM
  • Add back the "also matches previous" part of the diagnostics.
  • Fix formatting that the linter complains about.

Started a thread here [1] to resolve the difference in preference re: notes vs all-in-one-line errors.

1: http://lists.llvm.org/pipermail/llvm-dev/2020-July/143597.html

There probably needs to be a test in llvm/test/TableGen/

jroelofs updated this revision to Diff 279953.Jul 22 2020, 3:14 PM

add test case, clang-format

evandro added inline comments.Jul 22 2020, 3:31 PM
llvm/lib/TableGen/Error.cpp
53

Please, do not make it fatal.

jroelofs marked an inline comment as done.Jul 22 2020, 4:20 PM
jroelofs added inline comments.
llvm/utils/TableGen/CodeGenSchedule.cpp
1093

Or rather, continuing from the previous line:

"\"" +
" at " + RWD->getLoc());

@evandro what you're asking for here doesn't actually work. Twine doesn't know how to serialize the array of SMLoc's.

jroelofs marked 2 inline comments as done.Jul 22 2020, 4:28 PM
jroelofs added inline comments.
llvm/lib/TableGen/Error.cpp
53

This is appropriate for places that replace an existing PrintFatalError(); with a PrintError(); PrintFatalNote(); sequence.

If you'd like to take on replacing cases of PrintFatalError() with PrintError(), I'd be happy to review your patch.

evandro accepted this revision.Jul 24 2020, 11:27 AM
evandro added inline comments.
llvm/lib/TableGen/Error.cpp
53

Fair enough.

This revision is now accepted and ready to land.Jul 24 2020, 11:27 AM
This revision was automatically updated to reflect the committed changes.