[PATCH] D100165: [GVNHoist] fix a bug where GVNHoist preserves the debug location when it should be dropped

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added subscribers: fhahn, vsk, debug-info, Orlando.
Orlando added a comment.

You're preserving the location in the case that only one instruction is moved. Interestingly the linked document also says "a transformation should also preserve the debug location of an instruction that is moved between basic blocks, if the destination block already contains an instruction with an identical debug location." suggesting that you should only preserve the location if "an identical one" exists in the pred block. Scanning the block for an identical location sounds excessive. My gut reaction would be to simply add an additional check that the location of the instruction at the insertion point matches the (single) hoisted instruction's location before preserving the location, or alternatively always drop the location (i.e. remove the `if` guard altogether).

Digging deeper, it looks like this was discussed in the patch that introduces the wording to the document (D93662 <https://reviews.llvm.org/D93662>). cc @fhahn @vsk @jmorse. wdyt should happen here?

An aside: while looking at this I found a function `Instruction::updateLocationAfterHoist()` which looks like it might be useful, but all it does is call `dropLocation()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100165