This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Allow setting "CodeView" flag in LLVMIR translation on MSVC.
ClosedPublic

Authored by scotttodd on Nov 12 2020, 9:48 AM.

Diff Detail

Event Timeline

scotttodd created this revision.Nov 12 2020, 9:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
scotttodd requested review of this revision.Nov 12 2020, 9:48 AM

There's potential for a more generic solution here. I tried passing a ArrayRef<llvm::Module::ModuleFlagEntry> through: https://github.com/ScottTodd/llvm-project/commit/e276941578e8afd1f77380eceb9d76cd1b11b41c , but I think something using attributes on the Operation might be better. For projects not using memrefs, the "malloc" and "free" functions could also be optional.

ftynse added a comment.EditedNov 12 2020, 10:34 AM

There's potential for a more generic solution here. I tried passing a ArrayRef<llvm::Module::ModuleFlagEntry> through: https://github.com/ScottTodd/llvm-project/commit/e276941578e8afd1f77380eceb9d76cd1b11b41c , but I think something using attributes on the Operation might be better. For projects not using memrefs, the "malloc" and "free" functions could also be optional.

Can we just add a triple as a module attribute similarly to what we already have for DataLayout? module attributes {llvm.data_layout = "E", llvm.triple = "x86-windows-msvc"} {...}. There is always at least the top-level module. One caveat is that we need to make sure LLVM doesn't exit(1) if it fails to parse the triple.

rriddle added inline comments.Nov 12 2020, 10:40 AM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
957

Anything debug related should be in DebugTranslation.

Use LLVM Dialect attribute to pass llvm::Triple string to DebugTranslation.cpp.

Can you add a test for this?

There's potential for a more generic solution here. I tried passing a ArrayRef<llvm::Module::ModuleFlagEntry> through: https://github.com/ScottTodd/llvm-project/commit/e276941578e8afd1f77380eceb9d76cd1b11b41c , but I think something using attributes on the Operation might be better. For projects not using memrefs, the "malloc" and "free" functions could also be optional.

Can we just add a triple as a module attribute similarly to what we already have for DataLayout? module attributes {llvm.data_layout = "E", llvm.triple = "x86-windows-msvc"} {...}. There is always at least the top-level module. One caveat is that we need to make sure LLVM doesn't exit(1) if it fails to parse the triple.

Yep, that looks good to me. Any suggestions on avoiding the exit(1)?

You may need a refactor like Alex did here: https://reviews.llvm.org/D85650 ; what about a factory method returning ErrorOr<Triple> ?

Add tests for llvm.target_triple attribute handling.

There's potential for a more generic solution here. I tried passing a ArrayRef<llvm::Module::ModuleFlagEntry> through: https://github.com/ScottTodd/llvm-project/commit/e276941578e8afd1f77380eceb9d76cd1b11b41c , but I think something using attributes on the Operation might be better. For projects not using memrefs, the "malloc" and "free" functions could also be optional.

Can we just add a triple as a module attribute similarly to what we already have for DataLayout? module attributes {llvm.data_layout = "E", llvm.triple = "x86-windows-msvc"} {...}. There is always at least the top-level module. One caveat is that we need to make sure LLVM doesn't exit(1) if it fails to parse the triple.

Yep, that looks good to me. Any suggestions on avoiding the exit(1)?

Looked through the source and tried some bogus values, I don't think LLVM crashes if it fails to parse into a meaningful Triple:
https://github.com/llvm/llvm-project/blob/master/llvm/unittests/ADT/TripleTest.cpp
https://github.com/llvm/llvm-project/blob/4726a402a32a0553cf4da8c59646d8bc99c08383/llvm/lib/Support/Triple.cpp#L737-L773

This revision is now accepted and ready to land.Nov 12 2020, 4:47 PM
ftynse accepted this revision.Nov 13 2020, 1:56 AM

Thanks!

Thanks for the reviews! Can someone land this for me? (no commit access)