[Mlir-commits] [mlir] [MLIR][LLVM] Add DILocAttr for debug locations (PR #186146)

[Aman LaChapelle]

bzcheeseman wrote:

> Currently, this is a partial migration but even the existing upstream utilities will break this. Among these utilities is for example the inliner, which produces callsite locations. Therefore, all users of the debug locations would now need to deal with even more possible formats, which is not necessarily desired.

I'm not sure I agree with this - the inliner can continue to produce callsite locations, just the caller/callee will be DILocAttr rather than FusedLoc. I would expect that we'd have some test failures if this change was that broken :)

> However, this seems only beneficial after a full migration.

I think it's *most* beneficial after a full migration, yes, I would argue that some progress here is still good. To me there are currently 2 issues with the representation:
1. FusedLoc can mean many things - and if you want to represent more than just DILocalScope in the metadata attribute, you will break all the utilities that try to understand DILocalScope as the only thing in the metadata field, or you end up wrapping with another FusedLoc and that just gets weird fast.
2. Higher level dialects have to throw away structure (that exists in LLVM IR) to conform to this FusedLoc convention, and then we just re-create the structure again, which seems silly to me if we expect the LLVM dialect to be a relatively faithful representation of LLVM IR.

IMO, especially here, some progress >> no progress :)

> If you prefer having separate PRs, it is fine to prepare the follow-up changes as stacked PRs. I think it is important to see the full migration story. The change in isolation adds extra complexity since there are now two different representations for the same information.

+1, seeing the context of how this would be used I think will help.

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