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

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs marked an inline comment as done.
jroelofs added inline comments.


================
Comment at: llvm/utils/TableGen/CodeGenSchedule.cpp:1093
                    "\".");
+              PrintFatalError(RWD->getLoc(), "Previous match was here.");
             }
----------------
evandro wrote:
> 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.
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83588