[PATCH] D106915: Add a DIExpression const-folder to prevent silly expressions

Mon Aug 2 08:32:00 PDT 2021

probinson added inline comments.

================
Comment at: llvm/test/DebugInfo/X86/convert-linked.ll:22
   call void @llvm.dbg.value(metadata i8 %x, metadata !13, metadata !DIExpression()), !dbg !14
-  call void @llvm.dbg.value(metadata i8 32, metadata !15, metadata !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)), !dbg !17
+  call void @llvm.dbg.value(metadata i8 32, metadata !15, metadata !DIExpression(DW_OP_deref, DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)), !dbg !17
   ret i8 %x, !dbg !18
----------------
aprantl wrote:
> This looks wrong: What is a DW_OP_deref of the constant 32 supposed to be?
It is supposed to be a way to keep the constant folder from firing.  It doesn't "mean" anything.  I can add a comment that says so?

================
Comment at: llvm/unittests/IR/MetadataTest.cpp:2912
+  Expr = DIExpression::get(Context, {dwarf::DW_OP_deref});
+  EXPECT_EXPR_FOLD(32, 117, 32, 117, 1);
+  EXPECT_EQ(NewExpr, Expr);
----------------
aprantl wrote:
> Thanks! Assuming these are dwarf expression ops, could spell them out here using the Dwarf.def enums? Or does that mess with the macro? If so, maybe we could at least add a comment expanding the raw numbers.
The values are parameters to the APInt ctor, meaning 32-bits wide with value 117 as input, and 32-bits wide with value 117 as output, and 1 element remaining in the expression afterward.  I don't pass the APInt directly because the macro expansion needs to know the bit-width of the result independently.  I suppose it could become
`EXPECT_EXPR_FOLD(APInt(32, 117), APInt(32, 117), 32, 1)`
if that would be easier to read?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106915/new/

https://reviews.llvm.org/D106915