This is an archive of the discontinued LLVM Phabricator instance.

[mlir][LLVM] Delay debug intrinsic import to ensure dominance
ClosedPublic

Authored by Dinistro on Aug 9 2023, 4:59 AM.

Details

Summary

This commit ensures that debug intrinsics are imported after all other
instructions of a function were imported.

Debug intrinsics in LLVM are excluded from the define-before-use
invariant, i.e., they can reference SSA values that do not dominate
them. So far, we had implemented checks to stop the import of such
intrinsics, but there were always additional cases that were not
covered (the latest being terminators that define such used values).

Diff Detail

Event Timeline

Dinistro created this revision.Aug 9 2023, 4:59 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 9 2023, 4:59 AM

This revision solves the same fundamental problem as https://reviews.llvm.org/D157386. This revision is less generic, and thus requires much less boilerplate code to work.

zero9178 accepted this revision.Aug 9 2023, 5:11 AM

LGTM. I very much prefer this version over the other. It properly takes dominance into account, caches the dominance info and is a lot more straightforward in terms of controlflow

This revision is now accepted and ready to land.Aug 9 2023, 5:11 AM
gysit added a comment.Aug 9 2023, 5:23 AM

Thanks for iteration. I think simplicity wins here!

LGTM modulo some nit comments.

mlir/include/mlir/Target/LLVMIR/ModuleImport.h
214–215

nit: should we rename to clearRegionMappings or clearRegionState?

236–238

ultra nit:

mlir/lib/Target/LLVMIR/ModuleImport.cpp
1802

Would it make sense to call setNonDebugMetadataAttrs in case a downstream user wants to import custom metadata?

gysit accepted this revision.Aug 9 2023, 5:24 AM

LGTM

Forgot to accept :)

Dinistro updated this revision to Diff 548587.Aug 9 2023, 5:55 AM
Dinistro marked 3 inline comments as done.

address review comments

This revision was landed with ongoing or failed builds.Aug 9 2023, 6:08 AM
This revision was automatically updated to reflect the committed changes.