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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

The syntax the LLVM project uses to reference bugs is "PR<number>" (eg: PR39531) not "pr/<number>", if you could update the patch description to match that format. (this is also supported on llvm.org, where you can go to `llvm.org/PR39531` to reach that bug, for instance)

So reading the bug, it sounds like the branch's location is used for the location of a sanitizer coverage function call ( __sanitizer_cov_trace_pc ) and that's where the verifier trips up? I guess it'd probably be worth fixing that code to do something to add a valid location to those function calls too - unless we want to broaden this constraint to apply to require that all branch instructions have debug locations too (which I'd be on the fence about - I was the one who implemented the "calls must have locations if the function they're in have a location" and its found a bunch of bugs, but it's been a non-trivial cost and I'm not sure it'd reasonably extend to all branches).

If we are going to require that all branches have locations (in functions that have debug info) then we probably want to add that to the verifier, so these aren't found late/only when using sanitizers, for instance.

If we aren't, then the sanitizer shouldn't rely on this guarantee/should be able to cope correctly when it a branch does not have a location.



================
Comment at: llvm/test/Transforms/JumpThreading/branch-debug-info2.ll:22-23
+; 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 ![[DBG:[0-9]+]]
+; CHECK: ![[DBG]] = !DILocation(line: 0, scope: !{{.*}})
+
----------------
If you like, you can include the ! in the match, eg: `[[DBG:![0-9]+]]` ... `[[DBG]] = !DILocation...` (no hard rule either way - lots of examples in both ways in LLVM's tests I'm sure, though I find it a bit neater to include the !, avoids needing to include it in both places or accidentally omit it in the second match & allow other prefixes (eg: `!345 = ... ` could match `[[THING]] = ...` where THING = `45`))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100137