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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Orlando wrote:

> 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."
>
> 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?

IIRC, that wording means "It's sufficient to leave the DebugLoc unaltered if it's placed in a block where that DebugLoc already appears". It isn't necessary to scan for identical DebugLocs, instead it avoids un-necessary dropping of DebugLocs when we already know it won't mislead the developer.



================
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:1143-1146
+      // if there are multiple instructions to hoist, we should conservatively
+      // drop the debug location: according to the LLVM debug information update
+      // guide, if multiple identical instructions are hoisted into a
+      // predecessor block, the debug information should not be preserved.
----------------
Wording change suggestion above; I think it's clearer to just say "if instructions are hoisted". As I understand the HowToUpdate... document, we  drop source locations whenever we move instructions between blocks, no matter how many.

IMO, it's better to not refer to the debug-info-update-guide from source comments -- the comments should only describe the intention / purpose of the code. (This may be a style thing).


================
Comment at: llvm/test/Transforms/GVNHoist/hoist_debugloc.ll:3
+; COM: check whether the hoisted store instruction drops the debug location.
+; CHECK: store i32 1, i32* @{{[a-zA-Z_][a-zA-Z0-9_]*}}, align 4{{$}}
+source_filename = "abc.ll"
----------------
You can hard-code the "G" global variable name, LLVM is highly unlikely to change that as part of a future optimisation.


================
Comment at: llvm/test/Transforms/GVNHoist/hoist_debugloc.ll:35
+
+attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
+
----------------
Un-necessary attributes should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100165