[PATCH] D83588: [TableGen][CGS] Print better errors on overlapping InstRW

[Evandro Menezes via Phabricator via llvm-commits]

evandro added inline comments.


================
Comment at: llvm/utils/TableGen/CodeGenSchedule.cpp:1093
                    "\".");
+              PrintFatalError(RWD->getLoc(), "Previous match was here.");
             }
----------------
nhaehnle wrote:
> evandro wrote:
> > jroelofs wrote:
> > > evandro wrote:
> > > > Or rather, continuing from the previous line:
> > > > 
> > > > 
> > > > ```
> > > > "\"" +
> > > > " at " + RWD->getLoc());
> > > > ```
> > > 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.
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83588/new/

https://reviews.llvm.org/D83588