[PATCH] D133118: Fix invalid llvm.dbg.declare after instcombine (#56807)

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This looks good; in particular thanks for writing tests for both code paths. The only thing that needs doing IMO is adding a FileCheck variable capture for the metadata node numbers (see the inline comment) to avoid the test failing if something unrelated in metadata changes.



================
Comment at: llvm/test/Transforms/InstCombine/no-undef-bitcast-to-gep-ret.ll:11
+; CHECK-NEXT:    %pixels1 = alloca [1500 x i8], align 8
+; CHECK-NEXT:    call void @llvm.dbg.declare(metadata [1500 x i8]* %pixels1, metadata !7, metadata !DIExpression()), !dbg !12
+; CHECK-NEXT:    %pixels1.sub = getelementptr inbounds [1500 x i8], [1500 x i8]* %pixels1, i64 0, i64 0
----------------
Tests for metadata nodes ("!7", "!12") shouldn't test the explicit number, because those can change when other parts of LLVM change, and we should avoid spurious test failures. Instead, it's best to capture the metadata numbers and check that they correspond to the right metadata node -- there's an example in D133095.


================
Comment at: llvm/test/Transforms/InstCombine/no-undef-bitcast-to-gep.ll:13
+; CHECK-NEXT:    %pixels1 = alloca [1500 x i8], align 8
+; CHECK-NEXT:    call void @llvm.dbg.declare(metadata [1500 x i8]* %pixels1, metadata !7, metadata !DIExpression()), !dbg !12
+; CHECK-NEXT:    %pixels1.sub = getelementptr inbounds [1500 x i8], [1500 x i8]* %pixels1, i64 0, i64 0
----------------
(Same as with the other test)


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

https://reviews.llvm.org/D133118