Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MLIR][Flang][DebugInfo] Set debug info format in MLIR->IR translation #95098

Merged
merged 2 commits into from
Jun 11, 2024
Merged

[MLIR][Flang][DebugInfo] Set debug info format in MLIR->IR translation #95098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c84c56
[MLIR][Flang][DebugInfo] Set debug info format in MLIR->IR translation
SLTozer Jun 11, 2024
585fca5
Add comments for context
SLTozer Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions flang/lib/Frontend/FrontendActions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
#include "llvm/Analysis/TargetTransformInfo.h"
#include "llvm/Bitcode/BitcodeWriterPass.h"
#include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
#include "llvm/IR/DebugProgramInstruction.h"
#include "llvm/IR/LLVMRemarkStreamer.h"
#include "llvm/IR/LegacyPassManager.h"
#include "llvm/IR/Verifier.h"
Expand Down Expand Up @@ -81,6 +82,8 @@ using namespace Fortran::frontend;
llvm::PassPluginLibraryInfo get##Ext##PluginInfo();
#include "llvm/Support/Extension.def"

extern llvm::cl::opt<bool> WriteNewDbgInfoFormat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this. The driver should be controlled exclusively through options defined in clang/include/clang/Driver/Options.td.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright - I can revert the patch while figuring out another way to get the right behaviour. Just to clarify though - the issue isn't just that an LLVM opt appears here, but that the state of any llvm flag should have no influence on the output of the driver?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify though - the issue isn't just that an LLVM opt appears here, but that the state of any llvm flag should have no influence on the output of the driver?

Indeed. The design goal is to avoid "leaking" the internals of LLVM into the driver. Otherwise things can get very complex very quickly :) This declaration is just a manifestation of the fact that the layering has been violated.

Copy link
Contributor Author

@SLTozer SLTozer Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get the idea, but just to be clear; LLVM flags can influence the output of the clang driver as-is right now, in the sense that LLVM flags control the behaviour of LLVM internal functions, and those functions are invoked by the driver, but there's no direct use of those flags from the driver.

Currently, --write-experimental-debuginfo (the flag I added here) influences the output of the clang frontend, because the frontend uses llvm::PrintModulePass to print the final LLVM IR. I think the appropriate change to the flang frontend would be to have it do the same, so that there's no behavioural drift between the two - for an action like printing LLVM IR, I think it's probably sensible for both to offload that logic to LLVM's internals.

Does that sound like the right approach to you? Resulting change is something like:

@@ -995,6 +995,8 @@ void CodeGenAction::runOptimizationPipeline(llvm::raw_pwrite_stream &os) {

   if (action == BackendActionTy::Backend_EmitBC)
     mpm.addPass(llvm::BitcodeWriterPass(os));
+  else if (action == BackendActionTy::Backend_EmitLL)
+    mpm.addPass(llvm::PrintModulePass(os));

   // Run the passes.
   mpm.run(*llvmModule, mam);
@@ -1270,13 +1272,7 @@ void CodeGenAction::executeAction() {
   // Run LLVM's middle-end (i.e. the optimizer).
   runOptimizationPipeline(ci.isOutputStreamNull() ? *os : ci.getOutputStream());

-  if (action == BackendActionTy::Backend_EmitLL) {
-    llvmModule->print(ci.isOutputStreamNull() ? *os : ci.getOutputStream(),
-                      /*AssemblyAnnotationWriter=*/nullptr);
-    return;
-  }
-
-  if (action == BackendActionTy::Backend_EmitBC) {
+  if (action == BackendActionTy::Backend_EmitLL || action == BackendActionTy::Backend_EmitBC) {
     // This action has effectively been completed in runOptimizationPipeline.
     return;
   }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM flags can influence the output of the clang driver as-is right now, in the sense that LLVM flags control the behaviour of LLVM internal functions, and those functions are invoked by the driver, but there's no direct use of those flags from the driver.

Interesting. IMHO, that should not be allowed - it means that the internals of LLVM leak higher up the compilation stack. Flang will complain if the compiler is invoked with an unrecognised flag (-write-experimental-debuginf would qualify as such). There's a bit of context at the bottom of my comment.

Does that sound like the right approach to you? Resulting change is something like:

Yes, that makes sense to me. Not sure why we didn't use llvm::PrintModulePass to begin with. Looks like a relatively new addition to LLVM, so perhaps it wasn't available when we worked on this in Flang? 🤔

Btw, if you want to pass a flag to LLVM, use -mllvm ;-) In fact, that should be sufficient for what you are trying to achieve here?

Context

Since roughly ~1yr ago, we have this concept of "visibility" when defining compiler flags:

At that point, I made Flang reject all compilation flags that are not supported

That was primarily to avoid a situations where people get unexpected behaviour (e.g. "hey, I am passing this flag and nothing happens"). That also helps us quickly identify flags that are missing (if the compiler was to ignore unsupported flags, it would hard to tell what's supported and what is not). Admittedly, it can be a nuisance at times, but so far we've managed to support all users that raised issues related to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. IMHO, that should not be allowed - it means that the internals of LLVM leak higher up the compilation stack. Flang will complain if the compiler is invoked with an unrecognised flag (-write-experimental-debuginf would qualify as such). There's a bit of context at the bottom of my comment.

Ah, I think there's a terminology mismatch here - I do mean that the flag is being passed under -llvm, to an invocation of -fc1, and that affects the results of LLVM internal behaviour invoked by the frontend, which is what I'm going for here. The patch here seems to work, so this is probably the right way: #95142

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this makes more sense now 😅


/// Save the given \c mlirModule to a temporary .mlir file, in a location
/// decided by the -save-temps flag. No files are produced if the flag is not
/// specified.
Expand Down Expand Up @@ -1271,6 +1274,12 @@ void CodeGenAction::executeAction() {
runOptimizationPipeline(ci.isOutputStreamNull() ? *os : ci.getOutputStream());

if (action == BackendActionTy::Backend_EmitLL) {
// When printing LLVM IR, we should convert the module to the debug info
// format that LLVM expects us to print.
llvm::ScopedDbgInfoFormatSetter FormatSetter(*llvmModule,
WriteNewDbgInfoFormat);
if (WriteNewDbgInfoFormat)
llvmModule->removeDebugIntrinsicDeclarations();
Comment on lines +1279 to +1282
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO you want to put a comment here briefly indicating the purpose, and when it can be removed (this is all transitory, right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite transitory - it will eventually go, but this should remain for long as we support intrinsics at all, the same as all other code that selects the debug info format.

llvmModule->print(ci.isOutputStreamNull() ? *os : ci.getOutputStream(),
/*AssemblyAnnotationWriter=*/nullptr);
return;
Expand Down
10 changes: 10 additions & 0 deletions mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ using namespace mlir;
using namespace mlir::LLVM;
using namespace mlir::LLVM::detail;

extern llvm::cl::opt<bool> UseNewDbgInfoFormat;

#include "mlir/Dialect/LLVMIR/LLVMConversionEnumsToLLVM.inc"

namespace {
Expand Down Expand Up @@ -1789,6 +1791,9 @@ prepareLLVMModule(Operation *m, llvm::LLVMContext &llvmContext,
StringRef name) {
m->getContext()->getOrLoadDialect<LLVM::LLVMDialect>();
auto llvmModule = std::make_unique<llvm::Module>(name, llvmContext);
// ModuleTranslation can currently only construct modules in the old debug
// info format, so set the flag accordingly.
llvmModule->setNewDbgInfoFormatFlag(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment directing people to the RemoveDIs page would be good, we can assure readers that the meaning of the debug-info is the same, only the format is different.

if (auto dataLayoutAttr =
m->getDiscardableAttr(LLVM::LLVMDialect::getDataLayoutAttrName())) {
llvmModule->setDataLayout(cast<StringAttr>(dataLayoutAttr).getValue());
Expand Down Expand Up @@ -1867,6 +1872,11 @@ mlir::translateModuleToLLVMIR(Operation *module, llvm::LLVMContext &llvmContext,
if (failed(translator.convertFunctions()))
return nullptr;

// Once we've finished constructing elements in the module, we should convert
// it to use the debug info format desired by LLVM.
// See https://llvm.org/docs/RemoveDIsDebugInfo.html
translator.llvmModule->setIsNewDbgInfoFormat(UseNewDbgInfoFormat);

if (!disableVerification &&
llvm::verifyModule(*translator.llvmModule, &llvm::errs()))
return nullptr;
Expand Down
Loading