================
@@ -1129,13 +1130,17 @@ static void
cloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
Instruction *NewBonusInst = BonusInst.clone();
- if (!isa<DbgInfoIntrinsic>(BonusInst) &&
- PTI->getDebugLoc() != NewBonusInst->getDebugLoc()) {
- // Unless the instruction has the same !dbg location as the original
- // branch, drop it. When we fold the bonus instructions we want to make
- // sure we reset their debug locations in order to avoid stepping on
- // dead code caused by folding dead branches.
- NewBonusInst->setDebugLoc(DebugLoc());
+ if (!isa<DbgInfoIntrinsic>(BonusInst)) {
+ if (!NewBonusInst->getDebugLoc().isSameSourceLocation(
+ PTI->getDebugLoc())) {
+ // Unless the instruction has the same !dbg location as the original
+ // branch, drop it. When we fold the bonus instructions we want to make
+ // sure we reset their debug locations in order to avoid stepping on
+ // dead code caused by folding dead branches.
+ NewBonusInst->setDebugLoc(DebugLoc());
+ } else if (const DebugLoc &DL = NewBonusInst->getDebugLoc()) {
+ mapAtomInstance(DL, VMap);
----------------
OCHyams wrote:
> For the first part I think that answers the question (multiple instructions
> with the same source-location getting is_stmt). Would there also be potential
> for an intervening non-is_stmt instruction with the same source line between
> the two? It feels possible from instruction scheduling if not immediately in
> this pass, and if so I guess it's a general consequence of the technique.
Yeah I think so. And again, kind of up to the debugger how to handle that IMO.
FWIW I'm pretty sure our debugger skips forward until the next is_stmt line
doesn't have the same line number.
> I was wondering whether there can be a case where the instruction at PTI and
> the bonus instruction are in the same group, but the one being remapped here
> has the higher rank, and putting it in a different group causes the
> previously-lower-ranked instruction to become the highest ranked as a result.
Ah I see what you're saying. Yep, this is possibly edge-case and a subtle
limitation of remapping in general. I figured that because it's a bit of a
corner case and at worst we see an extra step (not regressing from current
behaviour, just an extra step than key instructions might *ideally* want to
produce), it wasn't worth worrying about.
> At face value this isn't a problem because the whole point of this function
> is we're duplicating a code path; but could there be scenarios where LLVM
> uses this utility to implement a move (i.e. clone-then-delete-old-path?)
I guess in theory... in practice it's only reached through
`foldBranchToCommonDest`. I'm not sure if there's any way to detect/prevent
future user error here.
> Or to put it another way: I believe (but may be wrong) that
> cloneInstructionsIntoPredecessorBlockAndUpdateSSAUses currently copies/moves
> instructions from one place to another. But, with this change, what was a
> plain copy/move now comes with changes to the stepping behaviour.
It copies rather than moves (because there can be multiple preds that we copy
into). However, if there's only ever one pred then yes, this essentially
moves-by-copy, and updates atom groups (even though it doesn't really need to).
We could say "if there's one pred, then don't remap the atoms". I tested that
and it works. But with multiple preds this function gets called multiple times,
and in the final call there's only one pred. So that final pred doesn't get
remapped atoms. It's a subtle difference, but in terms of "correctness" I think
that's ok (it's as-ok as the existing situation).
> I think this boils down to saying "so what?", and the answer to that is "the
> stepping is still superior to without key instructions". I'm just trying to
> think through the consequences of how these changes compose together.
I'm leaning towards "so what" and leaving it as I have in this commit. But if
my response above sounds better to you, I'll happily make that change.
https://github.com/llvm/llvm-project/pull/133482
_______________________________________________
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
