[llvm] [RemoveDIs][DebugInfo][IR] Add parsing for non-intrinsic debug values (PR #79818)

Jeremy Morse via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 22 06:22:15 PST 2024


https://github.com/jmorse commented:

This is looking good, I've got some moderate nits inline.

I've got a slight twitching feeling about the addition of the debug-records as keywords (well, Identifiers) as it seems like quite a large commitment. However: we can always change the manner in which it's parsed, I think the format is good.

I'm not completely certain about making the mixing of debug-info formats a parsing error: wouldn't it be more natural to defer it until the Verifier? I get that that's impossible right now due to the conversions, but we could check for the existence of the three intrinsics by querying the `Module` in `finalizeDebugInfoFormat` and make that an error. That'd be less invasive than filtering the names of all call instruction targets. (Downside: no localised errors, but this doesn't strike me as being something hard to pin down).

Having an error on even the _declaration_ of intrinsics, not just the use of them, seems like quite a firm requirement. I'm not sure if that'll bite us, but we can downgrade the check if that proves to be a problem.

Some sadly menial nits: IMO the test names shouldn't contain "removedi" as that's referring to a transitional project name, in several years time no-one will recognise that name, IMO they should be named something like "dbg-record-invalid-$num" or something? IMHO it's also worth generating a positive-input that successfully parses so that we can point people at it and indicate the range of different things that can be parsed.

(It feels like this is all pretty close!)

https://github.com/llvm/llvm-project/pull/79818


More information about the llvm-commits mailing list