This is an archive of the discontinued LLVM Phabricator instance.

[mlir][LLVM] Introduce verfier for call debug locations
ClosedPublic

Authored by Dinistro on Aug 4 2023, 7:06 AM.

Details

Summary

This commit introduces a debug location verifier for the LLVM dialect's
call operation. LLVM does not allow calls to have no debug location when
they reference an inlinable function with debug information. This
apparenlty breaks assumptions of LLVM's inliner.

So far, there was a hack in the LLVM export that avoided this case to
be triggered, but that hack causes issues when debug intrinsics are
involved. Link to the revision that inroduced the export hack:
https://reviews.llvm.org/D88135

LLVM's verifier as a reference: https://github.com/llvm/llvm-project/blob/2df05cd01c17f3ef720e554dc7cde43df27e5224/llvm/lib/IR/Verifier.cpp#L3546

Diff Detail

Event Timeline

Dinistro created this revision.Aug 4 2023, 7:06 AM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Dinistro requested review of this revision.Aug 4 2023, 7:06 AM
Dinistro edited the summary of this revision. (Show Details)Aug 4 2023, 7:07 AM
zero9178 added inline comments.Aug 4 2023, 7:16 AM
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1028

I believe it should be suffiecent to check !isa<UnknownLoc>(callOp->getLoc()) here as UnknownLoc can't occur in fused locs.
The constructor of FusedLoc sorts them out of the list: https://github.com/llvm/llvm-project/blob/46b2ad0224d3c9a9cc299211213e2cf677f5a78c/mlir/lib/IR/Location.cpp#L104

Should the variable also maybe be called just containsLoc? containsLLVMDebugLoc sounds to me like its talking about actuall LLVM IR past the exporter, while we are dealing with MLIR locs here

zero9178 accepted this revision.Aug 4 2023, 7:16 AM

LGTM otherwise

This revision is now accepted and ready to land.Aug 4 2023, 7:16 AM
Dinistro updated this revision to Diff 547201.Aug 4 2023, 7:19 AM
Dinistro marked an inline comment as done.

address review comments

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1028

I was not aware that UnknownLocs are filtered out, thanks for pointing it out.

gysit accepted this revision.Aug 4 2023, 7:28 AM

LGTM

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1013

ultra nit:

This revision was automatically updated to reflect the committed changes.
Dinistro marked an inline comment as done.Aug 6 2023, 11:05 PM