[PATCH] D100137: [JumpThreading] merge debug info when merging select+br

Thu Apr 8 14:06:35 PDT 2021

dblaikie added a comment.

> Link: b/184085493
> Link: https://bugs.llvm.org/show_bug.cgi?id=39531
> Link: https://www.llvm.org/docs/HowToUpdateDebugInfo.html#when-to-merge-instruction-locations
> Signed-off-by: Nick Desaulniers <ndesaulniers at google.com>

We don't have any tag styling like this in LLVM - maybe include some sentences about these links? (& if you like you can write the bug number with "PR39531") - oh, and at least Google tries not to include links to internal bugs in upstream contributions (not an entirely consistent policy, Apple folks regularly link to their internal bug tracker & I'm sure there are some google bugs referenced upstream, though we try not to). And "Signed off by" seems inaccurate (& Phabricator will add "Reviewed By" I think anyway, which is usually the thing we might want to record)

Could you explain further why this could result in a verifier failure? I don't know of any reason a branch would /need/ a debug location. Call instructions need locations due to some inlining concerns/constraints, but I don't think we require all/other instructions to have locations.

================
Comment at: llvm/test/Transforms/JumpThreading/branch-debug-info2.ll:26
+; CHECK-NOT: br i1 %cmp3.i, label %for.inc, label %ptr_to_shadow.exit
+; CHECK: br i1 %cmp3.i, label %for.inc, label %ptr_to_shadow.exit, !dbg !29
+  %spec.select.i = select i1 %cmp3.i, i32 -1, i32 %conv7.i, !dbg !29
----------------
This looks incorrect - if this is the result of merging the select and br on lines 27 and 29, then the location can't be the existing !29 (because that would pick one line over the other, which can, in general, cause problems for profiles). Perhaps !29 on the output isn't !29 on the input - in which case (actually in any case) it'd be good to not hardcode !29, instead using a pattern match on the dbg attachment, and verifying the specific contents of that dbg attachment in another CHECK line. (I'm guessing it should show that this location will have line 0 to resolve the ambiguity)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100137