[llvm] [RemoveDIs] Add a 'BeforeDbgRecords' parameter to C API isnt insertion functions (PR #92417)
via llvm-commits
llvm-commits at lists.llvm.org
Thu May 16 08:44:40 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-ir
Author: Orlando Cazalet-Hyams (OCHyams)
<details>
<summary>Changes</summary>
Add a new parameter `LLVMBool BeforeDbgRecords` to `LLVMPositionBuilderBefore`
and `LLVMPositionBuilder` in `llvm/include/llvm-c/Core.h`. It determines whether
the insertion point is before debug records attached to `Instr`.
More info on debug records and the migration towards using them can be found
here: https://llvm.org/docs/RemoveDIsDebugInfo.html
The distinction is important in some situations. An important example is when
inserting a phi before another instruction which has debug records attached to
it (these come "before" the instruction). Inserting before the instruction but
after the debug records would result in having debug records before a phi,
which is illegal. That results in an assertion failure:
`llvm/lib/IR/Instruction.cpp:166: Assertion '!isa<PHINode>(this) && "Inserting PHI after debug-records!"' failed.`
In llvm (C++) we've added bit to instruction iterators that carries around the
extra information. Adding a bool to two functions seemed like the least
invasive and least suprising way to update the C API.
The new PHI is tested in llvm/tools/llvm-c-test/debuginfo.c.
Update the OCaml bindings, the migration docs and release notes.
---
Full diff: https://github.com/llvm/llvm-project/pull/92417.diff
12 Files Affected:
- (modified) llvm/bindings/ocaml/llvm/llvm.ml (+8-8)
- (modified) llvm/bindings/ocaml/llvm/llvm.mli (+16-9)
- (modified) llvm/bindings/ocaml/llvm/llvm_ocaml.c (+3-3)
- (modified) llvm/docs/ReleaseNotes.rst (+5)
- (modified) llvm/docs/RemoveDIsDebugInfo.md (+10)
- (modified) llvm/include/llvm-c/Core.h (+9-2)
- (modified) llvm/lib/IR/Core.cpp (+7-3)
- (modified) llvm/test/Bindings/OCaml/core.ml (+2-2)
- (modified) llvm/test/Bindings/OCaml/debuginfo.ml (+1-1)
- (modified) llvm/test/Bindings/llvm-c/debug_info.ll (+3)
- (modified) llvm/test/Bindings/llvm-c/debug_info_new_format.ll (+3)
- (modified) llvm/tools/llvm-c-test/debuginfo.c (+18-1)
``````````diff
diff --git a/llvm/bindings/ocaml/llvm/llvm.ml b/llvm/bindings/ocaml/llvm/llvm.ml
index 003fd750cd9f8..f339ea1bae1a4 100644
--- a/llvm/bindings/ocaml/llvm/llvm.ml
+++ b/llvm/bindings/ocaml/llvm/llvm.ml
@@ -1137,22 +1137,22 @@ external delete_instruction : llvalue -> unit = "llvm_delete_instruction"
(*===-- Instruction builders ----------------------------------------------===*)
external builder : llcontext -> llbuilder = "llvm_builder"
-external position_builder : (llbasicblock, llvalue) llpos -> llbuilder -> unit
- = "llvm_position_builder"
+external position_builder : (llbasicblock, llvalue) llpos -> bool -> llbuilder ->
+ unit = "llvm_position_builder"
external insertion_block : llbuilder -> llbasicblock = "llvm_insertion_block"
external insert_into_builder : llvalue -> string -> llbuilder -> unit
= "llvm_insert_into_builder"
-let builder_at context ip =
+let builder_at context ip before_dbg_records =
let b = builder context in
- position_builder ip b;
+ position_builder ip before_dbg_records b;
b
-let builder_before context i = builder_at context (Before i)
-let builder_at_end context bb = builder_at context (At_end bb)
+let builder_before context i before_dbg_records = builder_at context (Before i) before_dbg_records
+let builder_at_end context bb = builder_at context (At_end bb) false
-let position_before i = position_builder (Before i)
-let position_at_end bb = position_builder (At_end bb)
+let position_before i before_dbg_records = position_builder (Before i) before_dbg_records
+let position_at_end bb = position_builder (At_end bb) false
(*--... Metadata ...........................................................--*)
diff --git a/llvm/bindings/ocaml/llvm/llvm.mli b/llvm/bindings/ocaml/llvm/llvm.mli
index 93540c619efba..e2999fae656b2 100644
--- a/llvm/bindings/ocaml/llvm/llvm.mli
+++ b/llvm/bindings/ocaml/llvm/llvm.mli
@@ -1867,26 +1867,33 @@ val incoming : llvalue -> (llvalue * llbasicblock) list
for [llvm::LLVMBuilder]. *)
val builder : llcontext -> llbuilder
-(** [builder_at ip] creates an instruction builder positioned at [ip].
+(** [builder_at ip before_dbg_records] creates an instruction builder
+ positioned at [ip]. [before_dbg_records] determines whether the insertion
+ point is before debug records attached to [ip].
See the constructor for [llvm::LLVMBuilder]. *)
-val builder_at : llcontext -> (llbasicblock, llvalue) llpos -> llbuilder
+val builder_at : llcontext -> (llbasicblock, llvalue) llpos ->
+ bool -> llbuilder
-(** [builder_before ins] creates an instruction builder positioned before the
- instruction [isn]. See the constructor for [llvm::LLVMBuilder]. *)
-val builder_before : llcontext -> llvalue -> llbuilder
+(** [builder_before ins before_dbg_records] creates an instruction builder
+ positioned before the instruction [isn]. [before_dbg_records] determines
+ whether the insertion point is before debug records attached to [isn].
+ See the constructor for [llvm::LLVMBuilder]. *)
+val builder_before : llcontext -> llvalue -> bool -> llbuilder
(** [builder_at_end bb] creates an instruction builder positioned at the end of
the basic block [bb]. See the constructor for [llvm::LLVMBuilder]. *)
val builder_at_end : llcontext -> llbasicblock -> llbuilder
-(** [position_builder ip bb] moves the instruction builder [bb] to the position
- [ip].
+(** [position_builder ip bb before_dbg_records] moves the instruction builder
+ [bb] to the position [ip]. [before_dbg_records] determines whether the
+ insertion point is before debug records attached to [ip].
See the constructor for [llvm::LLVMBuilder]. *)
-val position_builder : (llbasicblock, llvalue) llpos -> llbuilder -> unit
+val position_builder : (llbasicblock, llvalue) llpos -> bool -> llbuilder ->
+ unit
(** [position_before ins b] moves the instruction builder [b] to before the
instruction [isn]. See the method [llvm::LLVMBuilder::SetInsertPoint]. *)
-val position_before : llvalue -> llbuilder -> unit
+val position_before : llvalue -> bool -> llbuilder -> unit
(** [position_at_end bb b] moves the instruction builder [b] to the end of the
basic block [bb]. See the method [llvm::LLVMBuilder::SetInsertPoint]. *)
diff --git a/llvm/bindings/ocaml/llvm/llvm_ocaml.c b/llvm/bindings/ocaml/llvm/llvm_ocaml.c
index 6d08d78b84455..cb0f6f8ce71c9 100644
--- a/llvm/bindings/ocaml/llvm/llvm_ocaml.c
+++ b/llvm/bindings/ocaml/llvm/llvm_ocaml.c
@@ -2016,14 +2016,14 @@ value llvm_builder(value C) {
return alloc_builder(LLVMCreateBuilderInContext(Context_val(C)));
}
-/* (llbasicblock, llvalue) llpos -> llbuilder -> unit */
-value llvm_position_builder(value Pos, value B) {
+/* (llbasicblock, llvalue) llpos -> bool -> llbuilder -> unit */
+value llvm_position_builder(value Pos, value BeforeDbgRecords, value B) {
if (Tag_val(Pos) == 0) {
LLVMBasicBlockRef BB = BasicBlock_val(Field(Pos, 0));
LLVMPositionBuilderAtEnd(Builder_val(B), BB);
} else {
LLVMValueRef I = Value_val(Field(Pos, 0));
- LLVMPositionBuilderBefore(Builder_val(B), I);
+ LLVMPositionBuilderBefore(Builder_val(B), I, Bool_val(BeforeDbgRecords));
}
return Val_unit;
}
diff --git a/llvm/docs/ReleaseNotes.rst b/llvm/docs/ReleaseNotes.rst
index 84320461fa9e1..37e2827ed76cd 100644
--- a/llvm/docs/ReleaseNotes.rst
+++ b/llvm/docs/ReleaseNotes.rst
@@ -180,6 +180,11 @@ Changes to the C API
* ``LLVMGetCallBrNumIndirectDests``
* ``LLVMGetCallBrIndirectDest``
+* Modified the following functions, adding a new `BeforeDbgRecords` parameter which indicates whether or not the insertion position should also be before the debug records that precede the instruction. See the `debug info migration guide <https://llvm.org/docs/RemoveDIsDebugInfo.html>`_ for more info.
+
+ * ``LLVMPositionBuilder``
+ * ``LLVMPositionBuilderBefore``
+
Changes to the CodeGen infrastructure
-------------------------------------
diff --git a/llvm/docs/RemoveDIsDebugInfo.md b/llvm/docs/RemoveDIsDebugInfo.md
index 9e50a2a604aa6..7036ead9e092c 100644
--- a/llvm/docs/RemoveDIsDebugInfo.md
+++ b/llvm/docs/RemoveDIsDebugInfo.md
@@ -56,8 +56,18 @@ LLVMDIBuilderInsertDeclareBefore # Insert a debug record (new debug info forma
LLVMDIBuilderInsertDeclareAtEnd # Same as above.
LLVMDIBuilderInsertDbgValueBefore # Same as above.
LLVMDIBuilderInsertDbgValueAtEnd # Same as above.
+
+Existing functions (new parameter)
+----------------------------------
+LLVMPositionBuilder # Added BeforeDbgRecords parameter. See info below.
+LLVMPositionBuilderBefore # Same as above.
```
+`LLVMPositionBuilder` and `LLVMPositionBuilderBefore` have gained a parameter `BeforeDbgRecords` which indicates whether or not the insertion position should also be before the debug records that precede the instruction. Note that this doesn't mean that debug intrinsics before the chosen instruction are skipped, only debug records (which unlike debug records are not themselves instructions).
+
+If you don't know whether it should be true or false then follow this rule:
+If you are trying to insert at the start of a block, or purposfully skip debug intrinsics to determine the insertion point for any other reason, then set it to true. Otherwise set it to false.
+
# Anything else?
Not really, but here's an "old vs new" comparison of how to do certain things and quickstart for how this "new" debug info is structured.
diff --git a/llvm/include/llvm-c/Core.h b/llvm/include/llvm-c/Core.h
index 9d09546513f0e..12f9b5e2e3cd2 100644
--- a/llvm/include/llvm-c/Core.h
+++ b/llvm/include/llvm-c/Core.h
@@ -3952,14 +3952,21 @@ const unsigned *LLVMGetIndices(LLVMValueRef Inst);
* An instruction builder represents a point within a basic block and is
* the exclusive means of building instructions using the C interface.
*
+ * Some insertion-position-setting methods have a "BeforeDbgRecords"
+ * parameter. Set to true if the insertion point should be in front of debug
+ * records, for example when inserting a phi. If you are trying to insert at the
+ * start of a block, or purposfully skip debug intrinsics to determine the
+ * insertion point for any other reason, then set it to true. Otherwise set it
+ * to false.
* @{
*/
LLVMBuilderRef LLVMCreateBuilderInContext(LLVMContextRef C);
LLVMBuilderRef LLVMCreateBuilder(void);
void LLVMPositionBuilder(LLVMBuilderRef Builder, LLVMBasicBlockRef Block,
- LLVMValueRef Instr);
-void LLVMPositionBuilderBefore(LLVMBuilderRef Builder, LLVMValueRef Instr);
+ LLVMValueRef Instr, LLVMBool BeforeDbgRecords);
+void LLVMPositionBuilderBefore(LLVMBuilderRef Builder, LLVMValueRef Instr,
+ LLVMBool BeforeDbgRecords);
void LLVMPositionBuilderAtEnd(LLVMBuilderRef Builder, LLVMBasicBlockRef Block);
LLVMBasicBlockRef LLVMGetInsertBlock(LLVMBuilderRef Builder);
void LLVMClearInsertionPosition(LLVMBuilderRef Builder);
diff --git a/llvm/lib/IR/Core.cpp b/llvm/lib/IR/Core.cpp
index df90b88341123..6e97a4222a7dc 100644
--- a/llvm/lib/IR/Core.cpp
+++ b/llvm/lib/IR/Core.cpp
@@ -3158,15 +3158,19 @@ LLVMBuilderRef LLVMCreateBuilder(void) {
}
void LLVMPositionBuilder(LLVMBuilderRef Builder, LLVMBasicBlockRef Block,
- LLVMValueRef Instr) {
+ LLVMValueRef Instr, LLVMBool BeforeDbgRecords) {
BasicBlock *BB = unwrap(Block);
auto I = Instr ? unwrap<Instruction>(Instr)->getIterator() : BB->end();
+ I.setHeadBit(BeforeDbgRecords);
unwrap(Builder)->SetInsertPoint(BB, I);
}
-void LLVMPositionBuilderBefore(LLVMBuilderRef Builder, LLVMValueRef Instr) {
+void LLVMPositionBuilderBefore(LLVMBuilderRef Builder, LLVMValueRef Instr,
+ LLVMBool BeforeDbgRecords) {
Instruction *I = unwrap<Instruction>(Instr);
- unwrap(Builder)->SetInsertPoint(I->getParent(), I->getIterator());
+ BasicBlock::iterator It = I->getIterator();
+ It.setHeadBit(BeforeDbgRecords);
+ unwrap(Builder)->SetInsertPoint(I->getParent(), It);
}
void LLVMPositionBuilderAtEnd(LLVMBuilderRef Builder, LLVMBasicBlockRef Block) {
diff --git a/llvm/test/Bindings/OCaml/core.ml b/llvm/test/Bindings/OCaml/core.ml
index 64bfa8ee412df..cd3e87c00628c 100644
--- a/llvm/test/Bindings/OCaml/core.ml
+++ b/llvm/test/Bindings/OCaml/core.ml
@@ -832,7 +832,7 @@ let test_instructions () =
let fty = function_type void_type [| i32_type; i32_type |] in
let f = define_function "f" fty m in
let bb = entry_block f in
- let b = builder_at context (At_end bb) in
+ let b = builder_at context (At_end bb) false in
insist (At_end bb = instr_begin bb);
insist (At_start bb = instr_end bb);
@@ -1152,7 +1152,7 @@ let test_builder () =
(* CHECK: ret{{.*}}P1
*)
let ret = build_ret p1 atentry in
- position_before ret atentry
+ position_before ret false atentry
end;
(* see test/Feature/exception.ll *)
diff --git a/llvm/test/Bindings/OCaml/debuginfo.ml b/llvm/test/Bindings/OCaml/debuginfo.ml
index f95800dfcb025..4f779743adaa0 100644
--- a/llvm/test/Bindings/OCaml/debuginfo.ml
+++ b/llvm/test/Bindings/OCaml/debuginfo.ml
@@ -273,7 +273,7 @@ let test_variables f dibuilder file_di fun_di =
stdout_metadata auto_var;
(* CHECK: [[LOCAL_VAR_PTR:<0x[0-9a-f]*>]] = !DILocalVariable(name: "my_local", scope: <{{0x[0-9a-f]*}}>, file: <{{0x[0-9a-f]*}}>, line: 10, type: [[INT64TY_PTR]])
*)
- let builder = Llvm.builder_before context entry_term in
+ let builder = Llvm.builder_before context entry_term false in
let all = Llvm.build_alloca (Llvm.i64_type context) "my_alloca" builder in
let scope =
Llvm_debuginfo.dibuild_create_lexical_block dibuilder ~scope:fun_di
diff --git a/llvm/test/Bindings/llvm-c/debug_info.ll b/llvm/test/Bindings/llvm-c/debug_info.ll
index 9358bac59bd21..71986fbb2348f 100644
--- a/llvm/test/Bindings/llvm-c/debug_info.ll
+++ b/llvm/test/Bindings/llvm-c/debug_info.ll
@@ -10,7 +10,10 @@
; CHECK-NEXT: call void @llvm.dbg.declare(metadata i64 0, metadata !40, metadata !DIExpression()), !dbg !43
; CHECK-NEXT: br label %vars
; CHECK: vars:
+; CHECK-NEXT: %p1 = phi i64 [ 0, %entry ]
+; CHECK-NEXT: %p2 = phi i64 [ 0, %entry ]
; CHECK-NEXT: call void @llvm.dbg.value(metadata i64 0, metadata !41, metadata !DIExpression(DW_OP_constu, 0, DW_OP_stack_value)), !dbg !44
+; CHECK-NEXT: %a = add i64 %p1, %p2
; CHECK-NEXT: ret i64 0
; CHECK-NEXT: }
diff --git a/llvm/test/Bindings/llvm-c/debug_info_new_format.ll b/llvm/test/Bindings/llvm-c/debug_info_new_format.ll
index 05b6ef4de9adb..1b6f2c4ea403a 100644
--- a/llvm/test/Bindings/llvm-c/debug_info_new_format.ll
+++ b/llvm/test/Bindings/llvm-c/debug_info_new_format.ll
@@ -11,7 +11,10 @@
; CHECK-NEXT: #dbg_declare(i64 0, !40, !DIExpression(), !43)
; CHECK-NEXT: br label %vars
; CHECK: vars:
+; CHECK-NEXT: %p1 = phi i64 [ 0, %entry ]
+; CHECK-NEXT: %p2 = phi i64 [ 0, %entry ]
; CHECK-NEXT: #dbg_value(i64 0, !41, !DIExpression(DW_OP_constu, 0, DW_OP_stack_value), !44)
+; CHECK-NEXT: %a = add i64 %p1, %p2
; CHECK-NEXT: ret i64 0
; CHECK-NEXT: }
diff --git a/llvm/tools/llvm-c-test/debuginfo.c b/llvm/tools/llvm-c-test/debuginfo.c
index 35c65f885af32..ed1cb4e30223a 100644
--- a/llvm/tools/llvm-c-test/debuginfo.c
+++ b/llvm/tools/llvm-c-test/debuginfo.c
@@ -228,7 +228,24 @@ int llvm_test_dibuilder(bool NewDebugInfoFormat) {
LLVMPositionBuilderAtEnd(Builder, FooVarBlock);
LLVMTypeRef I64 = LLVMInt64TypeInContext(Ctx);
LLVMValueRef Zero = LLVMConstInt(I64, 0, false);
- LLVMBuildRet(Builder, Zero);
+ LLVMValueRef Ret = LLVMBuildRet(Builder, Zero);
+
+ // Insert a `phi` before the `ret`. In the new debug info mode we need to
+ // be careful to insert before debug records too, else the debug records
+ // will come before the `phi` (and be absorbed onto it) which is an invalid
+ // state.
+ LLVMValueRef InsertPos = LLVMGetFirstInstruction(FooVarBlock);
+ LLVMPositionBuilderBefore(Builder, InsertPos, true);
+ LLVMValueRef Phi1 = LLVMBuildPhi(Builder, I64, "p1");
+ LLVMAddIncoming(Phi1, &Zero, &FooEntryBlock, 1);
+ // Do the same again using the other position-setting function.
+ LLVMPositionBuilder(Builder, FooVarBlock, InsertPos, true);
+ LLVMValueRef Phi2 = LLVMBuildPhi(Builder, I64, "p2");
+ LLVMAddIncoming(Phi2, &Zero, &FooEntryBlock, 1);
+ // Insert a non-phi before the `ret` but not before the debug records to
+ // test that works as expected.
+ LLVMPositionBuilder(Builder, FooVarBlock, Ret, false);
+ LLVMBuildAdd(Builder, Phi1, Phi2, "a");
char *MStr = LLVMPrintModuleToString(M);
puts(MStr);
``````````
</details>
https://github.com/llvm/llvm-project/pull/92417
More information about the llvm-commits
mailing list