[PATCH] D77576: [globalisel] Add lost debug locations verifier

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In D77576#1978914 <https://reviews.llvm.org/D77576#1978914>, @qcolombet wrote:

> Hi Daniel,
>
> What is the compile time impact of this change?


That depends on VerifyDebugLocs. When it's `None` (default for release), there shouldn't be a measurable impact as the observer doesn't get installed. I haven't measured it for `Locations` (current default for asserts) yet as I've been more focused on fixing the bugs it finds in our out-of-tree target so far. I don't expect it to be noticable as it's a fairly simple matcher on small search spaces but if it is we can change the default to None at the cost of needing to use an extra option in out `llvm-lit -Dllc='llc ...'` command.

> Is there a way to make the callers agnostic to the kind of observers we are dealing with?

In general, not really as each observer implementation will have different requirements depending on what they're for. That said, two of the three observers so far wrap logical changes (transactions except we don't really have that concept) to the IR and the third just follows the events as they happen. So while we can't really solve it in general, we could add another event to observer interface. There's no guarantee we won't need something else in the future though that causes the observers to diverge again though.

> Basically, I like the observer idea for checking the debug information, but I don't like that:
> 
> 1. We have to add a bunch of checkpoint calls all over the place

There's not really any avoiding these calls. The verifier needs to check often to minimize the times it just gives up but not so often that it checks while the IR is expected to be in a broken state. Only the algorithm knows when it's safe to have the verifier check the locations. Maybe a transaction-complete style event is common enough that we can add it to the observer interface and leave it very vague as to what a transaction is.

> 2. We have to specifically carry this observer around instead of having it being with the other observers (`AuxObservers` argument)

FWIW, it's 'as well as it being with the other observers' but that makes no real difference to your point. I dislike this too both because AuxObservers is trying to force common behaviour where it doesn't really exist but also because if it's going to be there then there shouldn't need to be separate arguments as well. It's possible to fix this but only if we design the observer interface to suit the observers that exist today rather than what they are in principle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77576