[mlir][llvm] Dominance violating debug intrinsic import

[Dinistro]

Summary

Debug intrinsics are allowed to violate SSA dominance and might thus
cause the LLVM import to produce invalid LLVM dialect. This commit
ensures that the debug intrinsics are emitted right after the definition
of their SSA operands.
As the position of debug intrinsics has no meaning, changing it has no
semantic implication.

[gysit]

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

554

Let's maybe a add:

// TODO: Implement a two pass solution that translates the debug intrinsics after the entire function as been translated.

556–557

mlir/test/Target/LLVMIR/Import/debug-info.ll

379

nit: Can you add some checks that show the intrinsic is imported right after the getelementptr?

[Dinistro]

address reviewer comments

[Dinistro]

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

556–557

Whups

[gysit]

Thanks LGTM!

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

let mlirBuilder = [{
     // when attempting to convert an argument list.
 
     FailureOr<Value> argOperand = moduleImport.convertMetadataValue(llvmOperands[0]);
     // Drop the intrinsic when its operand could not be converted. This can
     // happen for use before definition cases that are allowed for debug
     // intrinsics.
     if (failed(argOperand))
       return success();
+
+    // Ensure that the debug instrinsic is inserted right after its operand is
+    // defined. Otherwise, the operand might not necessarily dominate the
+    // intrinsic.
+    OpBuilder::InsertionGuard guard($_builder);
+    //odsBuilder.setInsertionPointToEnd(argOperand->getParentBlock());
+    odsBuilder.setInsertionPointAfterValue(*argOperand);
     $_op = $_builder.create<$_qualCppClassName>($_location,
         *argOperand, $_var_attr($varInfo));
   }];
   let assemblyFormat = [{
     qualified($varInfo) `=` $}] # argName #
       [{ `:` qualified(type($}] # argName # [{)) attr-dict
   }];
 }

mlir/test/Target/LLVMIR/Import/debug-info.ll

 ; // -----
 
 ; Verify the module location is set to the source filename.
 ; CHECK: loc("debug-info.ll":0:0)
 source_filename = "debug-info.ll"
 
 ; // -----
 
+; NOTE: The debug intrinsics are reordered as a side-effect of the dominance-
+;       preserving measures needed to import LLVM IR.
+
 ; CHECK: #[[$SP:.+]] = #llvm.di_subprogram<
-; CHECK: #[[$VAR0:.+]] = #llvm.di_local_variable<scope = #[[$SP]], name = "arg", file = #{{.*}}, line = 1, arg = 1, alignInBits = 32, type = #{{.*}}>
 ; CHECK: #[[$VAR1:.+]] = #llvm.di_local_variable<scope = #[[$SP]], name = "arg">
+; CHECK: #[[$VAR0:.+]] = #llvm.di_local_variable<scope = #[[$SP]], name = "arg", file = #{{.*}}, line = 1, arg = 1, alignInBits = 32, type = #{{.*}}>
 
 ; CHECK-LABEL: @intrinsic
 ; CHECK-SAME:  %[[ARG0:[a-zA-Z0-9]+]]
 ; CHECK-SAME:  %[[ARG1:[a-zA-Z0-9]+]]
 define void @intrinsic(i64 %0, ptr %1) {
+  ; CHECK: llvm.intr.dbg.declare #[[$VAR1]] = %[[ARG1]] : !llvm.ptr loc(#[[LOC1:.+]])
   ; CHECK: llvm.intr.dbg.value #[[$VAR0]] = %[[ARG0]] : i64 loc(#[[LOC0:.+]])
   call void @llvm.dbg.value(metadata i64 %0, metadata !5, metadata !DIExpression()), !dbg !7
-  ; CHECK: llvm.intr.dbg.declare #[[$VAR1]] = %[[ARG1]] : !llvm.ptr loc(#[[LOC1:.+]])
   call void @llvm.dbg.declare(metadata ptr %1, metadata !6, metadata !DIExpression()), !dbg !9
   ret void
 }
 
-; CHECK: #[[LOC0]] = loc(fused<#[[$SP]]>[{{.*}}])
 ; CHECK: #[[LOC1]] = loc(fused<#[[$SP]]>[{{.*}}])
+; CHECK: #[[LOC0]] = loc(fused<#[[$SP]]>[{{.*}}])
 
 declare void @llvm.dbg.value(metadata, metadata, metadata)
 declare void @llvm.dbg.declare(metadata, metadata, metadata)
 
 !llvm.dbg.cu = !{!1}
 !llvm.module.flags = !{!0}
 !0 = !{i32 2, !"Debug Info Version", i32 3}
 !1 = distinct !DICompileUnit(language: DW_LANG_C, file: !2)

 !5 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !3, flags: DIFlagArtificial | DIFlagObjectPointer)
 !6 = !DIDerivedType(tag: DW_TAG_member, name: "call_field", file: !2, baseType: !5)
 !7 = !DILocalVariable(scope: !8, name: "var", file: !2, type: !5);
 !8 = distinct !DISubprogram(name: "dbg_use_before_def", scope: !2, file: !2, spFlags: DISPFlagDefinition, unit: !1)
 !9 = !DILocation(line: 1, column: 2, scope: !8)
 
 ; // -----
 
+; This test checks that broken dominance doesn't break the metadata import.
+
+define void @dbg_broken_dominance(ptr %arg, i1 %cond) {
+  br i1 %cond, label %b1, label %b2
+b1:
+  br label %b3
+b2:
+  %dbg_arg = getelementptr double, ptr %arg, i64 16
+  br label %b3
+b3:
+  call void @llvm.dbg.value(metadata ptr %dbg_arg, metadata !7, metadata !DIExpression()), !dbg !9
+  ret void
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!1}
+!llvm.module.flags = !{!0}
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_C, file: !2)
+!2 = !DIFile(filename: "debug-info.ll", directory: "/")
+!3 = !DICompositeType(tag: DW_TAG_class_type, name: "class_field", file: !2, line: 42, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !4)
+!4 = !{!6}
+!5 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !3, flags: DIFlagArtificial | DIFlagObjectPointer)
+!6 = !DIDerivedType(tag: DW_TAG_member, name: "call_field", file: !2, baseType: !5)
+!7 = !DILocalVariable(scope: !8, name: "var", file: !2, type: !5);
+!8 = distinct !DISubprogram(name: "dbg_use_before_def", scope: !2, file: !2, spFlags: DISPFlagDefinition, unit: !1)
+!9 = !DILocation(line: 1, column: 2, scope: !8)
+
+; // -----
+
 ; CHECK-DAG: #[[NAMESPACE:.+]] = #llvm.di_namespace<name = "std", exportSymbols = false>
 ; CHECK-DAG: #[[SUBPROGRAM:.+]] =  #llvm.di_subprogram<compileUnit = #{{.*}}, scope = #[[NAMESPACE]], name = "namespace"
 
 define void @namespace(ptr %arg) {
   call void @llvm.dbg.value(metadata ptr %arg, metadata !7, metadata !DIExpression()), !dbg !9
   ret void
 }