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

SLTozer · 2024-06-11T11:42:23Z

MLIR's LLVM dialect does not internally support debug records, only converting to/from debug intrinsics. To smooth the transition from intrinsics to records, there is a step prior to IR->MLIR translation that switches the IR module to intrinsic-form; this patch adds the equivalent conversion to record-form at MLIR->IR translation, and also modifies the flang front end to use the WriteNewDbgInfoFormat flag when it is emitting LLVM IR.

llvmbot · 2024-06-11T11:42:53Z

@llvm/pr-subscribers-mlir-llvm
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-flang-driver

Author: Stephen Tozer (SLTozer)

Changes

MLIR's LLVM dialect does not internally support debug records, only converting to/from debug intrinsics. To smooth the transition from intrinsics to records, there is a step prior to IR->MLIR translation that switches the IR module to intrinsic-form; this patch adds the equivalent conversion to record-form at MLIR->IR translation, and also modifies the flang front end to use the WriteNewDbgInfoFormat flag when it is emitting LLVM IR.

Full diff: https://github.com/llvm/llvm-project/pull/95098.diff

2 Files Affected:

(modified) flang/lib/Frontend/FrontendActions.cpp (+7)
(modified) mlir/lib/Target/LLVMIR/ModuleTranslation.cpp (+5)

diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index b1b6391f1439c..585177b5c7647 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -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"
@@ -81,6 +82,8 @@ using namespace Fortran::frontend;
   llvm::PassPluginLibraryInfo get##Ext##PluginInfo();
 #include "llvm/Support/Extension.def"
 
+extern llvm::cl::opt<bool> WriteNewDbgInfoFormat;
+
 /// 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.
@@ -1271,6 +1274,10 @@ void CodeGenAction::executeAction() {
   runOptimizationPipeline(ci.isOutputStreamNull() ? *os : ci.getOutputStream());
 
   if (action == BackendActionTy::Backend_EmitLL) {
+    llvm::ScopedDbgInfoFormatSetter FormatSetter(*llvmModule,
+                                                 WriteNewDbgInfoFormat);
+    if (WriteNewDbgInfoFormat)
+      llvmModule->removeDebugIntrinsicDeclarations();
     llvmModule->print(ci.isOutputStreamNull() ? *os : ci.getOutputStream(),
                       /*AssemblyAnnotationWriter=*/nullptr);
     return;
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index 7b86b250c294b..a5c8c26483edf 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -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 {
@@ -1789,6 +1791,7 @@ prepareLLVMModule(Operation *m, llvm::LLVMContext &llvmContext,
                   StringRef name) {
   m->getContext()->getOrLoadDialect<LLVM::LLVMDialect>();
   auto llvmModule = std::make_unique<llvm::Module>(name, llvmContext);
+  llvmModule->setNewDbgInfoFormatFlag(false);
   if (auto dataLayoutAttr =
           m->getDiscardableAttr(LLVM::LLVMDialect::getDataLayoutAttrName())) {
     llvmModule->setDataLayout(cast<StringAttr>(dataLayoutAttr).getValue());
@@ -1867,6 +1870,8 @@ mlir::translateModuleToLLVMIR(Operation *module, llvm::LLVMContext &llvmContext,
   if (failed(translator.convertFunctions()))
     return nullptr;
 
+  translator.llvmModule->setIsNewDbgInfoFormat(UseNewDbgInfoFormat);
+
   if (!disableVerification &&
       llvm::verifyModule(*translator.llvmModule, &llvm::errs()))
     return nullptr;

jmorse

Given that this is just adding our debug-info-transition calls a the point where IR enters/exits, this seems fine, I reckon we could add some more comments to assist readers, see inline.

jmorse · 2024-06-11T11:46:29Z

flang/lib/Frontend/FrontendActions.cpp

+    llvm::ScopedDbgInfoFormatSetter FormatSetter(*llvmModule,
+                                                 WriteNewDbgInfoFormat);
+    if (WriteNewDbgInfoFormat)
+      llvmModule->removeDebugIntrinsicDeclarations();


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

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.

jmorse · 2024-06-11T11:47:42Z

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

@@ -1789,6 +1791,7 @@ prepareLLVMModule(Operation *m, llvm::LLVMContext &llvmContext,
                  StringRef name) {
  m->getContext()->getOrLoadDialect<LLVM::LLVMDialect>();
  auto llvmModule = std::make_unique<llvm::Module>(name, llvmContext);
+  llvmModule->setNewDbgInfoFormatFlag(false);


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.

…anslation (#95098)" Reverted due to failure on buildbot due to missing use of the WriteNewDbgInfoFormat flag in MLIR. This reverts commit ca920bb.

SLTozer · 2024-06-11T12:43:43Z

I ran the lit tests previously, but didn't rebuild all the MLIR binaries first - updated the patch with some more conversion sites and confirmed check-flang and check-mlir pass this time!

…ranslation (#95098)" Reapplies the original patch with some additional conversion layers added to the MLIR translator, to ensure that we don't write the new debug info format unless WriteNewDbgInfoFormat is set. This reverts commit 8c5d9c7.

Following from the previous commit, this patch converts to the appropriate debug info format before printing LLVM IR. See: #95098

banach-space

Why aren't any tests added to this PR?

banach-space · 2024-06-11T14:00:32Z

flang/lib/Frontend/FrontendActions.cpp

@@ -81,6 +82,8 @@ using namespace Fortran::frontend;
  llvm::PassPluginLibraryInfo get##Ext##PluginInfo();
 #include "llvm/Support/Extension.def"

+extern llvm::cl::opt<bool> WriteNewDbgInfoFormat;


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

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?

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.

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; }

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:

https://reviews.llvm.org/D157150.

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

https://reviews.llvm.org/D157837.

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.

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

Right, this makes more sense now 😅

banach-space · 2024-06-11T14:07:40Z

If I read GitHub logs correctly, this was uploaded+reviewed+merged ~2hrs ago, i.e. within a very narrow time window. Please, could you allow more time for reviews next time? We are distributed throughout the world and start work at different times, 2hrs is a very very narrow window, even for 1 time-zone.

SLTozer · 2024-06-11T14:12:56Z

Why aren't any tests added to this PR?

A further patch that I intend(ed) to land very shortly afterwards would update the state of the flags would lead to this getting coverage, but I'm happy to pull this back to make sure the patch itself has proper test coverage.

Please, could you allow more time for reviews next time?

Apologies - I'll revert the patch for now, and reopen the review for a longer period of time before landing. This looked like a hotfix situation to me based on similar changes elsewhere, but based on your comment about controlling the driver, this probably needs a slightly more involved fix.

…translation (#95098)" Also reverts "[MLIR][Flang][DebugInfo] Convert debug format in MLIR translators" The patch above introduces behaviour controlled by an LLVM flag into the Flang driver, which is incorrect behaviour. This reverts commits: 3cc2710. 460408f.

llvm#95098) MLIR's LLVM dialect does not internally support debug records, only converting to/from debug intrinsics. To smooth the transition from intrinsics to records, there is a step prior to IR->MLIR translation that switches the IR module to intrinsic-form; this patch adds the equivalent conversion to record-form at MLIR->IR translation, and also modifies the flang front end to use the WriteNewDbgInfoFormat flag when it is emitting LLVM IR.

…anslation (llvm#95098)" Reverted due to failure on buildbot due to missing use of the WriteNewDbgInfoFormat flag in MLIR. This reverts commit ca920bb.

MLIR's LLVM dialect does not internally support debug records, only converting to/from debug intrinsics. To smooth the transition from intrinsics to records, there is a step prior to IR->MLIR translation that switches the IR module to intrinsic-form; this patch adds the equivalent conversion to record-form at MLIR->IR translation. This is a partial reapply of #95098 which can be landed once the flang frontend has been updated by #95306. This is the counterpart to the earlier patch #89735 which handled the IR->MLIR conversion.

…#95329) MLIR's LLVM dialect does not internally support debug records, only converting to/from debug intrinsics. To smooth the transition from intrinsics to records, there is a step prior to IR->MLIR translation that switches the IR module to intrinsic-form; this patch adds the equivalent conversion to record-form at MLIR->IR translation. This is a partial reapply of llvm#95098 which can be landed once the flang frontend has been updated by llvm#95306. This is the counterpart to the earlier patch llvm#89735 which handled the IR->MLIR conversion.

SLTozer requested review from jmorse, abidh, fabianmcg, jeanPerier and OCHyams June 11, 2024 11:42

SLTozer self-assigned this Jun 11, 2024

llvmbot added mlir:llvm flang:driver mlir flang Flang issues not falling into any other category labels Jun 11, 2024

jmorse approved these changes Jun 11, 2024

View reviewed changes

Add comments for context

585fca5

SLTozer merged commit ca920bb into llvm:main Jun 11, 2024
4 of 6 checks passed

SLTozer added a commit that referenced this pull request Jun 11, 2024

[MLIR][Flang][DebugInfo] Convert debug format in MLIR translators

3cc2710

Following from the previous commit, this patch converts to the appropriate debug info format before printing LLVM IR. See: #95098

banach-space reviewed Jun 11, 2024

View reviewed changes

SLTozer mentioned this pull request Jun 12, 2024

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

Merged

HerrCai0907 mentioned this pull request Jun 13, 2024

tidy #95384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SLTozer commented Jun 11, 2024

llvmbot commented Jun 11, 2024 •

edited

Loading

jmorse left a comment

jmorse Jun 11, 2024

SLTozer Jun 11, 2024

jmorse Jun 11, 2024

SLTozer commented Jun 11, 2024 •

edited

Loading

banach-space left a comment

banach-space Jun 11, 2024

SLTozer Jun 11, 2024

banach-space Jun 11, 2024

SLTozer Jun 11, 2024 •

edited

Loading

banach-space Jun 11, 2024

SLTozer Jun 11, 2024

banach-space Jun 11, 2024

banach-space commented Jun 11, 2024

SLTozer commented Jun 11, 2024 •

edited

Loading

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

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

Conversation

SLTozer commented Jun 11, 2024

llvmbot commented Jun 11, 2024 • edited Loading

jmorse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SLTozer commented Jun 11, 2024 • edited Loading

banach-space left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SLTozer Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banach-space commented Jun 11, 2024

SLTozer commented Jun 11, 2024 • edited Loading

llvmbot commented Jun 11, 2024 •

edited

Loading

SLTozer commented Jun 11, 2024 •

edited

Loading

SLTozer Jun 11, 2024 •

edited

Loading

SLTozer commented Jun 11, 2024 •

edited

Loading