This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvm] Support importing magic globals
ClosedPublic

Authored by Dinistro on Jan 3 2023, 1:43 AM.

Details

Summary

This commit adds support for importing the magic globals "global_ctors"
and "global_dtors" from LLVM IR to the LLVM IR dialect. The import
fails when these globals have a non-null data pointer, as this can
currently not be represented in the corresponding MLIR operations.

Diff Detail

Event Timeline

Dinistro created this revision.Jan 3 2023, 1:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
Dinistro requested review of this revision.Jan 3 2023, 1:43 AM
gysit added inline comments.Jan 3 2023, 4:23 AM
mlir/include/mlir/Target/LLVMIR/ModuleImport.h
61

I would probably rename to convertGlobalCtorsAndDtors since most modern functions use convert instead of process. Also let's make it a private method except there is a good reason to not do that.

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

nit: drop newline

337–338

nit: Let's use braces for multi-line if statements.

601

nit: prio -> priority? and maybe init -> initializer?

620

After creating the GlobalCtorsOp/GlobalDtorsOp also update the globalInsertionOp (cf. line 540 in process global).

622

nit: As this is used twice I would suggest to add a getGlobalCtorVarName() or similar?

628

nit: Let's use mlirModule->getLoc() instead of UnknownLoc since we actually know at least the filename. I plan to change this for the other globals as well (the change has been discussed/suggested in https://reviews.llvm.org/D140768.

mlir/test/Target/LLVMIR/Import/import-failure.ll
41

nit: drop the newline here and below and also add "error" on the check line to make sure the code emits an error. I would probably also match at least the start of the variable.

72

nit: can you comment for the different tests why the import is supposed to fail?

Dinistro updated this revision to Diff 485979.Jan 3 2023, 6:07 AM

address review comments

Dinistro marked 9 inline comments as done.Jan 3 2023, 6:10 AM
Dinistro added inline comments.
mlir/lib/Target/LLVMIR/ModuleImport.cpp
622

Added a constexpr on the top instead.

Dinistro updated this revision to Diff 485982.Jan 3 2023, 6:35 AM
Dinistro marked an inline comment as done.

additional minor fixes

gysit accepted this revision.Jan 3 2023, 6:49 AM

LGTM!

This revision is now accepted and ready to land.Jan 3 2023, 6:49 AM
Dinistro updated this revision to Diff 486177.Jan 3 2023, 11:53 PM

Fix error location to point to module loc

This revision was automatically updated to reflect the committed changes.