Page MenuHomePhabricator

[mlir] Add support to SourceMgrDiagnosticHandler for filtering FileLineColLocs
ClosedPublic

Authored by rriddle on Jun 3 2021, 2:47 PM.

Details

Summary

This revision adds support for passing a functor to SourceMgrDiagnosticHandler for filtering out FileLineColLocs when emitting a diagnostic. More specifically, this can be useful in situations where there may be large CallSiteLocs with locations that aren't necessarily important/useful for users.

For now the filtering support is limited to FileLineColLocs, but conceptually we could allow filtering for all locations types if a need arises in the future.

Diff Detail

Event Timeline

rriddle created this revision.Jun 3 2021, 2:47 PM
rriddle requested review of this revision.Jun 3 2021, 2:47 PM
jpienaar added inline comments.Jun 4 2021, 8:26 AM
mlir/include/mlir/IR/Diagnostics.h
535

Could we formulate this one more actively?

538

Could this just be left up to caller? (Haven't gotten to code yet to see)

mlir/lib/IR/Diagnostics.cpp
466–472

Why not filter below just before emitting? Is this in case the top level one is filtered or all empty?

468

Couldn't this be in the function passed in and then the API be more general?

498

Here it is assuming that this is always due to callSiteLoc main loc - which is true today, but that was previously nicely guarded above, now it's an assumption below.

rriddle marked 5 inline comments as done.Jun 7 2021, 5:34 PM
rriddle added inline comments.
mlir/lib/IR/Diagnostics.cpp
466–472

Yeah, we need to extract one location to emit the main diagnostic. If we do it as is, we need to check if we've emitted the main diagnostic at each step, so it's simpler to just collect them post filtering.

rriddle updated this revision to Diff 350462.Jun 7 2021, 5:34 PM
rriddle marked an inline comment as done.

update

Thanks for the review, PTAL.

jpienaar accepted this revision.Jun 10 2021, 3:45 PM
jpienaar added inline comments.
mlir/lib/IR/Diagnostics.cpp
527

So a FusedLoc will not be shown if any of its children should not be shown? So the should show function is sort of an and over all the nested locations?

This revision is now accepted and ready to land.Jun 10 2021, 3:45 PM
rriddle marked an inline comment as done.Jun 14 2021, 12:39 PM
rriddle added inline comments.
mlir/lib/IR/Diagnostics.cpp
527

Right, fused loc is a bit weird because we want to show at least one. Reworked to pick one of the sub-locations that can be shown.

jpienaar accepted this revision.Jun 14 2021, 2:29 PM
jpienaar added inline comments.
mlir/lib/IR/Diagnostics.cpp
527

Still seems slightly weird: if all of the locations fused should be shown, then I'd expect a fusedloc result unchanged - also, else we miss the fusedloc's metadata in the result which could (for example) be used to capture which pass fused the ops. So it almost seems like the returned result should still be a fused loc but only with the non-filtered locations.

rriddle marked an inline comment as done.Jun 14 2021, 2:33 PM
rriddle added inline comments.
mlir/lib/IR/Diagnostics.cpp
527

This matches the existing behavior that we have today. We don't have a defined policy (other than a certain number of CallSite frames) on when locations should be shown. It isn't clear that we should always be showing every FusedLoc except for in very simple situations, but even that can balloon out of control.

jpienaar added inline comments.Jun 14 2021, 2:38 PM
mlir/lib/IR/Diagnostics.cpp
527

But it seems we do always show the metadata if there.

rriddle marked an inline comment as done.Jun 14 2021, 2:39 PM
rriddle added inline comments.
mlir/lib/IR/Diagnostics.cpp
527

We don't AFAIK. The only existing situation where we show anything about the original location, is when we can't find a nested FileLineColLoc inside of it.

https://github.com/llvm/llvm-project/blob/a58b2827feceaa27193318a12e3a06893eabdcb6/mlir/lib/IR/Diagnostics.cpp#L433

This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.

The newly added test is flaky and it fails on the Windows bot every few runs. I also saw it fail on Ubuntu in our private buildbots.
Failing build:
https://lab.llvm.org/buildbot/#/builders/13/builds/8922

Passing build:
https://lab.llvm.org/buildbot/#/builders/13/builds/8920